Attach touchstart/touchmove/wheel listeners as passive (#428)#5165
Attach touchstart/touchmove/wheel listeners as passive (#428)#5165JoviDeCroock wants to merge 1 commit into
Conversation
Matches React 18: these scroll-blocking events are registered with
{passive: true} so they can't block compositor scrolling, silencing
Chrome's non-passive listener violations and Lighthouse's
uses-passive-event-listeners audit. preventDefault() in these handlers
becomes a no-op; attach a non-passive listener via a ref when
cancelation is needed.
+32 B brotli (4456 -> 4488).
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Resultscreate10kduration
usedJSHeapSize
filter-listduration
usedJSHeapSize
hydrate1kduration
usedJSHeapSize
many-updatesduration
usedJSHeapSize
replace1kduration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
text-updateduration
usedJSHeapSize
tododuration
usedJSHeapSize
update10th1kduration
usedJSHeapSize
|
|
Size Change: +39 B (+0.25%) Total Size: 15.9 kB 📦 View Changed
ℹ️ View Unchanged
|
|
coverage: 98.534%. remained the same — JoviDeCroock/issue-428 into main |
|
The linked issue is asking for the ability for the user to mark a listener as passive, which this PR doesn't really support, and deviating from standard behavior to instead match React (which has been criticized over this) in core seems like it might be a bit much to me; a whitelist of events that automatically get different behavior without the author opting in is, I think, unexpected outside of compat for users. As for opting in, admittedly I'm not sure of any good solution there besides having the user rely on refs, can't really support an options object via altered prop names alone as there's way too many combinations. |
|
I was thinking so at first as well but I couldn't come up with many cases where you don't want these interactions to be passive 😅 so I thought maybe mirroring React was sane. In Vue they make it part of the event-name https://github.com/vuejs/core/blob/main/packages/runtime-dom/src/modules/events.ts#L73 |
rschristian
left a comment
rschristian
left a comment
There was a problem hiding this comment.
Same, but I can imagine there's some people out there who will want it and we generally get points on social media for not matching behavior like this 😅
I suppose I'd support this though, it's fairly limited and is trying to provide a better default, even if that default deviates. You convinced me.
As is we should probably unlink the issue though as this only partially addresses it (mostly the later comments in that thread over chrome warnings).
The naming thing seems harder if we want to combine options from the options bag (...CapturePassive?), but for single values it's probably fine, could offer it for all our handlers too like Vue
|
Yes, the vue thing is also elegant in a way but thinking about the types it might be... tedious and an expensive matcher. Gonna think some more about this in the next week. |
|
I think making these three events passive by default can be a reasonable default, but I'm not sure the available browser data is enough to conclude that active handlers are rare for Preact's case. Chrome's figures for touch and wheel listeners only cover listeners registered on root targets ( This makes me think the React-compatible default may be appropriate for There may also be a separate compatibility concern. The PR passes an EventListenerOptions object unconditionally, while the current build targets still include Chrome 40, Safari 9, Firefox 36, and Edge 12: https://github.com/preactjs/preact/blob/main/scripts/build.mjs#L33-L39 The EventListenerOptions explainer notes that older browsers interpret an object in the third argument as React performs that feature detection and falls back to the boolean capture argument when passive listeners aren't supported: https://github.com/react/react/blob/main/packages/react-dom-bindings/src/events/checkPassiveEvents.js Are those older targets still part of Preact's runtime support policy? If so, would this need a feature-detected fallback? Preact's test utilities already contain the same getter-based feature detection in It may be a useful project-local reference if a runtime fallback is needed, but an equivalent runtime check would of course need to be evaluated against the core size budget. |
|
Our browser target has since moved to modern browsers, we haven't put an explicit definition to that yet though but IE11 is dropped so Safari becomes the bottle-neck in most cases. I can see what you're saying and maybe we should skip this, I do think that having these magic keywords on events is going to be cumbersome especially since most of our counter-parts don't seem to be doing it. |
The browser versions they list (Chrome 40, Safari 9, Firefox 36, and Edge 12) are the browser versions we decided on earlier this year, same versions listed in the upgrade guide. IIRC matching the browsers that support It's a good point though that
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#browser_compatibility TIL, do we even need to include those events here then? If modern browsers will handle that for us, and realistically that's where most users are at, we can get them for free. |
4 participants
Resolves #428
Matches React 18: these scroll-blocking events are registered with {passive: true} so they can't block compositor scrolling, silencing Chrome's non-passive listener violations and Lighthouse's uses-passive-event-listeners audit. preventDefault() in these handlers becomes a no-op; attach a non-passive listener via a ref when cancelation is needed.
React