Block custom CSS: Fix incorrect CSS when multiple root selectors #53602
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
added
Global Styles
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/class-wp-theme-json-gutenberg.php ❔ phpunit/class-wp-theme-json-test.php |
|
Size Change: +73 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
t-hamano
commented
Aug 12, 2023
Comment on lines
+1148
to
+1150
| if ( count( $part ) !== 2 ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This is to exclude strings with invalid braces, such as the following, and to ensure that they are correctly split into selector and value.
& ::marker{{ color: red;}
There was a problem hiding this comment.
Thanks for fixing this @t-hamano 👍
It tests well for me although I think there are a couple of tweaks we can make to the code.
There are existing util functions to help scope selectors on both the PHP and JS side. Using those will make the code easier to read and maintain etc. What do you think?
| Before | After |
|---|---|
The custom CSS for the block uses __experimentSelector in block.json as the root selector.
It might be worth clarifying that the __experimentalSelector is only one (deprecated) source for the block's selector. The root selector for a block comes from the Selectors API. The Selectors API does provide support for, and falls back to, the __experimentalSelector block.json property though for backwards compatibility purposes.
packages/block-editor/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
|
Thanks for the review, @aaronrobertshaw!
Oops, I didn't know that such a utility exists for both PHP and JS 😅 However, I found that the nested selector has two meanings:
My understanding is correct, right? So I used Based on these, I updated the PR as follows:
|
|
Flaky tests detected in e2d8f78. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6321567362
|
aaronrobertshaw
approved these changes
Sep 28, 2023
There was a problem hiding this comment.
This is looking in pretty good shape @t-hamano 👍
It's working well for me although there are a few typos that we can tweak before landing. Bonus points for the updated tests, thank you!
✅ Blocks with selector lists as their primary selector now function correctly with custom CSS
✅ Works with simple selectors in custom CSS
✅ Works with nested sub-selectors
✅ Works with nested selectors that are appended to root selectors
✅ Unit tests pass
It might also be worth getting this fix into the 6.4 beta, what do you think?
packages/block-editor/src/components/global-styles/test/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/test/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/test/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/test/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
…obal-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
…obal-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
…obal-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
…obal-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
|
Thank you for the test and typo correction, @aaronrobertshaw!
I think so too. Should I just submit a core ticket and attach a patch with similar changes to |
aaronrobertshaw
added
the
Backport to WP 6.7 Beta/RC
Yep, sounds like a plan. I've added the backport to WP Beta label to this PR for the JS side of things. |
|
I have submitted a core patch and would like to merge this PR 🚀 |
mikachan
pushed a commit
that referenced
this pull request
Sep 29, 2023
) * Block custom CSS: Fix incorrect CSS when multiple root selectors * Fix PHP lint error * Use `scope_selector` and `append_to_selector` method and update unit test * Use `scopeSelector` and `appendToSelector` function and update JS unit test * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/utils.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * re-trigger CI --------- Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
|
I just cherry-picked this PR to the commits-for-6.4-beta2 branch to get it included in the next release: c35b6d0 |
mikachan
removed
the
Backport to WP 6.7 Beta/RC
86 tasks
fluiddot
pushed a commit
that referenced
this pull request
Oct 2, 2023
* Fix the install of system fonts from a font collection (#54713) * Fix set upload dir test (#54762) * Site Editor: Prevent unintended actions on the classic theme (#54422) * Add action and selector to track access to Patterns page * Add a URL parameter to check if the Patterns page was accessed directly * Revert lib changes * Fix critical error in the Post Editor * Explicitly add `! isBlockBasedTheme` with `isTemplatePartsMode` * Fix critical error in the Post Editor * Patterns: Fix back navigation after pattern creation (#54852) * Patterns: Fix category control width in site editor (#54853) * Patterns: Allow non-user patterns under Standard filter (#54756) * Performance Tests: Separate page setup from test (#53808) # Conflicts: # test/performance/specs/post-editor.spec.js * Performance Tests: Support legacy selector for expanded elements (#54690) * Paragraph: Make 'aria-label' consistent with other blocks (#54687) * Paragraph: Make 'aria-label' consistent with other blocks * Update tests * Try using BC labels in performance tests * Move lightbox render function to filter (#54670) * syntax refactor repace strpos with str_contains (#54832) * Font Library: avoid deprected error in test (#54802) * fix deprecated call * removing unwanted line * Fix the ShortcutProvider usage (#54851) Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Image: Ensure `false` values are preserved in memory when defined in `theme.json` (#54639) * Modify conditional when parsing config * Only drop the value if it's undefined. --------- Co-authored-by: Michal Czaplinski <mmczaplinski@gmail.com> * Use "Not synced" in place of "Standard" nomenclature for patterns (#54839) * Standard -> Not synced * Fix broken test --------- Co-authored-by: Glen Davies <glen.davies@automattic.com> * Format Library: Try to fix highlight popover jumping (#54736) * Move mime-type collection generation to a function that can be tested… (#54844) * Move mime-type collection generation to a function that can be tested. Refactored to use that function. * linting changes * Add unit tests to mime type getter * Fixed linting errors * test the entire output array and replace assertTrue by assertEquals * fixing docs --------- Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> * Ensure lightbox toggle is shown if block-level setting exists (#54878) * Block Editor: Update strings in blocks 'RenameModal' component (#54887) * Footnotes: Add aria-label to return links (#54843) * Add aria-label to footnotes front-end links. * Add visual output. Fix PHPCS errors. * Remove visual changes, fix in follow-up. * Font Library: Changed the OTF mime type expected value to be what PHP returns (#54886) * Changed the OTF mime type expected value to be what PHP returns * add unit test for otf file installation --------- Co-authored-by: madhusudhand <madhusudhan.dollu@gmail.com> * Image: Fix layout shift when lightbox is opened and closed (#53026) * Replace overflow: hidden with JavaScript callback to prevent scrolling * Disable scroll callback on mobile; add comments; fix scrim styles The page jumps around when trying to override the scroll behavior on mobile, so I disabled it altogether. I've also added comments to clarify this and other decisions made around the scroll handling. While testing, I realized that the scrim was completely opaque during the zoom animation, which does not match the design, so I added a new animation specifically for the scrim to fix that. * Add handling for horizontally oriented pages * Move close button so that it's further from scrollbar on desktop * Fix call to handleScroll() and add touch callback to new render method * Improve lightbox experience on mobile To ensure pinch to zoom works as expected when the lightbox is open on mobile, I added logic to disable the scroll override when touch is detected. Without this, the scroll override kicks in whenever one tries to pinch to zoom, and it breaks the experience. I also revised the styles for the scrim to make it opaque, as having content visible outside of the lightbox is distracting when pinching to zoom on a mobile device, and I think having a consistent lightbox across devices will make for the best user experience. * Fix spacing * Add touch directives to block supports * Fix spacing in block supports * Rename attribute for clarity * Update comment * Update comments * Fix spacing --------- Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> * Font Library: move font uploads to a new tab (#54655) * move font uploads to a new tab in the modal * fix invalid success message and revert the dropzone to modal * add success notice for font uploads * make supported file formats message dynamic based on allowed extensions * update hint text and clear successful message with a fresh upload * Block custom CSS: Fix incorrect CSS when multiple root selectors (#53602) * Block custom CSS: Fix incorrect CSS when multiple root selectors * Fix PHP lint error * Use `scope_selector` and `append_to_selector` method and update unit test * Use `scopeSelector` and `appendToSelector` function and update JS unit test * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/utils.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * re-trigger CI --------- Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Add new e2e test for creating a pattern (#54855) * Use list role instead of listbox for patterns (#54884) * Popover: Fix the styles for components that use emotion within popovers (#54912) # Conflicts: # packages/components/CHANGELOG.md * Footnotes: use core’s meta revisioning if available (#52988) # Conflicts: # packages/block-library/src/footnotes/index.php * Remove base url from link control search results (#54553) * Expose baseURL setting on Post and Site Editors via block settings * Strip baseURL from rendered search results * Only fetch baseURL once in top level component * Simplify implementation to utilise URL parse functions * Improve comment wording to avoid referencing undefined var * Remove superfluous conditional * Decode URL prior to operations * Refactor for readability * Fix where url is not defined * Revert change to filter util * Ensure that filterURLForDisplay always receives a string as an arg * Make e2e test locator less strict * Prefer pipe * Force remove trailing slash * [Site Editor]: Update copy of using the default template in a page (#54728) * Remove overflow: hidden from the entity title h1 in the site editor sidebar (#54769) * Site Editor: Avoid same key warnings in template parts area listings (#54863) * TemplateAreas use template part clientId as key * HomeTemplateDetails use template part clientId as key * Cleanup useSelect hook * Fix ToolSelector popover variant (#54840) * Font Library: refactor endpoint permissions (#54829) * break the checking of user permission and file write permissions * return error 500 if the request to the install fonts endpoint needs write permissions and wordpress doens't have write permission on the server * do not ask file write permission on uninstall endpoint * Standardize the output of install and uninstall fonts endpoints Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> * Handle standardized output from install and uninstall endpoints in the frontend Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> * Update the install and unintall fonts endpoints unit tests for the new standardized output format Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> * fix the refersh call for the library Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> --------- Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> * Don’t use TypeScript files in scripts package (#54856) * Fix Search Block not updating in Nav block (#54823) * Avoid setState in render * Attempt at test coverage * Improve tests and make them work * Remove word-wrap property (#54866) --------- Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Bart Kalisz <bartlomiej.kalisz@gmail.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Artemio Morales <artemio.morales@a8c.com> Co-authored-by: Riad Benguella <benguella@gmail.com> Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> Co-authored-by: Michal Czaplinski <mmczaplinski@gmail.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Glen Davies <glen.davies@automattic.com> Co-authored-by: Jason Crist <jcrist@pbking.com> Co-authored-by: Alex Stine <alex.stine@yourtechadvisors.com> Co-authored-by: madhusudhand <madhusudhan.dollu@gmail.com> Co-authored-by: Carlos Bravo <37012961+c4rl0sbr4v0@users.noreply.github.com> Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> Co-authored-by: Kai Hao <kevin830726@gmail.com> Co-authored-by: Adam Silverstein <adamjs@google.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> Co-authored-by: Pascal Birchler <pascalb@google.com>
|
Using this in theme.json, for context: |
|
Looking at your code, it looks like you want to target a Llist block that has a style variation named "core/list": {
"css": "&.is-style-no-style {list-style-type: none;padding-left: 0;}",
},Additionally, in 6.4 Beta2, this issue is not yet fixed on the frontend side. The patch attached to this ticket must be committed to core. |
Yes, the GB change was included in Beta 2 with this group of backports: #54914.
I'll see if I can help get some eyes on the trac ticket, it possibly needs to be brought up in a Bug Scrub. |
|
✅ I updated this PR with the |
getdave
added
Backported to WP Core
|
This PR contains the actual fix and unit test updates. The fix itself has been backported, but the unit test chages have not yet been backported to the core. Even in the tracking issue, one of the two has not been checked yet. Therefore, I would like to Remove the "Backported" label for now. Once the unit test changes are committed to the core, I would like to add the "Backported" label again. |
t-hamano
added
Needs PHP backport
youknowriad
added
Backported to WP Core
|
Looks like @youknowriad has completed the backport of the unit tests so we're all good now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #53597
What?
🤖 Generated by Copilot at 65d02b2
This pull request refactors the PHP and JS functions that process nested CSS rules for global styles, and adds a unit test for the PHP function. The refactor fixes a bug that caused invalid CSS output when multiple root selectors were present, and improves the code readability and consistency.
Why?
The custom CSS for the block usesThe custom CSS for the block uses the selector obtained via the Selectors API as the root selector. For the List block, for example, there will be multiple selectors separated by commas, such as__experimentSelectorin block.json as the root selector.ul,ol.However, when nested CSS is defined in the global styles or
cssproperties intheme.json, the current implementation does not take multiple selectors into account and treats them as a single selector, causing the issue reported in #53597.How?
When generating nested CSS, I first convert the root selectors into an array by splitting them with commas. Then, the nested selector is added to each root selector, which is an element of the array, and converted back into a comma-separated string.
Testing Instructions
preparation
Via theme.json
{ "version": 2, "settings": { "layout": { "contentSize": "840px", "wideSize": "1100px" } }, "styles": { "blocks": { "core/list": { "css": "background: #ccc; & ::marker{ color: red;}" } } } }The gray background color and red marker style should be correctly applied to both the Ordered and Unordered lists.
Via the Site Editor
background: #ccc; & ::marker{ color: red;}