fix: ensure event context is reset before invoking callback

[trueadm]

Fixes #13684 and is a better approach than #13694 #13719.

Essentially, we have a complicated issue that only affects Chromium and they're well aware of it https://chromestatus.com/feature/5128696823545856. If you have an element with onblur, the event will fire synchronously if that element is removed from the DOM or if the element or its parent moves in certain cases. This behaviour was discovered because of the improved validation around block effects erroring on state mutations – but has nothing to do with this issue at all, it actually was great that it helped show this huge issue.

In the ideal world, event handlers are always without a reactive context – because this is how it works in 99% of cases. The case where it's not true is synchronous events handlers – either because of this strange Chromium bug or because someone does element.focus() or element.click(). If there is a reactive context, it's likely the wrong one that just happens to be active at the time of dispatch, which can cause very bad bugs:

• if the event handler reads state, the active context now has a dependency on it too, or you get an invalid mutation error from writing to state
• if the event handler creates an effect or derived, it's now incorrectly connected to the wrong thing

Now, how I thought we could tackle this is to wrap the call-sites where we do the actual DOM mutations, such as removals or moves with the try/finally logic that sets the context. However, I was recently made aware that these changes have caused performance to degrade significantly – our removal logic is now 3.3x slower than before! Try/finally wrapping clearly has overhead. For even more investigation I wrapped the each move function logic and saw what the impact would be on reversing an array of 100 items – it was 78ms, compared to 9ms. That's unacceptable.

So instead, what if we deal with removing the context when we invoke the event handler through Svelte's event system instead? This has the least overhead and works nicely in that it's predictable and can be applied to all events – which I think is desirable. I don't want an event handler to have a context, even if you call element.click() from within another context as it's a nightmare to debug these cases when it comes to event propagation. This also applied to our on event listener too.

However, that raises the question, what if someone does their own event handling without on? Well they'll unfortunately run into the same issues from before, and for that I have no good solution. FWIW this issue affects all the other signals frameworks too – and it causes the same buggy behaviour there in my testing. So it would be good to be the first framework to solve this from Chromium browsers.

[changeset-bot]

🦋 Changeset detected

Latest commit: 895e85b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[dummdidumm]

Given all the explanation I think this approach is the most sensible one. People may already run into subtle bugs when not using on, so to me it's okay that this one happens, too - I hope it's very rare to have addEventListener('blur', ...) anyway