@wordpress/ui: Add global CSS defense module#76783
Conversation
|
Size Change: +2.31 kB (+0.03%) Total Size: 7.74 MB
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in 9fbaf30. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23952899915
|
ciampo
left a comment
There was a problem hiding this comment.
This is clever! And it makes me want to setup visual regression for all storybook examples that compares with vs without global CSS to make sure all relevant properties are "guarded".
A few more questions:
- should we also prioritize working on t the proposed solution ? The sooner it's merged, the sooner it will propagate (hopefully for 7.3 or 7.4?)
- can we meaningfully make sure that, while the core solution hasn't been applied and propagated yet, our defense module stays in sync with any changes to the global styles?
| p.p { | ||
| font-size: var(--_gcd-p-font-size, 13px); | ||
| line-height: var(--_gcd-p-line-height, 1.5); | ||
| margin: var(--_gcd-p-margin, 1em 0); |
There was a problem hiding this comment.
Should the default margin here be also 0? It feels like a healthier default to delegate spacing to parent layout components, especially for components like Text. It would also be consistent with --_gcd-heading-margin
There was a problem hiding this comment.
So would you say that the Text component should set margin: 0? It currently doesn't, so will have UA margins when rendered as p or h* (existing behavior unrelated to this defense file).
p is a bit of a tough call because I can imagine a lot of multi-paragraph Text. Like maybe there should be a default vertical margin. .text + p.text selectors for margin-top on consecutive paragraphs? Maybe even a similar setup for .text + :is(h1, h2, h3, h4, h5, h6).text. An alternative may be to nudge people towards Stack.
@WordPress/gutenberg-design Do we have any specs on how vertical margins should be in body copy, between headings and paragraphs? We don't cover them in the typography tokens doc.
There was a problem hiding this comment.
I agree it's tough, but I believe that the cleanest configuration for better composition is not to have any margin on the individual components, and to resort to components like Stack (or custom styling` for vertical spacing.
Adding margin on Text would create inconsistency with other components that don't come with any margin, and would add some friction to composing components togetther
There was a problem hiding this comment.
and would add some friction to composing components togetther
This has come up a few times, and I agree with keeping these components unopinionated about spacing. Text should focus on typography, not layout.
The trade-off is that it leaves room for inconsistent spacing between consecutive paragraphs, but that risk exists anyway; whether Text has a margin or not consumers can still compose it with Stack and choose any gap they like to fudge the spacing.
Text + Stack feels like a solid foundation. From there, I suppose it becomes more of an ergonomics question: how do we guide consumers toward consistent composition?
As a next layer, we could introduce something like a TextBlock component to encode paragraph spacing. But then there's the question of headings + paragraphs... The gap between a h1 and a paragraph might be different to the gap between a h4 and a paragraph. A heading following a paragraph might benefit from a top margin. And then there's the question of how to space consecutive TextBlocks 😅
Maybe it's something better left to linting and documentation? Or maybe it's not a huge issue... large bodies of text in UI are relatively rare...
There was a problem hiding this comment.
Thanks for the discussion here. I split out the margin removal to #76970, as this can potentially be a breaking change for consumers who may have been (probably unintentionally) relying on UA margin.
Right, sooner is always better than later. But I actually have a lower sense of urgency now that we can see the full extent of this defense solution. It isn't as awful as I expected. Let's aim to get it in WP 7.1 though.
The "safety profile" is no different than it was before with |
ciampo
left a comment
There was a problem hiding this comment.
LGTM 🚀
Can be merged once rebased to include Text margin changes and remaining feedback is addressed.
should we also prioritize working on t the proposed solution ? The sooner it's merged, the sooner it will propagate (hopefully for 7.3 or 7.4?)
Right, sooner is always better than later. But I actually have a lower sense of urgency now that we can see the full extent of this defense solution. It isn't as awful as I expected. Let's aim to get it in WP 7.1 though.
👌 Sounds good. Merging this PR early in the 7.1 lifecycle should give us the necessary time.
can we meaningfully make sure that, while the core solution hasn't been applied and propagated yet, our defense module stays in sync with any changes to the global styles?
The "safety profile" is no different than it was before with
@wordpress/components. We could potentially set up some kind of mechanism where we are alerted when the upstream styles change, but not yet sure if something like that will be worth the trouble, especially considering signal/noise ratio. It wasn't all that necessary during our several years looking after@wordpress/components.
Visual regression could definitely help here :daydreaming:
| - `Dialog.Root`: expose `disablePointerDismissal` prop ([#76847](https://github.com/WordPress/gutenberg/pull/76847)). | ||
| - `Dialog.Popup`: Default `initialFocus` now deprioritizes the close icon, focusing the first tabbable content element instead (following WAI-ARIA APG guidance) ([#76910](https://github.com/WordPress/gutenberg/pull/76910)). | ||
|
|
||
| ### Enhancements |
There was a problem hiding this comment.
Nit: move to unreleased section after rebasing
There was a problem hiding this comment.
We should probably add a section in CONTRIBUTING.md in the CSS architecture section about Global CSS defense — what it is, why it exists, how it works, when to use it.
# Conflicts: # packages/ui/src/text/text.tsx
* InputControl: Add defense styles * Textarea: Add defense styles * Link: Add defense styles * Use Link component for links * Button: Add defense styles * Fieldset: Add defense styles * Text: Add defense styles * Add links to stylesheets * Add changelog * Move import ordering * Fix anchor transition * Use `0 solid transparent` for outline fallbacks * Improve clarity of textarea defenses * Set paragraph margin fallback to 0 * Fix changelog * Add documentation to CONTRIBUTING.md Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: simison <simison@git.wordpress.org>
What?
Part of #76135
Closes #76570
Adds a shared, unlayered CSS module (
global-css-defense.module.css) that shields@wordpress/uicomponents from wp-admin's global stylesheets (common.css,forms.css), and wires it into all currently affected components.Why?
@wordpress/uiplaces all component styles in CSS cascade layers. Per the spec, unlayered styles always win over layered styles regardless of specificity. Since wp-admin's global stylesheets are unlayered, they unconditionally override component styles — breaking borders, outlines, colors, focus rings, typography, and more.This is a blocker for using
@wordpress/uiin Gutenberg. While the problem itself is better addressed upstream, waiting for these changes to land in a WP release is going to be too slow for plugin developers to start adoption of@wordpress/ui. These plugins usually want to support at least two latest versions of WP, so that means they won't be able to use@wordpress/uifor another 6–12 months.The solution proposed in this PR is pretty much the only solution I could come up with that unblocks us immediately, is reasonably manageable, and is easy to remove once upstream improvements make the workarounds unnecessary.
How?
The defense module provides per-element classes (
.input,.textarea,.button,.div,.p,.heading,.a) that are applied directly to the elements that need protection. Each class is unlayered, so it competes with admin CSS on equal footing and wins by specificity.Properties are declared via a custom property bridge pattern:
@wordpress/ui, so most components need zero overrides.Testing Instructions
npm run storybook:dev) and enable the WordPress global CSS mode via the toolbar.Use of AI Tools
Cursor with Claude was used to author this PR.