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

passive event listeners #3720

Open
musicformellons opened this issue Nov 30, 2016 · 5 comments
Open

passive event listeners #3720

musicformellons opened this issue Nov 30, 2016 · 5 comments
Labels
needs discussion 💬 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage

Comments

@musicformellons
Copy link

musicformellons commented Nov 30, 2016

I am working with most.js stream framework to catch mapbox events in streams and stumble on this (since I got the message in my browser)
http://stackoverflow.com/questions/39152877/consider-marking-event-handler-as-passive-to-make-the-page-more-responive

So are passive event listeners implemented yet in mapbox gl js?

@mourner
Copy link
Member

mourner commented Nov 30, 2016

I would guess that this is not applicable to GL JS since we rely on preventDefault for touchmove and wheel events. Not sure though.

@mourner mourner added needs discussion 💬 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage labels Nov 30, 2016
@jfirebaugh
Copy link
Contributor

jfirebaugh commented Mar 23, 2017

This is related to #4259, but independent. Specifically, we could opt to pass {passive: true} in cases where we don't need to use preventDefault. Doing so might provide a performance benefit in some situations.

Because EventListenerOptions is not available on all targeted browsers, we will need to conditionalize its use, likely by writing internal utility methods wrapping {add,remove}EventListener with a feature detect. (Typical polyfills are not appropriate, since libraries such as Mapbox GL JS should not modify the global environment.)

@mourner
Copy link
Member

mourner commented Apr 24, 2017

Upgraded to Chrome 58 and it started complaining about this:

scroll_zoom.js:49 [Violation] Added non-passive event listener to a scroll-blocking 'wheel' event. Consider marking event handler as 'passive' to make the page more responsive.
scroll_zoom.js:50 [Violation] Added non-passive event listener to a scroll-blocking 'mousewheel' event. Consider marking event handler as 'passive' to make the page more responsive.
drag_pan.js:59 [Violation] Added non-passive event listener to a scroll-blocking 'touchstart' event. Consider marking event handler as 'passive' to make the page more responsive.
touch_zoom_rotate.js:54 [Violation] Added non-passive event listener to a scroll-blocking 'touchstart' event. Consider marking event handler as 'passive' to make the page more responsive.
bind_handlers.js:33 [Violation] Added non-passive event listener to a scroll-blocking 'touchstart' event. Consider marking event handler as 'passive' to make the page more responsive.
bind_handlers.js:35 [Violation] Added non-passive event listener to a scroll-blocking 'touchmove' event. Consider marking event handler as 'passive' to make the page more responsive.

@GLosch
Copy link

GLosch commented Jul 17, 2017

I'm getting the same behavior @mourner since adding map.scrollZoom.disable();.

Was hoping to be able to add {passive: true} as an option to that function, but it's not set up to accept it

@jfirebaugh
Copy link
Contributor

#6248 will silence the log messages from Chrome.

As a further step, we could explicitly specify {passive: true} or {passive: false} for every DOM event used. This would require a careful audit to figure out which events may need to preventDefault. The advantages would be:

  • Theoretically better performance where {passive: true} is used, because the browser will know that these event bindings can't prevent default behavior.
  • The use of {passive: false} where necessary will protect us against potential future browser changes described in Touch listeners defaulting to passive WICG/interventions#18.

The caveat to this is that it'll have publicly visible side effects, namely whether or not e.originalEvent.preventDefault() works if called from e.g. map.on('touchmove', (e) => { ... }.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion 💬 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

No branches or pull requests

4 participants