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

CSS Custom Properties on BODY lead to DOM inheritance issues #35840

Closed
markhowellsmead opened this issue Oct 21, 2021 · 48 comments · Fixed by #42084
Closed

CSS Custom Properties on BODY lead to DOM inheritance issues #35840

markhowellsmead opened this issue Oct 21, 2021 · 48 comments · Fixed by #42084
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended

Comments

@markhowellsmead
Copy link

markhowellsmead commented Oct 21, 2021

Description

Classically, the assignment of CSS Custom Properties begins at the top-level (or "root") element through the use of :root. This is common usage and allows the values to be inherited down through the DOM tree.

(It is also one of the methods by which the popular IE11 polyfill css-vars-ponyfill works. I realise that IE11 support is being dropped from WordPress Core, but solving the inheritance issue would also allow this ponyfill to work again with no additional effort.)

Gutenberg adds the generated CSS Custom Properties to the body element, and not :root, through applying global-styles using the wp_add_inline_style function. This leads to issues where third-party CSS authors connect the generated properties to their own Custom Properties on :root. The generated properties are not available at the top root level, and so the standard CSS cascade (inheritance) doesn't work.

Recommendation (and request!): apply the global CSS Custom Properties to :root, not body.

Step-by-step reproduction instructions

:root {
   /* Theme author's definition */
    --my-theme--primary--color: var(--wp--preset--color--red);
}

body {
   /* WordPress Core definition */
    --wp--preset--color--red: #f00;
}

h1 {
    /* This value is noted as undefined*/
    color: var(--my-theme--primary--color);
}

Screenshots, screen recording, code snippet

Here's a Codepen example of the code above: https://codepen.io/permanenttourist/pen/oNeYgVP?editors=1100

Environment info

  • WordPress 5.8.1
  • All modern browsers (and IE11 with ponyfill)

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@markhowellsmead
Copy link
Author

