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

The polyfill won’t work with CSSStyleSheet() or CSSStyleDeclaration() #228

Closed
marchbox opened this issue Aug 28, 2024 · 6 comments · Fixed by #256
Closed

The polyfill won’t work with CSSStyleSheet() or CSSStyleDeclaration() #228

marchbox opened this issue Aug 28, 2024 · 6 comments · Fixed by #256
Labels
enhancement New feature or request

Comments

@marchbox
Copy link
Contributor

marchbox commented Aug 28, 2024

Is your feature request related to how popovers work in general?

No.

Is your feature request related to how this polyfill works? Please describe.

When using *.adoptedStyleSheets or *.style.setProperty() to add CSS Anchor Positioning declarations, in unsupported browsers, these declarations will be ignored/removed since they are not recognized CSS properties and values. As a result, this polyfill won't work because those declarations aren't there in the first place.

The current work around is to use *.setAttribute('style', '...') or creating a style element with the Anchor Positioning declarations as its text content. But this work around could trigger content security policy issues.

Describe the solution you'd like

Unclear.

Additional context

We are currently building some custom elements (using Popover API) that would benefit from CSS Anchor Positioning, so we are investigating if using the polyfill would help in unsupported browsers. We understand the cross-root referencing issue, so in the custom element, we creates a new CSSStyleSheet() object and inject that into document.adoptedStyleSheets, e.g.

#trigger-123 {
  anchor-name: --trigger-123;
}
#popover-123 {
  left: anchor(--trigger-123, left);
  top: anchor(--trigger-123, bottom);
}

But in unsupported browsers, this style sheet would just be:

#trigger-123 {}
#popover-123 {}

We are wondering if there is a solution for this use case, or rather this is out of the scope of this polyfill.

@marchbox marchbox added the enhancement New feature or request label Aug 28, 2024
@jgerigmeyer
Copy link
Member

Hm, good question. It's definitely related to this polyfill, but somewhat tangential to the work of the polyfill itself -- more generally "How can I define styles for custom elements in browsers that don't understand them as styles?".

I'm not thinking of anything other than <link> or the workarounds you outlined (createElement('style') with textContent). @jamesnw @mirisuzanne @mmalerba Any ideas?

@mmalerba
Copy link
Contributor

I think if we really wanted to be fancy we could try to monkey-patch the relevant APIs to intercept these styles, for example:

const realReplaceSync = CSSStyleSheet.prototype.replaceSync;
CSSStyleSheet.prototype.replaceSync = function(css) => {
    addToPolyfill(css);
    return realReplaceSync.apply(this, arguments);
}

If that's too crazy, the design direction I'm exploring involves moving all of the unsupported properties and properties with unsupported values into custom properties as part of a preprocessing step. Maybe we could expose that preprocessing function to allow users to preprocess their own CSS, and update the polyfill to look for already pre-processed styles

@mirisuzanne
Copy link
Member

I think the pre-processing approach makes a lot of sense as a workaround here. But I would defer to others if the ✨fancy approach is more usable.

@marchbox
Copy link
Contributor Author

marchbox commented Aug 29, 2024

Thank you for the discussion so far! This is a tricky one.

The pre-processing approach would force all browsers to use the polyfill, it's still nicer than directly using Floating UI because you can just remove the pre-processor when the future arrives. But it would be nicer if we can still leverage the built-in support whenever we can.

The API interception approach may cause performance issue, because all calls to those built-in APIs will have to go through the process. I wonder, instead of polyfilling the built-in APIs, maybe provide a ponyfill function, something like:

function replaceSyncWithAnchorPositioning(styleSheet, css) {
  if (CSS.supports('anchor-name: --a')) {
    return;
  }

  addToPolyfill(css);
  styleSheet.replaceSync(css);
}

This way at least authors can opt in.

Either way, it would be really nice to have that addToPolyfill() function exported, so authors can use it more flexibly. For example, in our use case, our library is a group of custom elements for a design system, and we do not want to bundle the polyfill in so that we can keep the bundle size down, also users of our library may want to use their own polyfill solutions, so rather than out-of-the-box polyfill support, we'd recommend polyfill solutions in our documentation. The ponyfill approach will not work for this use case, but having the addToPolyfill() function could help make it easier.

Edit: sorry, I meant to say "we do NOT want to bundle the polyfill in" 🥴

@marchbox
Copy link
Contributor Author

marchbox commented Oct 8, 2024

I just created a draft PR for proof-of-concept purpose: #256

The change is quite small (hopefully the implementation is correct). And I can make it more robust if needed, e.g. supporting <link> element, supporting inline styles, etc. But I'd like to get your thoughts on the approach before I go deeper. Also, if you already have a plan for addressing the issue, let me know, happy to wait for an official solution. Thanks!

@jgerigmeyer jgerigmeyer linked a pull request Oct 11, 2024 that will close this issue
@jgerigmeyer
Copy link
Member

This is released in v0.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants