fix inconsistencies in global styles feature selectors#11857
fix inconsistencies in global styles feature selectors#11857tellthemachines wants to merge 10 commits into
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
ae8bc5e to
3254b82
Compare
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@tellthemachines I have pushed updates from my refactor to This will pair with #12306, which I think will break without it (this is due to tests being written which depend on earlier parsing errors, which this PR will now fix). |
|
@dmsnell thanks for the update, but relevant PHP tests are now failing due to a discrepancy in spacing between comma-separated selectors. Could you update your changes to reinstate the missing space? (Or otherwise update the test strings; but reinstating the space would be more consistent with how we treat those selectors elsewhere) |
@tellthemachines I wrestled with this in the update, because I expected downstream code to expect that each component would start with the actual selector. in three out of the six cases where that’s happening, the call-sites are calling we can change it back, but I think it might be less risky if we removed that burden from every caller, particularly because of the demonstrated mismatch between the default either way I will follow up, but what are your thoughts here?
there’s another inconsistency regardless of the spaces which is what to do with things like comments. I left them in here because there was nothing hinting that this method would remove them, but I was trying to follow through with the idea that this breaks CSS selector string into separate strings where each is its own selector, as much as that’s possible without defining these other cases. |
(I haven't dug deeply into this btw, my comment was based on comparing test strings where there are spaces around most comma-separated selectors) I think it's fine to just update the tests if you're confident this is a better solution. |
If we declare it at the I’ll update the tests, as I think that will work out more along the lines of peoples’ expectations. Like I noted, there are still inconsistencies due to the mismatch between the mental model of the method “split selectors” and the fact that there may be more than selectors in a CSS selector string. In the process I found what appears to be code that 100% could never work as expected from WordPress/gutenberg#58051 and am trying to understand and address it. |
5b3273d to
74bfb0e
Compare
|
Thanks for the updates @dmsnell! This won't be fully testable in the core dev env until the editor files are synced to include WordPress/gutenberg#79210. But the equivalent changes in Gutenberg are testing well fwiw. Regarding your observation about comments, I hadn't seen it as an issue because this function isn't used for processing arbitrary CSS, but is only applied on strings that come from block or theme.json-shaped data, which is very unlikely to contain comments. We do know that we don't want comments in selectors 😅 so if there's a chance they could end up in there, it would be worth ensuring they're removed, but I don't think that's something we need to be concerned about right now. |
|
Thanks for merging @tellthemachines — I think I already noted this but I only examined the selector-splitting part and did not attempt to review the rest of the code or update anything. Really appreciate your efforts; I’ll get the Gutenberg updates merged promptly so everything runs in harmony. |
In #79499 the `split_selector_list()` method was updated to avoid a number of mis-parsing cases with CSS selectors. The Core backport in WordPress/wordpress-develop#11857 includes a couple of updates not caught during review in Gutenberg, so this patch includes those updates: - the speedup from #76556 is added to `append_to_selector()` as well as `prepend_to_selector()`. - all selectors are trimmed, even single-selector selector lists and the last selector in a list. - callees no longer manually trim the results, improperly, leaning instead on the behavior in `split_selector_list()`.
In #79499 the `split_selector_list()` method was updated to avoid a number of mis-parsing cases with CSS selectors. The Core backport in WordPress/wordpress-develop#11857 includes a couple of updates not caught during review in Gutenberg, so this patch includes those updates: - the speedup from #76556 is added to `append_to_selector()` as well as `prepend_to_selector()`. - all selectors are trimmed, even single-selector selector lists and the last selector in a list. - callees no longer manually trim the results, improperly, leaning instead on the behavior in `split_selector_list()`. Source: WordPress/gutenberg@e36bcc1

Trac ticket: https://core.trac.wordpress.org/ticket/65265
This is WordPress/gutenberg#78276 minus a little button width-related logic which is waiting for #12046 to be in.
Use of AI Tools
copied from WordPress/gutenberg#78276 with codex/gpt 5.5 and human-reviewed.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.