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

[BUG] Broadening of elements with modified CSS in 0.2.0 #241

Closed
jonathanex opened this issue Sep 11, 2024 · 3 comments · Fixed by #256
Closed

[BUG] Broadening of elements with modified CSS in 0.2.0 #241

jonathanex opened this issue Sep 11, 2024 · 3 comments · Fixed by #256
Labels
bug Something isn't working

Comments

@jonathanex
Copy link

So I should be clear up front - I don't know if this is strictly a bug, or if it's behaviour I've not figured out. There's a change in behaviour in 0.2.0 which I'm assuming is for position-try support, but I'm also unclear of if it needs to be applied as broadly as it is.

Basically - with the change from 0.1.1 to 0.2.0, a lot more elements are having their styles modified than before.

To Reproduce
Simple way to compare, these two codepens:
0.1.1: https://codepen.io/jonathanex/pen/rNERrQe
0.2.0: https://codepen.io/jonathanex/pen/ExBMdeB

Version 0.1.1, there's no modification to the styles on the '.wrapper' element. But on v0.2.0, a custom property has been added for the top on the '.wrapper' div.

What's not immediately clear to me is why that element needs it - it's neither the anchor or the target, it's just that one sits in it further up the chain? Should the wrapper element be affected?

Additional context
So in theory nothing wrong with an additional CSS custom prop, doesn't affect the rendering, it just sits there.

But I've been working with the anchor positioning polyfill on a codebase which has a quite large CSS file. This has the side effect of meaning that when it runs on some browsers, and the CSS file is removed and the blob with the modified CSS is added, we're seeing a flash of unstyled HTML in between and some little quirks with paths.

So to combat that, our approach was to keep everything we knew may be affected by the polyfill separate - so its own file, style tag, inline, whatever - just to minimize what the polyfill needed to touch, the browser has a lot less to refresh.

That was working with 0.1.1 - but with this change, if an anchored element sits within something that's got any inset set on it any way up the tree, that also then has its styles modified, so caused the main (large) CSS file to be replaced and the unstyled-flash kicks in.

This may be very much "my problem" to fix, but I thought it was worth seeing if the style replacement is being overzealous or if it needs to impact those parent elements as it does now.

Thank you!

@jonathanex jonathanex added the bug Something isn't working label Sep 11, 2024
@jamesnw
Copy link
Contributor

jamesnw commented Sep 11, 2024

Your observations on the change in 0.2.0 are correct. Essentially, we need to move a number of properties to custom properties in order to get the cascaded raw values. The full list is here.

We can't know from a StyleSheet which properties will apply to anchored elements. For instance top might be set in .wrapper and anchoring rules set in #target, and both would apply to <div class="wrapper" id="target">.

As far as an eventual solution, this could benefit from one solution discussed in #228, where authors would have some control over which of their stylesheets get polyfilled.

@jonathanex
Copy link
Author

Ah - that makes more sense. I was looking at it from the DOM point of view first but of course you'd be manipulating based on stylesheet first, absolutely makes sense.

I like the idea proposed in #228 you mention - I think that could work for this sort of use cases.

For now on our project we'll stick to 0.1.1 and hold off for a bit, as it works great from testing for what we need, but I'll keep an eye on things.

(I don't know if you want to consider this closed as it's not strictly a bug - or keep it open to track other impacts of this - entirely comfortable either way!)

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

@jonathanex The opt-in proposal from #228 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
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants