Allow use passive listeners

sergeymorkovkin commented 5 years ago

Bootstrap native now only uses non-passive listeners, which decreases PageSpeed Insights score and potentially lowers scroll performance. Suggest implementing an options allowing to use passive listeners. All is takes is adding {passive: true} as a third argument passed to addEventListener.

More:

sergeymorkovkin commented 5 years ago

I'm now patching build script output. According to my preliminary tests, following event handlers should be passive:

- touchDownHandler
- touchMoveHandler
- touchEndHandler
- pauseHandler
- resumeHandler

thednp commented 5 years ago

Sup. According to this MDL page our code should look something like this.

someElement.addEventListener("mouseup", handleMouseUp, passiveSupported
                               ? { passive: true } : false);

What do you think?

sergeymorkovkin commented 5 years ago

Your code is correct, but not all events should be passive. E.g. event handlers that use e.preventDefault() should not be passive for sure. What I do with compiled bundle is this:

        bundle = bundle.replace(/(element, event, handler)(\)\s\{\n)/g, '$1, options$2');
        bundle = bundle.replace(/(event, handler), false/g, '$1, options ? options : false');
        bundle = bundle.replace(/(touchDown|touchMove|touchEnd|pause|resume)(Handler)\s*\)/g, '$1$2, {passive: true} )');

It works pretty well.

thednp commented 5 years ago

I mean, I wanna know how the code should look like in our library to comply :)

sergeymorkovkin commented 5 years ago

It looks totally okay for me! )

thednp commented 5 years ago

Sup @sergeymorkovkin @midzer I will need your help a bit.

download latest master
check out the new on/off utils
let me know which components and their events should look like

Thanks

sergeymorkovkin commented 5 years ago

@thednp, I only use Modal and Carousel components on my website. You can take a look how it works here: https://edgeof.it. Pulled latest master and can't event find the word "passive". Do you ever use it somewhere?

thednp commented 5 years ago

No not yet, I've only prepared the on/off event utilities. That's why I want you guys to have a look and come with suggestions.

sergeymorkovkin commented 5 years ago

Hm, it seems to me that we should have a default behavior (automatically setting passive mode depending on the environment), but still give control to the user. What do you think? Utilities look good to me.

вт, 28 мая 2019 г. в 16:42, thednp notifications@github.com:

No not yet, I've only prepared the on/off event utilities. That's why I want you guys to have a look and come with suggestions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/thednp/bootstrap.native/issues/283?email_source=notifications&email_token=AAOUOFQPHSVJYM53I7F74ATPXUZFFA5CNFSM4HIB7LS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWMFESY#issuecomment-496521803, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOUOFT6HDHCHMSP2UEFBVDPXUZFFANCNFSM4HIB7LSQ .

-- Сергей Морковкин www.morkovkin.info +38 050 445 01 45

thednp commented 5 years ago

That's why I ask you guys make an input.

sergeymorkovkin commented 5 years ago

