jquery-mousewheel icon indicating copy to clipboard operation
jquery-mousewheel copied to clipboard

Please make non-passive event listeners

Open ghost opened this issue 5 years ago • 13 comments

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: https://github.com/jquery/jquery-mousewheel/blob/fcb425e872c5b1eb45cbc4c8a5dcf4865b991bab/jquery.mousewheel.js#L40

ghost avatar Aug 27 '19 22:08 ghost

This requires changes to jQuery, planned for jQuery 4.0.0 right now. See https://github.com/jquery/jquery-mousewheel/issues/168.

mgol avatar Aug 27 '19 22:08 mgol

@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?

ghost avatar Aug 27 '19 23:08 ghost

That’s the jQuery issue: https://github.com/jquery/jquery/issues/2871

mgol avatar Aug 28 '19 08:08 mgol

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.

dmethvin avatar Aug 28 '19 22:08 dmethvin

@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 avatar Aug 28 '19 22:08 mgol

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.

ghost avatar Aug 29 '19 13:08 ghost

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.

dmethvin avatar Aug 30 '19 03:08 dmethvin

+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 ?

ghost avatar Aug 30 '19 11:08 ghost

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.

mgol avatar Aug 30 '19 12:08 mgol

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.

ghost avatar Aug 30 '19 12:08 ghost

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.

mgol avatar Aug 30 '19 12:08 mgol

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.

sanderkie avatar Jun 05 '20 12:06 sanderkie

This solve to me:

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

sergiorighi avatar Aug 16 '20 00:08 sergiorighi