bigskysoftware / htmx

</> htmx - high power tools for HTML
https://htmx.org
Other
38.52k stars 1.31k forks source link

Wrong detail.elt for htmx:beforeSwap #3000

Open benvcutilli opened 2 weeks ago

benvcutilli commented 2 weeks ago

Hi, I am trying to write an extension using the htmx:beforeSwap event, and it appears that line 4714:

if (!triggerEvent(target, 'htmx:beforeSwap', beforeSwapDetails)) return

ends up resulting in detail.elt being set to target (as target is passed into the first parameter of triggerEvent, which sets beforeSwapDetails.elt to it). However, other calls to triggerEvent pass in elt instead. Also, in the documentation at https://htmx.org/events/#htmx:beforeSwap, it says that detail.elt is supposed to be "the element that dispatched the request". This makes more sense to me because it makes beforeSwap more useful (you get to additionally access the request element), but also because detail.target already exists and can be used.

Is this intended behavior? As stated earlier, it can be really difficult to take action on this event if you don't know which element sent the request.

Telroshan commented 2 weeks ago

Hey, indeed it's likely a bug, especially as the documentation says the opposite of the current behavior. As you said, we have detail.target available so it doesn't make much sense to double down with an extra reference to it in detail.elt. Feel free to open a bugfix PR!

MichaelWest22 commented 2 weeks ago

From investigating why htmx is doing this i found this commit from 2020 https://github.com/bigskysoftware/htmx/commit/6f14cba2e65356844ae1bcb7ac5b2feb01dcffd1 It changes it from elt to target with the comment

Trigger events on new content, rather than triggering element, since triggering element may have been replaced

see #79

It seems that often the element has already been replaced at this stage so elt is no longer a valid and so the event has to fire on the target that is being swapped instead of the old possibly removed element. which can cause events to get lost it seems.

So i think the documentation instead needs updating to make this 4 year old behavior clearer.

I also think you can probably reliably find the requesting element in the additional detail provided in the event.

try detail.requestConfig.elt

benvcutilli commented 2 weeks ago

That commit isn’t for the same lines, I think? (EDIT: nvm, that's a very old commit with many fewer lines, so it ends up being the right one) Besides, I don’t think this is intuitive behavior; changing the semantics of the same name (detail.elt) across events can be pretty unwieldy, especially when detail.target already exists, which would suggest that detail.elt should be something different. I feel as though it should be set to something like null and therefore the burden should be on the front end to check for this. It also indicates to the developer that something got removed from the DOM, instead of being confused as to why they are getting the same element twice and quadruple checking their attributes because they think they’re going crazy, as I was doing.

benvcutilli commented 2 weeks ago

I am also not sure why beforeSwap would be affected by this. In theory, none of the elements should have changed at that point, right? Or is this due to the event loop queueing events but getting to the swap before the events actually fire? I would think this latter scenario wouldn’t happen given that event was specifically made so that people can change the swap behavior.

MichaelWest22 commented 2 weeks ago

yeah re-looking at the commit I think it was in fact the afterSwap events that were broken and not the beforeSwap events. But it looks like that when he switched afterSwap to the destination element and not the requesting one to solve the issue the beforeSwap was also changed to the same to be symmetrical probably. So it probably didn't have to be changed originally in theory. But now that its been like this since almost the first version of htmx there will be many applications that now depend on their beforeSwap event working with the event listeners setup on the target elements. The problem is the detail.elt field is not just a data field but it is also used as the target for where the event is fired so changing its data also changes how people listen for the event :(

Also while the documentation seems to have detail.elt as a very consistent description of the element that dispatched/triggered the request for a total of 32 times which is why you expected to be able to depend on this being the default for most events when I went though all the events in the source one by one I found the following:

Events where documentation says detail.elt is "the element that dispatched the request" but its the impacted element or target instead:

htmx:oobBeforeSwap
htmx:oobAfterSwap
htmx:afterSwap
htmx:afterSettle
htmx:beforeProcessNode
htmx:afterProcessNode
htmx:validation:failed
htmx:validation:halted
htmx:beforeSwap

Events where the detail.elt is the one the dispatched the request so documentation is fine:
htmx:validateUrl
htmx:prompt
htmx:configRequest
htmx:validation:halted
htmx:beforeRequest
htmx:xhr:abort
htmx:xhr:loadstart
htmx:xhr:loadend
htmx:xhr:progress
htmx:beforeSend
htmx:beforeOnLoad
htmx:beforeTransition

This event the documentation is often correct but depending on the hx-sync setting the detail.elt can sometimes be the sync element instead:
htmx:abort

These events the detail.elt is often the one that dispatched the request but if the item is removed they are instead set to the closest parent:
htmx:afterRequest
htmx:afterOnLoad

So as you can see the documentation is only about 50% right which is why the documentation probably needs updating.
Do you think it would be helpful to as well as updating the beforeSwap event documentation also adding the requesting elt as another alias name in the event data like requestElt? And then calling this out in the documentation. Adding a new field to the event won't be a breaking change.

benvcutilli commented 2 weeks ago

I’d hate to clutter up the event with another attribute. So, I think if this were to be implemented, I think it would make sense to make it more comprehensive and have more purpose than fixing this one issue. Given that, having an attribute on all events that stores a deep-ish copy of the element that made the request (so that it doesn’t disappear because it got swapped) would be good, I think. Is requestConfig.elt this already?

The reason why is that I could see there being cases (my extension is one of them) in which the users of the events for which the element has been deleted might still need attributes on that element to properly do what they want to do. After all, that element does contain almost all the meta information about the htmx action taken, even things that were written for an extension, which, therefore, htmx (I would think) doesn’t track.

In theory, it would make the most sense to save in that variable the element that also references the state of the DOM at that time so that all the information is available, but I don’t know how realistic that is from a memory usage perspective; maybe it actually isn’t so bad if, say, it’s a kind of diff between the old and the new.