Okay, here is my input. In my application I'm using {passive: true} always, which means it will not work in browsers that don't support it (see https://caniuse.com/#search=passive). The only reason to use passive event listeners is performance. By setting event listener passive you simply let browser know that you are only interested in being notified and that's it. You will not call preventDefaut(). This allows browser make various internal optimizations. But since you're publishing a library it's a matter of good tone to let user decide (even though all event listeners are in our own scope). I'd personally enable it for all mouse/wheel events by default, but only for browsers that support it.

вт, 28 мая 2019 г. в 17:13, thednp notifications@github.com:

That's why I ask you guys make an input.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/thednp/bootstrap.native/issues/283?email_source=notifications&email_token=AAOUOFQXZQ53TLPMXP5PDMTPXU4W3A5CNFSM4HIB7LS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWMIHZI#issuecomment-496534501, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOUOFRUCXDEKLFW6BBVO6DPXU4W3ANCNFSM4HIB7LSQ .

-- Сергей Морковкин www.morkovkin.info +38 050 445 01 45

thednp commented 5 years ago

I'm now patching build script output. According to my preliminary tests, following event handlers should be passive:
- touchDownHandler
- touchMoveHandler
- touchEndHandler
- pauseHandler
- resumeHandler

Are these the only handlers to work with passive event option? Also need to know what other event options might be needed.

thednp commented 5 years ago

@sergeymorkovkin @midzer and anyone else please check and test latest master code, I need your confirmation this is implemented properly before pushing to npm.

Thanks

midzer commented 5 years ago

Using latest master of bsn at my site https://feuerwehr-eisolzried.de/ still shows a [Violation] in Console with (verbose) full level logging enabled:

"Added non-passive event listener to a scroll-blocking 'touchstart' event"

Sorry, that I can't help any further right now.

thednp commented 5 years ago

@midzer solved, seems our new utility was missing ONE word. The verbose notice is now gone.

midzer commented 5 years ago

Thanks. I can double check new passive feature tonight

thednp commented 5 years ago

Great also @sergeymorkovkin please have a minute to test ;)

midzer commented 5 years ago

Hmm, somehow this still doesn't work out for me. Maybe some1 else can confirm?

thednp commented 5 years ago

On my demo you have the scroll triggered ScrollSpy component using passive handler, verbose mode shows no problem.

midzer commented 5 years ago

Yeah, everything fine now with the passive event listener :-)

(I had to clone repo in node_modules because somehow yarn mixed up those source files)

thednp commented 5 years ago

Let me know your final thoughts on the code and improvement opportunities.

midzer commented 5 years ago

Regarding passive listener support checking, code looks pretty good for my taste. Copying from WICG explainations should be safe, never had something been broken from there...

Good thing you've commented everything properly :-)

thednp commented 5 years ago

@sergeymorkovkin @midzer and everyone else, please confirm: are we ready to push the next version to the cloud?

midzer commented 5 years ago

I don't want to give a final word here as there have been way more changes lately and I didn't test all existing features yet. I remember last year there has been a regression, because we didn't test everything and I said we ready to go...

All I can tell I have latest master (with custom webpack build ignore: ['scrollspy', 'popover', 'alert', 'affix', 'toast']) running properly at my productive site https://feuerwehr-eisolzried.de

thednp commented 5 years ago

My question is, in this case, strictly tied to the feature requested with this issue.

thednp commented 5 years ago

Suggestion implemented, time to close.

midzer commented 5 years ago

Sorry to bring this up again.

Somehow, after an Chrome audit all levels Console still complains about

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

in my App https://feuerwehr-eisolzried.de/ In regular usage of site, this does not appear, only for the audit.

I did some debugging now, but I'm kind of stuck here.

Have a nice week & cheers midzer

thednp commented 5 years ago

Please test https://thednp.github.io/bootstrap.native/ on your machine and confirm the issue. I cannot confirm such issue for some reason. I believe you might be using an older version of the library.

midzer commented 5 years ago

I've just cleared yarn cache, fetched all npm packages (+ BSN master) and re-uploaded all JS to my site.

Yet the problem still occurs. I wonder why this does not happen on demo page...

Strangely, after implementing this feature the issue had been gone. I wonder if this might be a browser regression somehow, using Chrome 78 dev build.

thednp commented 5 years ago

But the latest code isn't published on npm/CDN yet man, I suggest getting the latest MASTER/dist version.

midzer commented 5 years ago

@thednp I pulled latest git via "bootstrap.native": "git+https://git@github.com/thednp/bootstrap.native.git",

thednp commented 5 years ago

I don't understand why you still get that warning, please tell me does the DEMO page also throws it?

midzer commented 4 years ago

Finally, I've figured out what has been wrong:

Somehow, yarn pulled (old?) version files from GitHub repository. I switched to npm again and everything is fine.

thednp / bootstrap.native

Allow use passive listeners #283