inject stylesheets automatically in the JS #137
Conversation
✅ Deploy Preview for popover-polyfill ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
* main: update changelog Upgrade to node v18
There was a problem hiding this comment.
Works for me! @mirisuzanne Would you be willing to review this too?
@keithamus It's not new in this PR, but I notice that the "Click to toggle shadowed Popover" button crashes the tab for me when clicked in FF 118.
There was a problem hiding this comment.
I like this approach! The polyfill is going to to require JS either way, so no point shipping the CSS without it.
Realizing the comments I left aren't really specific to the PR - so feel free to merge. I'm still curious about your thinking on the styles, but that shouldn't hold up the actual change here.
| @@ -35,6 +35,86 @@ function patchSelectorFn<K extends string>( | |||
|
|
|||
| const nonEscapedPopoverSelector = /(^|[^\\]):popover-open\b/g; | |||
|
|
|||
| const styles = ` | |||
There was a problem hiding this comment.
I'm curious where these styles come from (besides z-index which is filling in for top-layer)? Are they the proposed browser defaults? It looks like this is attempting to reset styles that may be applied on the popover element itself, and return them to spec initial values? I'm a bit skeptical about overstepping the role of a polyfill, into supplying our own recommended styles – if there's any chance these confuse the transition onto the browser-native feature?
There was a problem hiding this comment.
Yes these are all the UA defaults (aside from - as you rightly said - z-index). Here's chromes UA stylesheet, Firefox's UA sheet, and Webkit's is here.
These have some small differences, mostly vestigial from the initial implementation, but some are to account for some subtle differences. I can follow up with some PRs to better align these to the UA stylesheets but I'd like to test those changes independently.
There was a problem hiding this comment.
Yeah, looks good and sounds great :)
| display: revert; | ||
| } | ||
|
|
||
| [anchor].\\:popover-open { |
There was a problem hiding this comment.
I would probably keep things simpler with .popover-open rather than trying to duplicate pseudo-class syntax inside the class syntax. Maybe that ship has already sailed, though.
There was a problem hiding this comment.
Originally this pseudo class was :open, and so I opted for .:open to provide a classname that would be different enough that it wouldn't conflict with existing styles. The pseudo class has since shifted to :popover-open and so we followed up with a name change, but even so .popover-open seems to be in common enough usage that it felt best to stick with the : prefix.
That's interesting. Is that on stock Firefox, or have you enabled popovers or some shadowdom features? I can't reproduce locally, but it'd be interesting to investigate. Do you have a crash report? |
This is stock Firefox 118.0.2 with popovers disabled. It works fine for me in Safari 16.6 (using the polyfill). If I test in Chrome (popovers enabled) or Firefox (popovers enabled), the button just doesn't do anything. Crash report: https://crash-stats.mozilla.org/report/index/7ac206de-9229-402a-9e6f-31ee80231019 |
Description
As popover support is gradually being added to various browsers we find ourselves sometimes hitting compatibility issues where a browser supports the CSS syntax, but not the API, or the other way around, or it supports some outdated syntax like we saw in #128.
Most recently we've realised that Firefox on iOS 17 supports
@supports selector(:popover-open)but doesn't support any of the APIs, and as such gets polyfilled.The net result is that there is no way to detect in CSS alone whether or not popover is supported. So we need to look at other options.
In this PR I've adjusted the CSS we ship to apply some safe defaults (although we could look at emptying it or otherwise removing it in a subsequent version), and instead of a CSS file, the JS directly injects the CSS into the page.
The benefit of injecting the CSS is that we can do it at a time when we know the polyfill needs to be applied. This helps us in several ways:
Steps to test/reproduce
You can manually test the polyfill in a variety of browsers to get a sense of this change. I tested in the following:
Show me