Safari 13.x t.addEventListener is not a function

hamidrezahanafi commented 10 months ago

Bug is related to

[x] embla-carousel (core package)

Embla Carousel version

v8.0.0-rc11

Describe the bug

t.addEventListener is not a function on Safari 13.x This comes adding event to mediaHandlers mediaHandlers.add(query, 'change', reActivate));

Rest of the error: `TypeError: t.addEventListener is not a function. (In 't.addEventListener(r,o,i)', 't.addEventListener' is undefined)

./node_modules/embla-carousel/embla-carousel.esm.js at line 192:10

function EventStore() { let listeners = []; function add(node, type, handler, options = { passive: true }) { node.addEventListener(type, handler, options); listeners.push(() => node.removeEventListener(type, handler, options)); return self; }`

CodeSandbox

It only happens on older browsers

Steps to reproduce

Go to '...'
Click on '....'
Scroll down to '....'
See error

Expected behavior

Is it possible to support older browsers?

Additional context

Add any other context about the problem here, like screenshots or similar.

hamidrezahanafi commented 10 months ago

@davidjerleke I added a PR to check if the addEventListener exist before calling it that might help just not throw errors on older browsers and work limited https://github.com/davidjerleke/embla-carousel/pull/561

davidjerleke commented 10 months ago

Hi @hamidrezahanafi,

First of all, thank you for wanting to contribute to Embla Carousel. The intention to help improve this library is always appreciated 👍!

A friendly reminder: Before starting any work, always start a new discussion first. The reason why I'm mentioning this is because this could prevent you from doing unnecessary work. Looking at the usage statistics, it seems like iOS13 is not quite common from 2022 and forward, and as you said it's a quite old version. I'm not convinced that we should go through the trouble solving this:

Best, David

7iomka commented 10 months ago

Hi everyone. Related issue with possible solutions: https://github.com/microsoft/TypeScript/issues/32210

I'm always in favour of supporting new features, but usually when it comes to complex properties like flex's gap - it requires a lot of overhead and in such cases it's worth explicitly defining the list of supported browsers on the product page to avoid wasting energy and bloating the codebase with unnecessary fallbacks. However, in this case we have a well-known exception to the rule.

People knowing about this use either an outdated method or make a wrapper with a simple check and a small fallback.

My proposal is to add a small fallback in your helper function code - it's not difficult for you and won't add a lot of code to the bundle but unlike the current behaviour or the proposed PR - it will work everywhere.

before

node.addEventListener(type, handler, options)
listeners.push(() => node.removeEventListener(type, handler, options))

after


if ('addEventListener' in node) {
      node.addEventListener(type, handler, options)
     listeners.push(() => node.removeEventListener(type, handler, options))
} else {
     node.addListener?.(handler);
     listeners.push(() => node.removeListener?.(handler))
}

If for some reason such a condition may seem redundant to you - I'll accept even the current PR I want to make progressive improvements, not breaking ones.

hamidrezahanafi commented 10 months ago

Hey @davidjerleke yeah will start the discussion first in the future Thanks for the reminder. The solution from @7iomka also seems reasonable too. My intention is more to fail-safe instead of a hard crash. I am using it now and seeing 10/20 hard crashes per hour because of this issue. It seems that people are still using their old mac with outdated safari and they might continue doing it for years to come. Is there any hard objection for not using one of the two simple solutions so at least not to crash?

davidjerleke commented 10 months ago

@7iomka thank you for joining the discussion. Thanks for the additional input @hamidrezahanafi.

I don't think that "Changing this will prevent this from crash in an old browser" is a good argument for fixing it. We can't start supporting IE11 because Embla will crash there simply because Embla doesn't support IE11 anymore.

However, since you have proof that some people (although not many based on statistics) are still using iOS13, and if it's only a matter of changing one thing to make it work:

if ('addListener' in node) {
     node.addListener(handler);
     listeners.push(() => node.removeListener(handler))
} else {
     node.addEventListener(type, handler, options)
     listeners.push(() => node.removeEventListener(type, handler, options))
}

I would be happy to add this too Embla core for now. But we need to test this on iOS13 to make sure that the above fix is enough.

Best, David

hamidrezahanafi commented 10 months ago

Hey @davidjerleke Thanks for understanding I just tested on Safari 13.1 the new code at https://github.com/davidjerleke/embla-carousel/pull/561/files Video:

https://github.com/davidjerleke/embla-carousel/assets/91487491/ed1412a4-0f77-4098-bd19-ddca99392ac6

davidjerleke commented 10 months ago

Thanks @hamidrezahanafi. I'll have to do some functionality testing before I can be sure that all features work in Safari 13 before merging the PR.

Best, David

hamidrezahanafi commented 9 months ago

Hey @davidjerleke, Is there any update on this? Let me know how I can help with testing.

davidjerleke commented 9 months ago

Hi @hamidrezahanafi,

Yes that would be helpful 👍. I'm thinking that we should mainly test the following things:

[x] Media query list listeners: Test the breakpoints option meaning setting different options based on breakpoints. For example, you can use media queries that will trigger when you trigger an orientation cchange on your mobile phone (landscape --> portrait and portrait --> landscape).
[x] ResizeObserver: Make sure that changing slide sizes triggers a emblaApi.reInit() so it's working as expected. This can be tested like this.
[x] MutationObserver: Test if adding/removing slides triggers a emblaApi.reInit() so the carousel works as expected after that. Can be achieved with a setTimeout that adds slides or similar.
[x] IntersectionObserver: Test if slidesInView is working correctly as you scroll. This can be tested like this:

emblaApi.on('slidesInView', (emblaApi) => {
  console.log(emblaApi.slidesInView())
})

Any help appreciated. Please make sure to test the latest version 8.0.0-rc12.

Best, David

hamidrezahanafi commented 9 months ago

Hey @davidjerleke I tested following on Safari 13 Media query list listeners works perfectly and applies all different options based on screen size IntersectionObserver works as the slidesInView event fired while scrolling ResizeObserver changed slide size and reInit event fired as expected MutationObserver set a timeout and added one slide and reInit event fired as expected

davidjerleke commented 9 months ago

Thanks a lot for your efforts @hamidrezahanafi 👍. I've been busy rewriting all tests for this project from scratch before the stable v8 release so any help with other stuff is appreciated. This speeds up things.

I just merged your PR and will release it with version 8.0.0-rc13 as soon as possible.

davidjerleke commented 9 months ago

Thanks for all your help @hamidrezahanafi! I just released this feature with version 8.0.0-rc13.

davidjerleke / embla-carousel