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

Please make non-passive event listeners #202

Open
ghost opened this issue Aug 27, 2019 · 13 comments
Open

Please make non-passive event listeners #202

ghost opened this issue Aug 27, 2019 · 13 comments

Comments

@ghost
Copy link

ghost commented Aug 27, 2019

I believe I'm seeing a warning error in Google Chrome saying:

[Violation] Added non-passive event listener to a scroll-blocking 'wheel' event. Consider marking event handler as 'passive' to make the page more responsive. See https://www.chromestatus.com/feature/5745543795965952

This is pointing to the following code line:

this.addEventListener( toBind[ --i ], handler, false );

@ghost ghost changed the title Please make event listners passive Please make non-passive event listeners Aug 27, 2019
@mgol
Copy link
Member

mgol commented Aug 27, 2019

This requires changes to jQuery, planned for jQuery 4.0.0 right now. See #168.

@mgol mgol closed this as completed Aug 27, 2019
@ghost
Copy link
Author

ghost commented Aug 27, 2019

@mgol Do you have a link to an open issue, so we can track the progress. The one you linked to is closed and all the other issues linking to that one are also closed. I know you guys would have an open issue with a Milestone tag of 4.0.0 in the jQuery repo somewhere?

@mgol
Copy link
Member

mgol commented Aug 28, 2019

That’s the jQuery issue: jquery/jquery#2871

@dmethvin
Copy link
Member

This is an interesting case, since my proposed workaround employs special event handlers and the mousewheel plugin uses those already. You could make the mousewheel listeners passive, but currently it would force all jQuery mousewheel listeners on the same node to be passive regardless of whether they wanted it or not. It seems like most of the time there would not be multiple listeners though. So it might make sense for the mousewheel plugin to detect support for passive listeners if they're available.

@mgol
Copy link
Member

mgol commented Aug 28, 2019

@dmethvin Lots of mousewheel use cases implement custom scroll (something akin to fullPage) so we can't just use passive handlers if available. If we wanted, we could allow users to set an option that would switch the behavior. I'll reopen the issue to reconsider.

@mgol mgol reopened this Aug 28, 2019
@ghost
Copy link
Author

ghost commented Aug 29, 2019

If we wanted, we could allow users to set an option that would switch the behavior.

I think that would be the best solution.

For our CMS we use the mousewheel to scroll through a bunch of menu options. When doing some testing and setting the event listeners to passive it did reduce some jank issues and improve performance. So it would be great to have an option to turn it on/off as per use case.

@dmethvin
Copy link
Member

Any thoughts on the best interface to do this? There is no way in the jQuery special-event system to receive its own arguments via .on(). The easiest way is to define an event namespace with special meaning, for example:

// Attach a non-passive handler to #myBox, but only if
// it doesn't have a passive mousewheel event already
$("#myBox").on("mousewheel", function(e) { ... });

// Attach a passive handler to #yourBox, but only if
// it doesn't have a non-passive mousewheel event already
$("#yourBox").on("mousewheel.passive", function(e) { ... });

This makes the .passive namespace magic for mousewheel events, it has no special meaning to jQuery itself or to any other event type.

Another possibility is to pass it in the data argument and hope no mousewheel handlers are using or want to use that feature in a conflicting way.

// Attach a passive handler to #yourBox, but only if
// it doesn't have a non-passive mousewheel event already
$("#yourBox").on("mousewheel", { passive: true }, function(e) { ... });

I think the first option is a minor version bump and the second option is a major version bump due to our co-opting the user's data object in a potentially incompatible way.

@ghost
Copy link
Author

ghost commented Aug 30, 2019

+1 for the method:

// Attach a passive handler to #yourBox, but only if
// it doesn't have a non-passive mousewheel event already
$("#yourBox").on("mousewheel.passive", function(e) { ... });

I was playing around with your example: https://jsbin.com/bupesajoza/edit?html,js,output

Works well with no jank at all.

Stupid question, will this method be v3.4.1 and v4.0.0 compatible ?

@mgol
Copy link
Member

mgol commented Aug 30, 2019

It's hard to tell at this point how many breaking changes the event subsystem will have in jQuery 4.0 although we'll try to minimize them as much as possible within our constraints. It will certainly be compatible with 3.4.1; jQuery 4.0 will have a different native way to register passive event handlers & it's possible the mousewheel plugin will need adjustments, we'll see.

@dmethvin I like the namespace proposal, although technically it's a breaking change as someone may have been using such a namespace already. Maybe we should bump the major?

BTW, just to clarify - whatever solution we choose here, it's going to be slightly clunky. For a plugin it's not a huge deal, though - we can always do a breaking change & bump the major if we deem that necessary. With jQuery it's more difficult as it affects the whole broad ecosystem so we try to avoid temporary hacky APIs there. Whatever may work in the mousewheel plugin would not necessarily work in jQuery Core.

@ghost
Copy link
Author

ghost commented Aug 30, 2019

Sounds all good to me, we use Semantic Versioning for everything anyway. I guess when v4.0.0 comes out you could post two versions on the readme for this plugin just to let people know.

@mgol
Copy link
Member

mgol commented Aug 30, 2019

I think it shouldn't be an issue to support both jQuery 3 & 4 with the same plugin version; it's just that we're not sure yet if we will need any changes in the plugin to be compatible with jQuery 4.

@sanderkie
Copy link

Any updates on this?
The error/warning also shows in Lighthouse (google dev)

Does not use passive listeners to improve scrolling performance
Consider marking your touch and wheel event listeners as `passive` to improve your page's scroll performance. Learn more.

@sergiorighi
Copy link

sergiorighi commented Aug 16, 2020

This solve to me:

Line 40: this.addEventListener( toBind[ --i ], handler, { passive: false } );

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

No branches or pull requests

4 participants