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

drop :open styles #128

Merged
merged 3 commits into from
Sep 18, 2023
Merged

drop :open styles #128

merged 3 commits into from
Sep 18, 2023

Conversation

keithamus
Copy link
Collaborator

@keithamus keithamus commented Sep 18, 2023

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 support popover. 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:

  • Modern WebKit (Safari 14.1)
  • Latest Edge
  • Latest Chrome
  • Latest Firefox
  • Chrome Dev
  • Gnome Web
  • QuteBrowser v3.0.0
  • Palemoon (works but has some CSS issues unrelated to this PR)
  • Chrome 113 (w/ Experimental Web Platform Features) - it doesn't work here - this is the trade-off.
  • Chrome 113 (w/o Experimental Web Platform Features)
  • Chrome 114 (w/ Experimental Web Platform Features)
  • Chrome 114 (w/ Experimental Web Platform Features)
  • Latest Safari iOS

@netlify
Copy link

netlify bot commented Sep 18, 2023

Deploy Preview for popover-polyfill ready!

Name Link
🔨 Latest commit d0189d8
🔍 Latest deploy log https://app.netlify.com/sites/popover-polyfill/deploys/65086712eb846900088993dd
😎 Deploy Preview https://deploy-preview-128--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.

@WebReflection
Copy link

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 @support([any-attribute]:pseudo) seems to be a way too generic check to me, and I wonder if there's any way that @support could be improved to know if such support is experimental ... is this maybe something to discuss with the CSS working group? an :experimental selector that exists only if a feature is, effectively, experimental?

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
  ...
@jgerigmeyer jgerigmeyer merged commit edae6c2 into main Sep 18, 2023
@jgerigmeyer jgerigmeyer deleted the drop-open-styles branch September 18, 2023 15:09
@keithamus
Copy link
Collaborator Author

hopefully the lesson learned here is to drop ASAP any experimental hack for forever green browsers

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.

way too generic check to me

There's a lot of nuance here. Chrome shipped :open but it was deemed to be inappropriate as it will be used for other things, such as <dialog>, that may need to differentiate between a popover that is open vs its own open state. We could have added a JavaScript check, but at the moment :open does not exist in any capacity so it was deemed okay to ship this CSS. Sadly it seems WebKit passes this check, which I consider to be a bug.

an :experimental selector that exists only if a feature is, effectively, experimental?

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.

@WebReflection
Copy link

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!

Chrome 113 with experimental web platform features was too high. Higher than Gnome Web, for example.

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!

@WebReflection
Copy link

@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!

@keithamus
Copy link
Collaborator Author

It’ll go out likely this week.

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