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

No swiping over nested shadow doms #6063

Closed
6 tasks done
stefanholzapfel opened this issue Sep 14, 2022 · 8 comments · Fixed by #6065
Closed
6 tasks done

No swiping over nested shadow doms #6063

stefanholzapfel opened this issue Sep 14, 2022 · 8 comments · Fixed by #6065

Comments

@stefanholzapfel
Copy link
Contributor

Check that this is really a bug

  • I confirm

Reproduction link

https://codesandbox.io/s/swiper-default-forked-0viqjr?file=/index.html

Bug description

Since upgrading to the latest version of swiper (8.4.0), swiping throws an error when started over an embedded shadow dom.

I used a simple webcomponent from shoelace to showcase the issue in codesandbox. Just try to swipe when starting over the progress ring webcomponent. Please note that the error is not thrown to the embedded console but to the browsers console:

Uncaught TypeError: Cannot read properties of undefined (reading '0')
    at V.L (onTouchStart.js:55:25)

This behavior is new, until recently I was working with embedded web components without any issues.

I guess it's a typo in 8df7edf line 55.
I added a comment there.

Expected Behavior

Don't throw an error when start swiping over nested shadowDOM

Actual Behavior

No response

Swiper version

8.4.0

Platform/Target and Browser Versions

Ubuntu / Chromium

Validations

  • Follow our Code of Conduct
  • Read the docs.
  • Check that there isn't already an issue that request the same feature to avoid creating a duplicate.
  • Make sure this is a Swiper issue and not a framework-specific issue

Would you like to open a PR for this bug?

  • I'm willing to open a PR
@realityfilter
Copy link

Same here. Thanks for reporting.

@Allan-Nava
Copy link

Same here:


TypeError: Cannot read properties of undefined (reading 'includes')
    at isChildSwiperSlide (file:///app/node_modules/swiper/react/get-children.js:4:47)
    at file:///app/node_modules/swiper/react/get-children.js:28:9
    at Array.forEach (<anonymous>)
    at getChildren (file:///app/node_modules/swiper/react/get-children.js:27:29)
    at file:///app/node_modules/swiper/react/swiper.js:47:7
    at Wc (/app/node_modules/react-dom/cjs/react-dom-server.browser.production.min.js:68:44)
    at Zc (/app/node_modules/react-dom/cjs/react-dom-server.browser.production.min.js:73:362)
    at Z (/app/node_modules/react-dom/cjs/react-dom-server.browser.production.min.js:76:89)
    at Zc (/app/node_modules/react-dom/cjs/react-dom-server.browser.production.min.js:71:479)
    at Z (/app/node_modules/react-dom/cjs/react-dom-server.browser.production.min.js:76:89)

@johnark88
Copy link

Screen Shot 2022-09-14 at 11 56 39 AM

@jay-herrera
Copy link

Same error. Had to revert to 8.3.2

@Shane-Geary
Copy link

Same error here. Upgrading from a working version of 8.3.2 broke the library

@Allan-Nava
Copy link

So the solution is downgrade to 8.2.4?

"swiper": "8.2.4",

@stefanholzapfel
Copy link
Contributor Author

stefanholzapfel commented Sep 15, 2022

@johnark88 @Allan-Nava I'm not sure we are talking about the same error (even if they might be connected).

The reported error looks like it comes from introducing a new helper variable (eventPath) but using it only in the if condition and not in the assignment.

So before the change, the variable that get's assigned (event.path[0]) was part of the if condition and therefore not assigned if undefined. But now the helper variable is checked, which might have a value while event.path is nullish.

@nolimits4web fyi

@stefanholzapfel
Copy link
Contributor Author

@nolimits4web sorry for bothering, but I think the solution provided was not ideal. In case of a composedPath, you now get an array of elements in your $targetEl.

Please check #6065 for a slightly other suggestion.

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

Successfully merging a pull request may close this issue.

7 participants