search

mercs600 / vue2-perfect-scrollbar

Vue.js wrapper for perfect scrollbar
https://mercs600.github.io/vue2-perfect-scrollbar/
MIT License
275 stars 43 forks source link

Adding Passive event listener options #54

Closed EmilsM closed 4 years ago

EmilsM commented 4 years ago

Setting Passive Event Listener options to false, because Chrome, in the newest versions, sets events as Passive. This means, that e.preventDefault() cannot be used and that throws an error in the console, and, when scrolling the container, scrolls the entire page.

mercs600 commented 4 years ago

thanks @EmilsM good point, but please notice that this library just importing the original perfect-scrollbar lib and this PR should be report there. What's more I see they are working on it: https://github.com/mdbootstrap/perfect-scrollbar/pull/945 - so when they merge this PR I will upgrade this library.

andrei-gheorghiu commented 4 years ago

"They are working on it!" is a gross overstatement.

What happened was that I (an anonymous Joe) have read the documentation around the warning and have applied the MDN suggested feature detection before applying the listener and it seems to work as expected: warning gone, all good in all browsers (including the ones without passive feature).

Since it seemed like a good change, I wanted to share, so I tried to PR it into the library repo, but I can't seem to get it in a form which would pass the project linting. So far there hasn't been much help, although I did ask for it.

But I've been using the fix for some time now, in multiple applications (3, to be exact), 1 of them with very large public exposure), no warnings, everything works as expected.

To get the patch before it gets merged into the library, use:

"vue2-perfect-scrollbar": "github:andrei-gheorghiu/vue2-perfect-scrollbar#develop",

// or for the original perfect-scrollbar package:
"perfect-scrollbar": "github:andrei-gheorghiu/perfect-scrollbar#develop",

in your package.json.

Goes without saying: I welcome any help in getting a) steps to get perfect-scrollbars format:lint script to run correctly (it complains about not matching any files) or b) my changes into a form which would pass their linting setup - PR into my fork's develop branch

Edit: nevermind, I figured it out. All tests pass now, so it will make it into the lib at some point. Cheers!

andrei-gheorghiu commented 4 years ago

@EmilsM , simply replacing false with {passive: false} won't cut it (on browsers without support for object syntax it will revert the intended value, as {passive: false} is truthy). So you need feature detection.

When (and if) my PR gets merged into perfect-scrollbar, vue2-perfect-scrollbar will also get the fix.

hmoffatt commented 3 years ago

According to https://caniuse.com/mdn-api_eventtarget_addeventlistener_options support for the options/boolean is pretty widespread @andrei-gheorghiu - do you think the feature detection is important?

andrei-gheorghiu commented 3 years ago

@hmoffatt yes, I do. It's about supporting legacy browsers. There are two aspects to consider here:

a) the importance of supporting legacy browsers varies from client to client. Although, a client with outrageous requirements from this POV (e.g: an energy supplier, national bank, post office) will never use this library anyways, specifically because they want to support very old browsers to serve every single request, even if made in ancient browsers.

b) the other important aspect here is that, in legacy browsers, passing the new syntax ({ passive: true | false }) instead of the old syntax: true | false, actually means true, because an object is always truthy. For this particular library, that is a problem. Passing passive always true means it won't be able to call preventDefault on scroll, which means it won't work. As in: "in legacy browsers it will completely stop working, although it used to work until now".

So yeah, I'd say feature detection is sort of important on this one. What do you think?