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: prevent error for root shadow elements when restorEl is enabled #8679

Merged
merged 1 commit into from
May 3, 2024

Conversation

jboix
Copy link
Contributor

@jboix jboix commented Apr 9, 2024

Description

Addresses an issue where activating the restoreEl option for an element at the root of a shadow DOM threw a "TypeError: el.parentNode.hasAttribute is not a function". The bug was due to the parentNode being a shadow root, which does not have the hasAttribute method.

Specific Changes proposed

The fix implemented checks for the existence of the hasAttribute method on parentNode.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

Addresses an issue where activating the `restoreEl` option for an element at the root of a shadow DOM threw a "TypeError: el.parentNode.hasAttribute is not a function". The bug was due to the `parentNode` being a shadow root, which does not have the `hasAttribute` method.

The fix implemented checks for the existence of the `hasAttribute` method on `parentNode`.
Copy link

welcome bot commented Apr 9, 2024

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

jboix added a commit to SRGSSR/pillarbox-web-theme-editor that referenced this pull request Apr 10, 2024
Added a div container to ensure that the parent of the element passed to video.js is not
the shadow root directly. This workaround resolves the issue until a permanent fix is
implemented in video-js, see: videojs/video.js#8679
jboix added a commit to SRGSSR/pillarbox-web-theme-editor that referenced this pull request Apr 10, 2024
Added a div container to ensure that the parent of the element passed to video.js is not
the shadow root directly. This workaround resolves the issue until a permanent fix is
implemented in video-js, see: videojs/video.js#8679
github-merge-queue bot pushed a commit to SRGSSR/pillarbox-web-theme-editor that referenced this pull request Apr 11, 2024
Added a div container to ensure that the parent of the element passed to video.js is not
the shadow root directly. This workaround resolves the issue until a permanent fix is
implemented in video-js, see: videojs/video.js#8679
@mister-ben mister-ben added the needs: LGTM Needs one or more additional approvals label Apr 12, 2024
Copy link
Contributor

@amtins amtins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

To reproduce the issue:

  • open codepen
  • open developer console

@mister-ben mister-ben removed the needs: LGTM Needs one or more additional approvals label Apr 16, 2024
@mister-ben mister-ben merged commit 31b0378 into videojs:main May 3, 2024
9 of 10 checks passed
Copy link

welcome bot commented May 3, 2024

Congrats on merging your first pull request! 🎉🎉🎉

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 this pull request may close these issues.

3 participants