oddbird / popover-polyfill

Polyfills the HTML popover attribute and showPopover/hidePopover/togglePopover methods onto HTMLElement, as well as the popovertarget and popovertargetaction attributes on <button> elements.
BSD 3-Clause "New" or "Revised" License
247 stars 14 forks source link

Current implementation incompatible with css layers #175

Closed Prestaul closed 7 months ago

Prestaul commented 7 months ago

User-agent (and polyfilled) styles should always have lowest priority, but if a consumer uses cascade layers (i.e. @layer) then polyfilled popover styles have highest priority and overwrite other styles.

In any browser, the user-agent styles are effectively in the first layer and therefore any non-agent styles win, regardless of specificify. When styles include layers, styles not in a layer always win. The polyfilled styles are not in a layer and this means that, even with :where() and a specificity of 0-0-0-0, they always overwrite any styles on sites using cascade layers.

I'm not sure what the intended browser support matrix is for this polyfill, but all current browsers have full @layer support so it is possible to just drop polyfilled styles into a layer and this may be resolved.

Another option would be to make it configurable, either with a boolean flag or possibly even a nullable layer name. The obvious drawback here is that you'd have to make a function call (like the anchor positioning polyfill) to initialize the polyfill if it has configuration.

mirisuzanne commented 7 months ago

I chatted with @jgerigmeyer about this, and see a few options:

It might make sense to provide both in this case? I imagine, for broader support, we should keep the unlayered option as the default - but make the layered option as easy an upgrade as possible.

For the details of that, I would expect we wrap the entire stylesheet with something like @layer popover-polyfill.default { /* styles… */ }. The double-nesting allows authors to both place popover-polyfill where they want it in a layer order, and also add styles to that layer above or below the defaults. Once you start layering, you may as well go all-in. :)

Curious if @keithamus has thoughts on this as well.

Prestaul commented 7 months ago

@shannonmoeller mentioned the possibility of having a popover-layer.min.js as a separate build as well. I'm not sure if there is a convention here the Odd Birds prefer.

Another possibility would be an optional param added to apply and consumers using cascade layers would have to use the more verbose if (!isSupported()) apply(true); pattern of use.

keithamus commented 7 months ago

I personally prefer zero configuration wherever possible. Given we inject the styles in JS, we could feature detect for @layer and do it automatically, but it may cause surprising style issues as browsers which dont support @layer would suddenly have CSS that participates in the regular cascade in ways that perhaps we are unable to anticipate or correct for?

Having said that I don’t have enough experience in using @layer to know if we’d be causing gotchas with this technique.

So if we can’t reasonably do it automatically my second preference would be for a separate file, where users can opt in with a nice clear ergonomic import like import '@oddbird/popover-polyfill/with-layer.

Prestaul commented 7 months ago

I hadn't thought about feature detecting for @layer support, but that seems ideal. Having used layers quite a bit now, I think there is little potential for harm in browsers that have layer support. The biggest gotchya is that, if consumers care to control layer order, they will need to ensure they set the order before initializing the polyfill. The good thing here is that, 1) that isn't hard, and 2) including the polyfill first will also default to using it as the first layer which is closest to the behavior of user-agent styles.

The current implementation should work well for browsers that do not support cascade layers because of changes in #153.

mirisuzanne commented 7 months ago

I agree, detecting support should be fine here. Let's go with that.