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

Passive event listeners #62

Closed
skworden opened this issue May 13, 2020 · 6 comments · Fixed by #421
Closed

Passive event listeners #62

skworden opened this issue May 13, 2020 · 6 comments · Fixed by #421
Labels
feature request New feature or request resolved This issue is resolved

Comments

@skworden
Copy link

I was wondering if there was a way to set all of the event listeners to passive? In the type EventStore the event options are defined but it doesn't' look like in the eventDispatch they are included. Plus I don't see away that you can set them. I'm hoping I overlooked something and there is a way to set them.

I'm getting some jank on mobile with a slider (3 full width image slides + some HTML). Also, I have a client that is adamant about the Google Lighthouse scores (Chrome->Inspector->Audit). This is the only thing I'm getting dinged for in best practices category.

@davidjerleke
Copy link
Owner

davidjerleke commented May 14, 2020

Hello @lifedup,

Thank you for your question. The answer is no, it's not possible to set all listeners to passive. As far as I understand, passive event listeners matter the most for event listeners that could affect performance like touch and wheel event listeners. So I don't think it makes sense to make all listeners passive by default. As described in this article:

Touch and wheel event listeners are useful for tracking user interactions and creating custom scrolling experiences, but they can also delay page scrolling. Currently, browsers can't know if an event listener will prevent scrolling, so they always wait for the listener to finish executing before scrolling the page. Passive event listeners solve this problem by letting you indicate that an event listener will never prevent scrolling.

With that said, Embla is using non-passive touch listeners by design. It's intentional.

prevent-intersecting-axis-scroll

This is because carousels that allow for scrolling in both directions simultaneously have horrible UX. If you've used native CSS scroll snaps in any project, it also prevents intersecting axis scroll just like Embla does.

Now I personally think it's unfortunate that tools like lighthouse can be quite blunt in their judgement and rank sites lower because of this. I don't necessarily think that using non passive event listeners is a problem. In most cases you won't even notice it unless you have heavy scroll listeners or similar. I think it's a much bigger problem when devs use heavy scroll listeners without throttles triggered 100 times a second with exaggerated scroll into view effects. But tools like lighthouse probably won't pick that up, even though it will be a much bigger performance hit on the browser.

If there is a workaround, I would definitely consider implementing it. I stumbled upon this on Stack Overflow but haven't tried it yet. I’m going to investigate this further 🙂.

I'm getting some jank on mobile with a slider (3 full width image slides + some HTML)

In order to debug this, I need information about what device and browser you're using. I also need to see your code to see how you've setup the carousel, and what could be the cause of the jank you're experiencing.

Kindly,
David

@davidjerleke davidjerleke added the investigating Issue is being looked into label May 15, 2020
@davidjerleke davidjerleke changed the title Passive event listners Passive event listeners May 15, 2020
@skworden
Copy link
Author

skworden commented May 15, 2020

Thanks for the detailed response. I'll look into a few css options on my end in the coming days. Maybe using css overscroll-behavior with touch-action might do the trick when trying to contain the scroll of an overflowed container.

I'm also not a huge fan of going strictly by the numbers that the tools like Lighthouse provide. They are great for guidance but should be used on a case by case basis. I'm using google maps and their own tool hits it big on accessibility and performance.

I was testing it on an iPhone 11 Pro Max in the latest Safari but the jank stopped when I push the react app to production.

@davidjerleke
Copy link
Owner

Hello again @lifedup,

I've experimented with the following properties in combination with passive touch listeners in many different ways, in order to recreate Embla's touch drag behaviour:

  • touch-action: pan-y
  • touch-action: pan-x
  • touch-action: none

...with no luck.

Any luck on your side?

Kindly,
David

@davidjerleke davidjerleke added awaiting response Issue is awaiting feedback and removed investigating Issue is being looked into labels May 23, 2020
@davidjerleke
Copy link
Owner

Hi @lifedup,

At this point I've experimented a lot with touch-action and passive listeners to no avail. If you find a way to reproduce the current Embla behavior with passive listeners - please share it. Until then, I'm saying no to this request because it destroys the UX of the carousel. In my opinion, to such an extent that it's unusable on touch devices.

Thank you for your request. Cheers!
David

@davidjerleke
Copy link
Owner

@lifedup, @dermotduffy I think I’ve found a way to solve this without changing the Embla behavior so I’ll implement it soon. Stay tuned.

@davidjerleke davidjerleke reopened this Jan 19, 2023
@davidjerleke davidjerleke added the upcoming A feature or bug fix is on its way for this issue label Jan 19, 2023
davidjerleke added a commit that referenced this issue Jan 19, 2023
@davidjerleke davidjerleke linked a pull request Jan 19, 2023 that will close this issue
davidjerleke added a commit that referenced this issue Jan 20, 2023
@davidjerleke davidjerleke added resolved This issue is resolved and removed upcoming A feature or bug fix is on its way for this issue labels Jan 20, 2023
davidjerleke added a commit that referenced this issue Jan 20, 2023
@davidjerleke
Copy link
Owner

This feature has been released with v7.0.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request resolved This issue is resolved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants