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

Fix non-passive event listener on Chrome #264

Merged
merged 6 commits into from
Nov 24, 2018
Merged

Fix non-passive event listener on Chrome #264

merged 6 commits into from
Nov 24, 2018

Conversation

bianpratama
Copy link
Contributor

@bianpratama bianpratama commented Aug 17, 2018

Implement recommended practice by enabling passive events. This is related to #245 by @antonlukin. Somehow, with v3.2.3 I still get the warning:

[Violation] Added non-passive event listener to a scroll-blocking 'touchstart' event. Consider marking event handler as 'passive' to make the page more responsive.

Tried to add the passive event listener in event-binder.js, and it successfuly removes the warning.

Bian Pratama added 2 commits August 18, 2018 04:33
Implement recommended practice by enabling passive events.
Fix EventsBinder test to expect Object instead of Boolen
@bianpratama
Copy link
Contributor Author

@jedrzejchalubek, I also modified the EventsBinder test to pass Object instead of Boolean, I don't know if it's the right way.

Cheers 🍻

@jedrzejchalubek
Copy link
Member

Changes have been introduced very well :) However there is one problem that blocks me from adding passive events - browser support. Unfortunatly, passive events are not supported in IE11 version which this library needs to support for now.

These need to wait until we drop support for IE11 😢

Implement recommended practice by enabling passive events. This is related to #245 by @antonlukin. Somehow, with v3.2.3 I still get the warning:

I've actually answered wongly in this issue. My bad.

* Check if the browser supports passive events
* Revert the EventsBinder test to expect Boolean
@bianpratama
Copy link
Contributor Author

@jedrzejchalubek,

Yeah, I tried my change in IE11. And the drag event doesn't work smoothly, it keeps sticking to the cursor even when I don't click/hold the mouse.

Then, I tried to add browser's passive event support detection as suggested in MDN web docs and implemented in detect-passive-events. Now the passive event listener only enabled if the browser supports it.

I have tried to test it in IE11 on Windows 10, and it works for me. You can test it, and let me know if I need to refactor the code (newbie here 😁).

Cheers 🍻

@jedrzejchalubek
Copy link
Member

Oh, Great :) 🎉

One thing I would recommend here. Let's move an entire try-catch validation to a utility function named something like isPassiveEventsSupported which would return Boolean, then import it as a module and use directly in if statement.

With this, we actually decouple this fallback and make it easier, later on, to refactor if we actually drop support for IE11.

@garygreen
Copy link

One thing I would recommend here. Let's move an entire try-catch validation to a utility function named something like isPassiveEventsSupported which would return Boolean, then import it as a module and use directly in if statement.

Yeah that's good idea. Also I would add it's probably good idea to "cache" the result of it so any subsequent calls to isPassiveEventsSupported will just return true/false, in case you are adding multiple carousels to a page, for example.

@garygreen
Copy link

garygreen commented Aug 31, 2018

This is what I use in my app, just runs once and exports a boolean true/false so you can import it and re-use it wherever. Maybe it's useful?

detect-passive-events.js

var supportsPassive = false;

var opts = Object.defineProperty && Object.defineProperty({}, 'passive', {
    get: function() {
        supportsPassive = true;
    }
});

document.addEventListener('test', function() {}, opts);

export default supportsPassive;

Usage

import supportsPassive from './detect-passive-events';

panel.addEventListener('touchstart', function(eve) { }, supportsPassive ? { passive: true } : {});

@jedrzejchalubek jedrzejchalubek merged commit 556eb0c into glidejs:master Nov 24, 2018
@jedrzejchalubek
Copy link
Member

I have made latest adjustments to this feature and it's going to the next release :)

Thanks for your input @bianpratama @garygreen 💯

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

Successfully merging this pull request may close these issues.

3 participants