(Aside: I've looked it up, and we're talking about WP_Theme_JSON::ROOT_BLOCK_SELECTOR (wp-includes/class-wp-theme-json.php).

@stevenlinx stevenlinx added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Bug An existing feature does not function as intended CSS Styling Related to editor and front end styles, CSS-specific issues. labels Oct 25, 2021
@aristath
Copy link
Member

aristath commented Nov 1, 2021

Related: #27478

@colorful-tones
Copy link
Member

Classically, the assignment of CSS Custom Properties begins at the top-level (or "root") element

I think that "classically" is misleading here. The transformation of usage of CSS Custom Properties has matured (rightly so) in the past few years. There is no classical usage in my mind, and merely an awareness and attempt at defining and shifting common practices.

I actually think it is a great idea to target and keep body for WP core Custom Properties. It leaves flexibility for theme and plugin authors to be able to leverage :root to register their own Custom Properties, while still using body to reuse WordPress core's should they choose to. So, let's educate them on how and when to use these different targets and all about the wonderful possibilities of CSS inheritance.

@markhowellsmead
Copy link
Author

markhowellsmead commented Nov 1, 2021

I agree that “classically” wasn't the right term to use. Progressive improvement is certainly the right way to go. Educating theme and plugin authors is certainly laudable, but they need to be educated to use CSS as it was intended, and as it's defined by the standards at the W3C. WordPress needs to follow the standards, and not divert from them without an essential reason.

It's definitely important to set the site-wide values as high up the cascade as they can go, in order to provide the maximum level of support. In this case, and according to the W3C definitions, that's the document root object :root.

This is especially important when, as in the case I've shown, the current WordPress Core logic leads to errors and usability problems which individual developers - myself included - have to work around by mutating core logic in every project.

@markhowellsmead
Copy link
Author

markhowellsmead commented Nov 1, 2021

I actually think it is a great idea to target and keep body for WP core Custom Properties. It leaves flexibility for theme and plugin authors to be able to leverage :root to register their own Custom Properties, while still using body to reuse WordPress core's should they choose to.

Additional: the problem is that if developers use the :root element for their own definitions, they can't use any of the values and custom properties defined in theme.json (and, hence, on the body element). This is shown in the code example I posted. Properties defined on the body element can't be “inherited” back up to :root because of the hierarchical CSS cascade.

@colorful-tones
Copy link
Member

Additional: the problem is that if developers use the :root element for their own definitions, they can't use any of the values and custom properties defined in theme.json (and, hence, on the body element).

Correct, and in that case they would want to use the body element to target their CSS Custom Properties. I'm not seeing the problem.

WordPress needs to follow the standards, and not divert from them without an essential reason.

There are no standards, but only recommendations from W3C.

@markhowellsmead
Copy link
Author

The problem can be worked around, but it would involve every front end developer who ever works on theme and plugin development for WordPress having to recognise a special case. Where they may have to diverge from standard CSS practice to accommodate a system-specific decision.

There are no standards, but only recommendations from W3C.

I know. But deliberately going against the W3C recommendations was how we arrived at the various browser prefixing and cross-browser conflicts which have plagued the industry for so many years. This is a simple drop in the ocean, but I'm still not seeing the need to diverge from a procedure which is recommended and used by so many developers, both within and without the WordPress ecosystem.

Can you clarify which decisions were taken to arrive at the reason to use body and not :root, please? That would help to understand why it has to be implemented in the way that it is.

@markhowellsmead
Copy link
Author

If anyone's following along (or finds this thread), then the code in this Gist will parse the contents of wp_head and move the definitions from body to :root. Thereby allowing the regular CSS cascade behaviour to be used, and to restore the functionality of css-vars-ponyfill for Internet Explorer 11 et. al.

@markhowellsmead
Copy link
Author

Additional note: one result of adding the rules to :root and using the technique above to re-assign them is that these rules are then not applied to the base div in the editor. This needs to be considered when considering any modification to the current status in core.

@keithdevon
Copy link

I agree with @markhowellsmead here.

It looks like this change was made here: #31302 to allow...

allow for things like margin and padding

It would be great to know what this means and whether there is a way to use :root for custom properties and still allow margin and padding styles on body (if that was the original problem - it's unclear).

I actually think it is a great idea to target and keep body for WP core Custom Properties. It leaves flexibility for theme and plugin authors to be able to leverage :root to register their own Custom Properties, while still using body to reuse WordPress core's should they choose to.

@colorful-tones are you able to expand on this? How does using body instead of :root improve the situation for theme and plugin authors? I'm not saying that it doesn't - I just don't understand how it does!

So, let's educate them on how and when to use these different targets and all about the wonderful possibilities of CSS inheritance.

Can you start by educating me, please? 😂

@webmandesign
Copy link
Contributor

Hey guys. I was thinking that maybe the selector specificity for declaring CSS variables in global styles could be even lowered to :where(:root) as mentioned in #40042.

But if IE11 support with css-vars-ponyfill is a concern here, maybe in such edge cases one could simply change the selector with code.

@cbirdsong
Copy link

I would discourage usage of :where() for such a fundamental thing, as it’s not that well supported: #39621

@markhowellsmead
Copy link
Author

Bumping this again as pull request #42084 which addressed the issue was marked as “not a priority”. I'd like to know why this isn't a priority, and how the development team came to this conclusion.

1 similar comment
@markhowellsmead
Copy link
Author

Bumping this again as pull request #42084 which addressed the issue was marked as “not a priority”. I'd like to know why this isn't a priority, and how the development team came to this conclusion.

@cbirdsong
Copy link

Here is another issue that is caused by this: #39131

@markhowellsmead
Copy link
Author

Bumping this -- it's still a problem for theme and plugin developers.

@colorful-tones
Copy link
Member

colorful-tones commented Aug 21, 2023

I tried to get traction on several issues like this a couple of years ago but after being discussed briefly, no further action was taken and the tickets have stagnated and remain ignored, so I don’t really see any point in trying anymore.

I'm sorry. I do not intend to increase frustration and/or exhaust your patience @markhowellsmead. I want to understand the use case and challenge. I can be dense at times and ask for extra clarification 🙃

The value of --wp--preset--color--red is defined on the body, so we can’t inherit it on :root.

Yes, understood. The cascade does not ascend but descends for inheritance. 👍

I think clarifying your expectations around where these definitions are coming from might help give context to the friction in the use-case.

For example, in your original code snippet:

:root {
   /* Theme author's definition */
    --my-theme--primary--color: var(--wp--preset--color--red);
}

body {
   /* WordPress Core definition */
    --wp--preset--color--red: #f00;
}

h1 {
    /* This value is noted as undefined*/
    color: var(--my-theme--primary--color);
}

I would ask: why is the theme author assigning their variables (--my-theme--primary--color) to the :root? Why not assign them to the html, body, or lower element (maybe .wp-site-blocks)? Is it only because the theme author wants to abide by standards? I'm not challenging and just trying to understand the reasoning.

For the body declaration in your example. There is no WordPress core variable declared for --wp--preset--color--red, but there is one for --wp--preset--color--vivid-red: #cf2e2e. Perhaps this is an artifact from an older WP version, or are you intending that your WP core example is something coming from theme.json and is defined by the theme author?

@markhowellsmead
Copy link
Author

markhowellsmead commented Aug 21, 2023

I would ask: why is the theme author assigning their variables to the :root?

These are all examples and apply to custom apps built on a WordPress basis, as well as in themes and plugins. W3C recommendations are to define top-level CSS Custom Properties on the document root element :root. In a regular HTML document, this equates to the html element. Not the body.

Why not assign them to the html, body, or lower element (maybe .wp-site-blocks)?

Again: check the documentation for CSS Custom Properties. Top-level rules can be overridden at any level in the DOM, provided that the basis rule is applied as high up the DOM as possible.

For the body declaration in your example. There is no WordPress core variable declared for --wp--preset--color--red

Once again: this is just a simple example for maximal clarity.

@markhowellsmead
Copy link
Author

There are also other more subtle issues which may not be immediately evident, which are linked in other issues above.

@colorful-tones
Copy link
Member

Just trying to revisit and gather further context here...

and as it's defined by the standards at the W3C. WordPress needs to follow the standards, and not divert from them without an essential reason.

There is nothing in the referenced W3C Candidate Recommendation Snapshot that states anything to the effect: CSS variables should be assigned to the :root. There is nothing in there that even mentions the :root element. There are several examples of CSS code where they demonstrate writing variables and in doing so they assign them to the :root, but nothing states that this is a "standard".

Furthermore, the referenced document is a W3C Candidate Recommendation Snapshot, which represents the following as defined by W3C:

A W3C Candidate Recommendation Snapshot is a document produced by a W3C Working Group. A W3C Candidate Recommendation Snapshot is a W3C Technical Report.

A Candidate Recommendation Snapshot is a document that satisfies the technical requirements established in the Group charter or in subsequent requirements documents, has consensus of the Group participants, has gotten public review, and has received formal review from other W3C Groups. Such specification is intended to gather final feedback from implementers.

A Candidate Recommendation Snapshot has been reviewed by W3C Groups and interested parties.

These documents MUST NOT be cited as W3C standards and may or may not become W3C standards.

Software MAY implement these specifications and implementation feedback is encouraged.

A Candidate Recommendation Snapshot has commitments from Working Group members to royalty-free licensing for implementations.

Again, I'm not trying to exhaust the issue and merely trying to clarify for all parties. ❤️

@cbirdsong
Copy link

cbirdsong commented Aug 21, 2023

Here's MDN:

A common best practice is to define custom properties on the :root pseudo-class, so that it can be applied globally across your HTML document.

Additionally, as mentioned in a comment by @Inwerpsel on the PR that merged in this change (#31302):

Particularly, if anyone has built a JavaScript app that interacts with the global properties, they most likely use document.documentElement.style.setProperty(key, value)or an equivalent way to set these. Similarly this W3C page on how to interact with them in JS uses document.querySelector(':root'). On Google you find that just about every result for "set CSS variable in JavaScript" will tell you to use the root element.

Simply put, custom properties being output on :root makes it easier to apply knowledge and reuse code when working on Wordpress.

The reasons given for this change made in that PR and the accompanying issue do not really make sense, and no further explanation has been articulated, despite someone asking very explicitly about that in this issue and in the original issue.

This feels like a case of Wordpress going its own way for arcane internal reasons and offloading the cost of that onto theme/plugin developers.

@colorful-tones
Copy link
Member

Simply put, custom properties being output on :root makes it easier to apply knowledge and reuse code when working on WordPress.

I still have not heard or seen any compelling evidence to validate this beyond it is a "standard" (reference to a W3C Candidate Recommendation Snapshot, which never indicates it as a standard and even explicitly states the contrary) and "a common best practice." (Reference MDN)

I think some stubborn opinions are held onto regarding these third-party resources. Yes, MDN and W3C are both valid, long-standing, and sound resources, but neither should be recognized as binding laws. These resources are governed by stakeholders, just like WordPress, and standards are continuously being discussed and molded.

@colorful-tones
Copy link
Member

@jorgefilipecosta @aristath @youknowriad do either of you folks have insight into history of how body was chosen beyond the noted Issue and PR please?

Noting that a lot of folks are in transit for WordCamp US right now and response times may be delayed.

@cbirdsong
Copy link

cbirdsong commented Aug 22, 2023

To be clear, you're right that it is just a convention, and I'm not adamantly opposed to these staying on body if there's a compelling argument1, but I haven't seen one, and without that the reasonable thing to do would be sticking to the convention of using :root (or possibly html, which has less specificity than :root).

Footnotes

  1. especially one that is not just about working around the awkwardness of applying styles inside the editor

@markhowellsmead
Copy link
Author

going with the convention of using :root (or possibly html, which has less specificity than :root).

Oh, good tip! I wasn't aware of that one.

especially one that is not just about working around the awkwardness of applying styles inside the editor

This might become easier when the editor is loaded in its own iframe, wouldn't it?

@Inwerpsel
Copy link

Additionally, as mentioned in a comment by @Inwerpsel on the PR that merged in this change (#31302):

I primarily wanted to highlight the practical problems resulting from using body instead of :root in a few places.
I mentioned W3C not as an authority here, just as an example of how extremely common it is for people to choose :root out of both options.

I agree there's no completely sound argument, based on the spec, to use either. You have to realize that if you use both together randomly, you'll create some very hard to understand bugs for a substantial amount of people. If you add custom properties to the mix, answering the question "why is my style not applied?" can get quite hairy.

So it's best to make some choice and stick to it. Now if you choose body at this point, you will inevitably prevent anyone's existing code that targeted :root from working, because the core CSS now targets an element inside of it. If you choose :root, that doesn't happen, and existing code with both body and :root will keep working.

@gyurmey2
Copy link

I tried to do a simple thing – add a color to the page's scrollbar using variable colors, but due to the current CSS specificity I can't do it.

What is the reason for the variables being in the body element?

@markhowellsmead
Copy link
Author

Still a problem.

@nextgenthemes
Copy link

nextgenthemes commented Mar 14, 2024

I did not read through the reasoning, I saw something about making it less complex. Can someone give me a summary why this exits at all? They want to overwrite some theme base styles with the theme.json? Why not just enqueue a base style sheet earlier than the inline styles are put out?

It's common practice to use :root, bootstrap uses :root, and I guess almost all the CSS frameworks out there. Super annoying.

You can not target scrollbars ... I just read about that you can target SVG with root so it makes a lot of sense why it's common practice, but GB messes it all up with this decision.

@markhowellsmead
Copy link
Author

Pinging @WordPress/outreach, I managed to get a little traction on this last year but it hasn't been discussed any further since then.

@nextgenthemes
Copy link

nextgenthemes commented Mar 15, 2024

Actually, I want to correct myself, you can easily overwrite Bootstraps CSS vars. In fact, WP putting custom vars out on the body makes even more sure vars get overwritten, even if themes styles come later. And I guess that is actually the reasoning for this.

So if you set the SCSS variable $prefix: 'wp--custom--bs' in Bootstrap. You can then use this, to overwrite any variable put out, and you can also use any CSS variables from BS, you just need to pretend they are all in custom with the bs- prefix.

		"custom": {
			"bs-body-bg": "green",
		}

So, not as bad as I thought. In fact, because by default the theme style comes after the inline styles from GB, if I get this right this works specifically because the vars are put out on the body. I kind of hate to realize this because I have a bad feeling that WP is doing it different from anyone else.

My suggestion if it is hard to change now would be to introduce a new custom-root section to theme.json where you can specifically specify custom variables that are output on :root. Maybe that would be the best of both worlds, so vars for scrollbars and things you need on :root can be put there. That however would not fix the OPs issue and I guess I would prefer having to go the extra step to deal with this with multiple stylesheets in the correct order and the normal CSS cascade.

Quote from the OP:

:root {
   /* Theme author's definition */
    --my-theme--primary--color: var(--wp--preset--color--red);
}

body {
   /* WordPress Core definition */
    --wp--preset--color--red: #f00;
}

h1 {
    /* This value is noted as undefined*/
    color: var(--my-theme--primary--color);
}

The order is wrong for the typical case where you enqueue your theme's stylesheet on wp_enqueue_scripts the styles are actually put out after the GB inline styles. The result is the same, but it's this:

/* WordPress Core definitions */
body {
    --wp--preset--color--red: #f00;
}

/* Also WordPress Core definition right? */
h1 {
    /* This value is noted as undefined*/
    color: var(--my-theme--primary--color);
}

/* Theme author's definitions */
:root {
    --my-theme--primary--color: var(--wp--preset--color--red);
}

But why not do it like this:

		"custom": {
			"primary-color": "var(--wp--preset--color--red)",
		}

And then use --wp--custom-primary in your theme directly instead of --my-theme--primary--color ?

@markhowellsmead
Copy link
Author

I can see the point you're trying to make, @nextgenthemes, but the order of this specific CSS is irrelevant. The value --my-theme--primary--color on the :root cannot inherit a definition from body. I've updated the example at https://codepen.io/permanenttourist/pen/oNeYgVP?editors=1100 to show this.

But why not […] use --wp--custom-primary in your theme directly instead of --my-theme--primary--color?

That is a use case for small projects and ones with few custom definitions. The projects I build for clients commonly have dozens—if not hundreds—of CSS custom properties for all manner of rules, so it's impractical to leave behind a solution which was introduced to CSS itself, which allows this to work without needing to implement system-specific workarounds.

Some might believe that WordPress can provide a solution which replaces the need for such custom development. That is not a realistic goal. Given that on big projects, there are always requirements for complex animation, layout shifts using CSS Grid across myriad breakpoints, and dynamic script-based interaction, sticking to years-long-established best practices will stop WordPress from being a hindrance.

WordPress encourages developers to use the theme.json solution it provides instead of using commonly-used best practices which have been in place for many years. The Web Standards Project began in 1998 and pushed developers and browser makers into working towards a common goal instead of working on proprietary solutions. It remains essential that WordPress doesn't actively stop W3C Recommendations from working, which it does at the moment by insisting that the top-level element is the body element and not the html element.

@markhowellsmead
Copy link
Author

The pull request #42084 was working towards a solution for this, but was closed in 2023 because it was “old”. 🤔

@markhowellsmead
Copy link
Author

This comment #31302 (comment) also points out that people using JavaScript to interact with the custom properties will encounter problems because of the divergence from established practices and documentation.

@nextgenthemes
Copy link

nextgenthemes commented Mar 15, 2024

@markhowellsmead I said

The result is the same,

So you do not need to tell me

but the order of this specific CSS is irrelevant.

I am aware, and I understand everything about this thing now, this was the reason for my long comment explaining it. I have no clue what you are on about W3C again, @colorful-tones already told you that you are wrong.

by insisting that the top-level element is the body.

WP devs never did such thing! I am basically on your side, but this made up stuff is not helping your argument. Using a different element for CSS vars is for sure not a "proprietary solution".

Given 5K issues and how WP and GB works dev works, I would not get my hopes up into this changing any time soon, @ramonjd with the PR self closed in a kind of hilarious way.

This PR is old. If the idea is still valid we can open a new issue

Who is "we", the GB dev team? He killed his own work on it because it's "old". This looks like the end of this to me. Now everything is build on top of this bad idea and WP has an insane backwards compat policy, so they will probably just say they can't change it now because there is already so much build on top of it, if they care at all. It could an opt-in switch ...

But if it's just about "many CSS" vars, what is the actual difference besides the forced naming that can probably even changed with code if you declare them in a (S)CSS or a JSON file? Well I know one thing, it's not cached because it's inline in the HTML on every page again and again and that sucks for performance optimization. But WP is going the road of putting out all these chunks of inline stuff, it has positives and negatives ...

@fabiankaegy
Copy link
Member

@tellthemachines I'm curious whether you have any insights here with your recent work or lowering the specificity/ looking at css layers :)

@markhowellsmead
Copy link
Author

@nextgenthemes I'm sorry to read that you're so negative about the potential for improvement. I respect other people's opinions. I've referenced the W3C Recommendations throughout because of their long-standing place at the centre of platform-independent HTML and CSS development since the 1990s.

My comment about WordPress using body as the top-level element is based on the fact that all of the CSS properties created by core are placed on this element. I certainly have not "made it up”. A review and improvement of the basis code is the purpose of this Github issue. (Inlining CSS is a separate topic which is outside the scope of this specific discussion.)

@colorful-tones
Copy link
Member

@colorful-tones already told you that you are wrong.

@nextgenthemes, there is no wrong or correct, and we’re discussing this communally to reach a consensus by stating and understanding the nuances.

I appreciate @markhowellsmead opening this issue and diligently providing clarity and details to push the conversation forward. As well as the many other folks that have chimed in.

I worry that the proposed changes may have a dramatic impact, but I have little understanding of what the changes would look like. I would be happy to champion thorough solution testing and solicit community feedback if someone were to reopen or put forward a pull request. Perhaps we could target this for the next WordPress 6.6 release, as a proposed item. 🤔

@markhowellsmead
Copy link
Author

markhowellsmead commented Mar 15, 2024

If there is a change made here, then it will probably have a notable impact, which stakeholders need to assess. It's an important decision which needs to be taken by consensus, not just based on my opinion or by the individual opinions pro and contra. Perhaps @ramonjd can add his opinions to the discussion, and consider re-opening his pull request if he feels that it remains relevant (and, specifically, technically viable based on the current codebase).

@nextgenthemes
Copy link

nextgenthemes commented Mar 17, 2024

TLTR:

  • Not a WTC standard
  • Good and common practice
  • I am all for it

@markhowellsmead

I'm sorry to read that you're so negative about the potential for improvement.

I told you that I am for this change, here you go making stuff up again. You act as if I am against this improvement, never did I say that. You're calling this "proprietary" is a stretch if I ever heard one. And I made perfectly clear that I do understand this issue multiple times, there is no point for repeating yourself and explaining it to me like I do not understand what your reasons are why you opened this. My guess spamming around here won't help either.

@colorful-tones

@nextgenthemes, there is no wrong or correct, and we’re discussing this communally to reach a consensus by stating and understanding the nuances.

Based on what I read above, he claimed that putting it on :root is some kind of W3C standard. Then you corrected him that said the thing he cited was not an official W3C standard.

Quoting you quoting their stuff:

These documents MUST NOT be cited as W3C standards and may or may not become W3C standards.

So you are in fact right and he is wrong! Sorry, I am not playing this bullshit "understanding nuances" game. You know you are right, and I know it. But this forced niceness and pretending things that are not true is trendy these days. Fits perfectly, who puts his pronouns on his profile. ROFL. And after he was corrected, he brought up W3C again, completely ignoring the fact you stated. That was my issue with it.

People hate facts so much on the internet, you are not supposed to point them out, you are deemed "negative" for stating them and calling people out for being wrong. That even people who did the calling out come up with pretentious bullshit, it's hilarious. It's the absolute worst of Reddit, Hacker News also suffers from this, and now It's even in GitHub issues. It's so 2024 </rant>

Are there filters for inline CSS, because at least we should have those. With a filter, we should just be able to preg_replace this in unofficially at least??

@annezazu
Copy link
Contributor

Hey folks. Let's please center the discussion on the issue at hand. A lot of folks were tagged in this when the @outreach handle was evoked and it's our collective responsibility to be mindful of that. I'm all for strong opinions and have them myself but we're moving out of that space of productive dialogue with these last few exchanges.

As for the PR being closed out, to add some context, it's encouraged that contributors working on the project regularly review their open PRs to ensure that those who might want to contribute with code (either with reviews, testing, or code) have a higher chance of jumping into active, relevant work. This is also so that those relying on a solution can understand whether something is in progress. This is tricky to get right as noted by the 1.1K pull requests and I appreciate the proactiveness there to close it out.

@ramonjd
Copy link
Member

ramonjd commented Mar 17, 2024

I recently revisited the body/:root conundrum in an unrelated PR.

I originally wanted to flip "body" to ":root" across the codebase, but quickly discovered that CSS declarations in the body tag are there in order that theme.json styles can overwrite browser defaults such as margin. As confirmed here, I believe that was the intention behind #31302

CSS custom properties, on the other hand, may still be safely contained under :root or html.  #42084 demonstrated this to some extent, but would need a far bit of testing I think.

I'm happy to reopen and rebase that PR if folks are willing to help test it?

The pull request #42084 was working towards a solution for this, but was closed in 2023 because it was “old”.
@ramonjd with the PR self closed in a kind of hilarious way.
He killed his own work on it because it's "old". This looks like the end of this to me.

To add context to the citation and comments, #42084 was "old" in Github PR terms, since the codebase, in particular the theme JSON class, had changed a lot.

More relevant was that the PR went many months without interest from testers/reviewers. Furthermore, other priorities arose. It was closed to indicate that that specific PR was no longer "in progress", not because the topic was irrelevant or defunct. Thanks to @annezazu, who described it best above.

Perhaps @ramonjd can add his opinions to the discussion, and consider re-opening his pull request if he feels that it remains relevant (and, specifically, technically viable based on the current codebase).

I recognize and appreciate @markhowellsmead's persistence and support on this issue.

It costs nothing to reopen a PR besides a bit of rebase tomfoolery 😄 I'm still convinced it's worth a shot.

It'd be great to get it widely tested as well. I'll make sure to promote it as much as I can among other core devs.

Cheers!

@tellthemachines
Copy link
Contributor

@tellthemachines I'm curious whether you have any insights here with your recent work or lowering the specificity/ looking at css layers :)

There shouldn't be any specificity-related consequences to this change either way. :root does have higher specificity than html, but given it's the top-level element it would still be possible to override the values of properties lower down in the tree if needed.

@markhowellsmead
Copy link
Author

Thanks for taking the time to look at this @ramonjd and @tellthemachines.

I'm appreciative of the work you're doing @ramonjd and thanks to the linked issues and pull requests, I have a better understanding that the topic of applying CSS selector rules—not custom properties—to the document root in order to resolve other issues has become particularly complex. I'll be honest and say that I can't appreciate the finer details without a deep dive, which I'd be happy to schedule in if you think it'd be appropriate.

I can now understand the use case of applying the top-level rules (background, margins etc.) from theme.json to the body instead of the :root/html element thanks to #59809. This was the query which @keithdevon and I had a long while ago, when we were unsure where the connection lay between the custom properties and the body margins.

However, the specific point in this particular issue relates to the assignment of CSS custom properties to what I believe is the “correct” root element. I don't feel that the custom properties and the selector-based rules necessarily need to be applied to the same element, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.