-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
drop :open styles #128
drop :open styles #128
Conversation
✅ Deploy Preview for popover-polyfill ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thank you very much for this and I can confirm everything works as expected now in Web: LGTM If worth anything, here my thoughts: /* This older `:open` pseudo selector is deprecated and support will be removed
in a later release. */ I think that comment says it all, hopefully the lesson learned here is to drop ASAP any experimental hack for forever green browsers (such as Chrome/ium). On the other hand, a This would allow more fine-tuning experiments targeting earlier versions (as it was in this case) without breaking browsers that implemented the "not experimental anymore" behavior. /cc @tabatkins for thoughts about this (in case is not possible already) |
* main: (23 commits) chore(deps): Automated dependency upgrades chore(deps): bump actions/checkout from 3 to 4 chore(deps): Automated dependency upgrades pin back typescript chore(deps): Automated dependency upgrades pin back TS for now chore(deps): Automated dependency upgrades chore(deps): Automated dependency upgrades chore(deps): Automated dependency upgrades chore(deps): Automated dependency upgrades chore(deps): Automated dependency upgrades chore(deps): Automated dependency upgrades chore(deps): Automated dependency upgrades Fix build chore(deps): Automated dependency upgrades Bump stylelint from 15.7.0 to 15.10.1 Fix link attribute on README v0.2.2 Pin back playwright for now (tests failing on Chromium v114) changelog ...
I'd hoped to drop this earlier but the popularity of Chrome 113 with experimental web platform features was too high. Higher than Gnome Web, for example.
There's a lot of nuance here. Chrome shipped
I don't think that's particularly necessary. This is a pathological example of the failure mode that's not representative of every or even some experimental features. Polyfills come with the caveat that things may change. |
as author of many in the JS world, I fully agree and understand, although in JS I do feature detection and yet these could fail in unexpected ways too ... fair enough then, and thanks for tackling this!
AFAIK this was present also in iOS and WebView based (not stock Safari) browsers but I understand the reason it got in, and stayed long, to start with. Thanks again! |
@keithamus apologies for the late ping but ... any ETA when this will be deployed on GitHub ??? I still see the popover and it keeps being annoying 😅 thanks! |
It’ll go out likely this week. |
Description
Older versions of Chrome with experimental web platform features turned on (Chrome 113) supported the
:open
pseudo class to determine if a popover was open or not. We have support code for this in the form of CSS that checks@supports ([popover]:open)
.The problem is that now causes issues with certain versions of WebKit, which claim to support
:open
and pass the CSS check, only to unapply the popover styles but not actually supportpopover
. In other words, there are versions of WebKit - such as Gnome Web - for which this breaks.Given the popularity of Gnome Web (a fairly small % of total market share) compared with Chrome 113 with Experimental Web Platform Features enabled (an even smaller %), it makes sense to remove this CSS in order to support browsers like Gnome Web.
Steps to test/reproduce
Please explain how to best reproduce the issue and/or test the changes locally (including the pages/URLs/views/states to review).
Visit the demo page in Gnome Web. Everything is broken.
Visit this PR demo page in Gnome Web. Everything should work.
/cc @WebReflection
I've manually tested this patch in the following browsers: