Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Event bubbling can break in test conditions #4161

Closed
1 task done
jramanat opened this issue Oct 19, 2023 · 5 comments · Fixed by #4322
Closed
1 task done

Event bubbling can break in test conditions #4161

jramanat opened this issue Oct 19, 2023 · 5 comments · Fixed by #4322

Comments

@jramanat
Copy link

jramanat commented Oct 19, 2023

  • Check if updating to the latest Preact version resolves the issue

Describe the bug
In test scenarios, on a fast machine, it's possible for preact rendering and a subsequent programmatic call to occur within the span of a single millisecond (yay!). However, that can trip up the logic in #4126 because the _dispatched and _attached flags in the dispatched events and associated event listener props all end up getting the same value from Date.now(), resulting in any bubbling listeners getting short-circuited.

To Reproduce

Steps to reproduce the behavior:

  1. Go to https://codesandbox.io/s/infallible-https-2z9kls?file=/src/index.js and view the console.log

The test case loops 50 times, rendering a div containing an input, both with a 'focusin' listener, and immediately calling focus on the input. The listener on the input element is always called (since the new logic always invokes on the first listener), but on my machine, the listener on the div is usually not called.

Expected behavior
If you flip the preact version back to 10.17.1, you'll see 50 perfectly interleaved calls to the input + div.

Not sure if the spirit of the original fix could be maintained by just using an incrementing counter rather than Date.now() which I think would solve this case as well?

@ritschwumm
Copy link

ritschwumm commented Oct 20, 2023

it might be worse - afaik the resolution of Date.now() is more granular depending on an number of factors. in firefox it can e.g. be up to 100ms (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/now) and it might depend on the underlying operating system, too.

i used a tuple of a Date millisecond and an auto-incrementing 32 bit int (number|0) in a similar case. of course this is not a "real" solution because as soon as someone manages to do 2^32 operations within the same date granule, the problem will come back. it's much more unlikely, though.

p.s. sorry i didn't raise this when i first saw the PR being merged - i thought it might happen but didn't want to be a smartass :/

@jramanat
Copy link
Author

Thanks for the additional context! I can confirm that running my testcase in Firefox with privacy.resistFingerPrinting enabled, the bubbling almost always fails to work (48+ of the 50 runs).

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Oct 20, 2023

Those are all very valid points, I am going to give it some thought whether we can have a better solution to this 😅 basically something where we can indicate that a dom-node/event-handler was created later than when this event started bubbling up.

In real user scenario's this probably works out pretty well, however for this case in particular, the programmatic one it's a bit harder. In real interactions 100ms isn't even a problem imho

@NatteeB
Copy link

NatteeB commented Nov 23, 2023

@JoviDeCroock Is there any chance that Date.now() can be replaced with performance.now()? This change will not resolve the issue completely but it will improve the chances of the correct behavior.
I've just tried it with the above code sample. I can still see some failures to bubble up the event, but it succeeds in most of the cases.

@JoviDeCroock
Copy link
Member

performance.now() is significantly slower, we might need to experiment with some counter approach where the counter increments upon render flushes 😅 maybe a similar approach to this where it gets cached 😅

I haven't had much time to look into this but I acknowledge that issues in the computed scenario's are annoying. It however does solve a lot of real world scenario's from component libraries

Also feel free to look into this, the issues fixed are linked and should have adequate reproductions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants