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

Assign adoptedStyleSheets rather than push #141

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

keithamus
Copy link
Collaborator

@keithamus keithamus commented Oct 26, 2023

Description

Some older browsers such as Chrome 98 have adoptedStyleSheets but it is a frozen array, rather than an ObservableArray, and can only be assigned to rather than push()ed to. Pushing to a frozen array causes an error to be thrown and so the polyfill doesn't work in those browsers if we continue to use push.

The fix for this is to always use an assignment on the array, it's a little slower the impact will be negligible.

See https://chromestatus.com/feature/5638996492288000 for some more detail.

Steps to test/reproduce

  • Visit https://popover-polyfill.netlify.app/ in Chrome <99 (I tested Chrome 96 for example).
  • Note that the styles haven't loaded and popovers are all visible.
  • Visit the console and note the error TypeError: Cannot add property 0, object is not extensible.

Show me

a screenshot of Chrome 96 showing the error condition described above. There are two windows open, one demonstrating the Chrome version (96) and the other with the popover demo page open; the devtools are open showing the error and the demo page is unstyled, with all popovers showing.

Some older browsers such as Chrome 99 have `adoptedStyleSheets` but it
is a frozen array, rather than an `ObservableArray`, and can only be
assigned to rather than `push()`ed to. Pushing to a frozen array causes
an error to be thrown and so the polyfill doesn't work in those browsers
if we continue to use push.

The fix for this is to always use an assignment on the array, it's a
little slower the impact will be negligible.

See https://chromestatus.com/feature/5638996492288000 for some more
detail.
@netlify
Copy link

netlify bot commented Oct 26, 2023

Deploy Preview for popover-polyfill ready!

Name Link
🔨 Latest commit 08e1a6b
🔍 Latest deploy log https://app.netlify.com/sites/popover-polyfill/deploys/653a36530b0d6a0008e0547f
😎 Deploy Preview https://deploy-preview-141--popover-polyfill.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@keithamus
Copy link
Collaborator Author

@jgerigmeyer just a friendly ping, would be great to get this one merged!

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

Looks good! @keithamus I'm traveling and won't be able to cut a new release until tomorrow.

@jgerigmeyer jgerigmeyer merged commit 69b9ed5 into main Oct 27, 2023
6 checks passed
@jgerigmeyer jgerigmeyer deleted the assign-adoptedstylesheets-rather-than-push branch October 27, 2023 17:22
@keithamus
Copy link
Collaborator Author

Thanks for the review and merge @jgerigmeyer. Happy travels!

@jgerigmeyer
Copy link
Member

@keithamus Published in v0.3.1 🚀

@keithamus
Copy link
Collaborator Author

Thanks so much!

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

Successfully merging this pull request may close these issues.

2 participants