Fix: Actions with options may run out of order

[flickgradley]

Problem

I was exploring implementing a new action option -:stopImmediate - and in working on that, I ran across what I believe is a bug in action ordering when options are provided.

Consider the following, assuming there is a controller called a:

<input data-action="keydown->a#firstAction:stop">

Then let's say I modify the element with JS dynamically to have a second action like this:

<input data-action="keydown->a#firstAction:stop keydown->a#secondAction">

When I run these actions, I would expect them to run in left-to-right order, as specified in the docs. However, that doesn't happen - the actions run in secondAction, firstAction order. Technically, the stop actually happens in between the secondAction and firstAction, since it runs before the action it modifies.

Cause

A single event handler is attached on an element for each event type + action option(s) combination. This makes sense for options which are passed to the native DOM handlers - we need these to be separate event handlers under the hood, as they are constructed differently.

For both the "standard" Stimulus set (currently :stop,:prevent, :self) and the user-defined option set added via registerActionOption(), the underlying event handler could be the same as in actions without these options.

Proposed Solution

I modified the Dispatcher#cacheKey method to only create separate keys (and therefore, separate event handlers) when using the 4 action options which are passed to the native addEventListener. This lets us run more (but, as mentioned below, not all) actions in the correct sequence.

Caveats

This solution does not fix the issue if I replace the :stop in my example above with :once - any option handled natively by the browser could still run in an unexpected order. I still believe this is valuable because it patches several (most?) usages (and opens the way for a :stopImmediate action option that stops further processing as expected, without having to call event.stopImmediatePropagation() in the action).

The more general case is much harder to fix. We could maintain a single event listener per event type (ignoring action options), and implement the :once ourselves - but that doesn't help with :passive (though I suppose it'd be unusual to attach a :passive handler alongside something that's not :passive for the same event, that would seem to defeat the point).

Maybe we could track which bindings have already executed for a given event, but that's its own can of worms.

Or, we could move towards a separation of native browser event attributes and Stimulus- or user-implemented actions. Perhaps putting browser-driven modifiers next to the event type itself could help since they apply at a deeper level - ex. keydown:once->a#firstAction)

Then, the section after the -> could support a larger set of options / filters. Something like keydown:once->beforeOption:a#firstAction:stop. (I personally find it unintuitive that a#firstAction:stop means that stop runs before firstAction)

Anyway - that's all a much larger discussion that may be out of scope here. I do think this patch is worthwhile for making more of the action + option combos run in the expected order today.

[marcoroth]

Hey @flickgradley, thank you so much for taking this one on, this is quite a tricky one.

I think your proposed solution makes sense, unless we can make a more general solution work. For that case I wonder if either @seanpdoyle or @adrienpoly has an idea on how we could to achieve this.

We had some related discussions in #530 and #546 regarding where to put the options / filters.

[adrienpoly]

Thanks @flickgradley for this detailed investigation and it is clearly a tricky edge case. While it does not fixes all cases, I agree that your fix is an improvement.

A complete fix probably means a profound change in the way the actions are tracked.

To my knowledge adding dynamically actions in JS is not something really documented. Maybe a solution could be to document this part. We could mention that adding manually actions in JS can lead to incorrect order for action processing and should be used with great care.

[adrienpoly]

I thought from reading the PR that this error would come up only when a new action was set dynamically to the dom. But as the test from this PR and this new issue #718 demonstrates, having actions like that : "c#log3:prevent c#log d#log2". Currently runs out of order.

While it is not a 100% fix for all cases, I think this solves the majority of the issues we could expect with event modifiers and action ordering