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

Scope list styles to UL/OL elements that are not blocks #37528

Closed
wants to merge 2 commits into from

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Dec 20, 2021

Fixes #37388

This PR changes how the styles of the list block are scoped to avoid applying them to top-level blocks that use the ol/ul HTML tags.

See #37388 for test instructions.

@oandregal oandregal requested a review from ajitbohra as a code owner December 20, 2021 14:10
@oandregal oandregal self-assigned this Dec 20, 2021
@oandregal oandregal added Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Question Questions about the design or development of the editor. and removed [Type] Question Questions about the design or development of the editor. labels Dec 20, 2021
@oandregal
Copy link
Member Author

Looking at the comments on #37388 we should go ahead with this.

@oandregal oandregal added [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Dec 21, 2021
@jasmussen
Copy link
Contributor

Thanks for the PR! This is a tricky one: style just the ul/ol elements, but don't style those elements when they're part of another block, such as post content.

On the face of it, this PR works. Before, the styles would seep in:
before

After, they don't:

after

However there's a problem. It doesn't work for the list block in the editor, because in the editor, the list block does have the wp-block-list CSS class, which this new CSS ignores:

however

Screenshot 2021-12-22 at 09 17 12

It's fine on the frontend, because the it's just the pure markup there:

fine on the frontend

Screenshot 2021-12-22 at 09 17 28

It also comes with the problem that the :not selector adds a little specificity to the ul/ol elements, though I think we should be able to write :where(ul:not([class*='wp-block'])) to lower that back down.


If we zoom out a bit further, this problem is likely going to come back again for other blocks that don't output a CSS class, such as Heading or Paragraph. Which means any blocks that include p or h[1-6] elements would be subject to the same cascade.

And so despite agreeing that the post list should not be affected by global styles set for other "blocks", given it's basic cascade I'm not not so sure we should do this after all. Imagine a few use cases:

  • You want a different font family, just for paragraphs.
  • You want a different color for headings.

Is the fact that these cascade into other blocks a benefit, or a downside? A this point I'm tempted to say it's a benefit, and that it would feel intuitive.

In the case of the Posts list, as I also noted on the thread, the use of ul/li is an implementation detail, which is why it feels jarring when the list change affects it.

I wonder if instead of scoping the list block CSS (which I think is going to hard, bordering on impossible) we could unset any inherited styles on the post list instead? What do you think?

@carolinan
Copy link
Contributor

There is also a request to add the wp-block-list class on the front
#12420
#36676

@jorgefilipecosta
Copy link
Member

I guess we can remove the class from the editor and merge this PR? In the future, we may consider adding the class wp-block-list without serialization (with PHP).

@jasmussen
Copy link
Contributor

jasmussen commented Dec 22, 2021

I don't think we should merge as is, because the not selector adds specificity.

As is I think we should either use where you reduce specificity, or (my current leaning), unset any inherited colors on the elements as part of the post list CSS.

@carolinan
Copy link
Contributor

carolinan commented Dec 22, 2021

I guess we can remove the class from the editor

I think part of the end goal is to use the same classes on the front and in the editor, removing it from the editor would not be ideal?

@oandregal oandregal force-pushed the update/scope-list-styles-to-no-blocks branch from d5165a3 to 1793978 Compare December 23, 2021 16:28
@oandregal
Copy link
Member Author

oandregal commented Dec 23, 2021

I made this work both in the editor and the front for block lists by using these set of selectors:

ol:where(:not([class*='wp-block'])),
ul:where(:not([class*='wp-block'])),
ol:where(.wp-block-list),
ul:where(.wp-block-list)

I wonder if instead of scoping the list block CSS (which I think is going to be hard, bordering on impossible) we could unset any inherited styles on the post list instead? What do you think?

The current fix should cover the more prominent cases, but it's limited: any inner list that doesn't have a .wp-block-list class is going to be affected by styles set by the user in the block list.

I'd be open to investigating alternatives (cover against this in the block's CSS) if they're more robust. I'm afraid I can't do it right now as I'll be out a few days. I can help when I'm back if nobody has gotten to it yet.

Longer-term, it'd be great to address the core issue by adding the class to the block (both in the editor and at render time in the frontend) to the three blocks that behave differently (paragraph, heading, list).

@youknowriad
Copy link
Contributor

youknowriad commented Jan 3, 2022

What about a "list" in classic block or a list in a "recipe" block or things like that? I feel that there should be a way to style all lists consistently in a theme (like links), would that make "lists" an element?

I'm not strongly opinionated but wanted to mention it's not really clear what the intent is.

@ndiego
Copy link
Member

ndiego commented Jan 11, 2022

Hopping on this PR late. What is the reason for not simply adding the wp-block-list class to lists created with the List block. This would provide a clean way for theme developers to target them with specific CSS as well.

@jasmussen
Copy link
Contributor

The list block is so atomic that to me, simply from a purity of markup perspetive, it makes no more sense to add a class to it than it would to add classes to a paragraph, which is affected in the same way as the list is, in this case. In that vein, I still think we should not do anything, and embrace this cascade as the intended behavior of the web; if you set a background color on a list or a paragraph in global styles, other blocks that use lists or paragraphs can be subject to that cascade, or unset them individually.

@ndiego
Copy link
Member

ndiego commented Jan 11, 2022

Conceptually I agree, but lists are used in so many ways that don't "look like a list" to users. A good example is the Social Icons block. If a user goes into Global Styles and changes the background color of the List block to red and then sees the background of their Social Icons block become red, it seems like a bug.

Understanding that it is "messy", I am advocating for all lists added via the List block itself to get the wp-block-list class.

@bgardner
Copy link

bgardner commented Jan 11, 2022

I am in full support of adding .wp-block-list on the front end to UL and OL's. With the former, it's especially helpful because as @ndiego mentioned, UL's are used with blocks such as navigation and social links. Having to include CSS in style sheets to account for this (especially as the goal is to avoid them altogether) seems unnecessary if a class can be added. This would also allow for margin support in theme.json as well, for those who want to use it.

@oandregal
Copy link
Member Author

Hey, so I want to resolve this PR in one way (merge) or another (close). We are too close to the release and already in RC2. It also doesn't seem this approach has gained too much consensus and this change could have unintended effects at such short notice. I lean towards closing it.

I'm also interested in fixing this. How about this as a plan going forward?

  • We introduce classes for the blocks that still don't have them (see related thread). This means the list, paragraph, and heading blocks gain classes. As a first step, the classes are only attached at runtime in the frontend (so no serialization to the blocks).
  • We also introduce three new elements: a paragraph (p as selector), an unordered list (ul as a selector), and an ordered list (ol as a selector). This matches what we have with headings (a block plus 6 elements).

By doing this, the intent becomes clear: when the user (or theme author via theme.json) wants to target all HTML elements they use the elements UI, when they want to target only the blocks they use the block UI.

Thoughts?

@ndiego
Copy link
Member

ndiego commented Jan 12, 2022

By doing this, the intent becomes clear: when the user (or theme author via theme.json) wants to target all HTML elements they use the elements UI, when they want to target only the blocks they use the block UI.

Love your proposal @oandregal 🙌

@jasmussen
Copy link
Contributor

jasmussen commented Jan 12, 2022

There's something unique about the list block, but even more so the paragraph block which is the primary hesitation to adding classes. They are such base ingredients in the journal of the web that additional identification via classes read to me as superfluous. The line is blurry, and for lists it might be fine. But in more general terms, the challenge is the same with headings and paragraphs, which is where it feels like crossing a line for good markup.

Going back to the intent, customizing the global styles of lists (but theoretically also paragraphs and headings), and having that bleed into other blocks, I'm not convinced is an issue that is best solved through scoping of styles. For blocks like social icons, the inheritance of undesired styles (like a background color) could be fixed by unsetting them in the social links block itself. Paddings and list-styles are already unset to repurpose the list for its horizontal layout, adding a transparent background to those resets feels like a softer starting point. In other cases, such as customizing the font of the Heading block and having that affect headings in a Posts Lists, honestly that feels expected to me, enough that the cascade could be argued to be a feature rather than a bug.

I don't mean that to dismiss the original report, and adding classes is something we can do. But it also feels like something to do as a last resort, when all other options are explored, and I'm not sure we're at that point with the List block quite yet.

@ndiego
Copy link
Member

ndiego commented Jan 12, 2022

Perhaps a better differentiation between "elements" and "blocks" is needed. If I change the styling of a "block" in Global Styles, the UI leads me to believe only that specific block should be altered. This led to my personal confusion around changing the color on a List block impacting other blocks that use list markup. On the other hand, if I instead change the styling on an "element" (at to top level, not within block-specific styles), then it makes sense that it should cascade through all blocks that include that element, i.e links and whatnot. It seems like we are mixing "blocks" and the concept of "elements". 🤔

@jasmussen
Copy link
Contributor

Perhaps a better differentiation between "elements" and "blocks" is needed.

Would it help if in global styles, Headings, Lists and Paragraphs were separated out with a subheading and/or descriptive text?

@jasmussen
Copy link
Contributor

I created a pair of PRs that together could address the original issue. Social icons: #37940, Posts list: #37941.

@carolinan
Copy link
Contributor

Would it help if in global styles, Headings, Lists and Paragraphs were separated out with a subheading and/or descriptive text?

Only partially, because the list element is used as part of blocks where it is not at all obvious to end-users that it is technically a list. Like the examples with the post template and social icon block changing colors.

@ndiego
Copy link
Member

ndiego commented Jan 13, 2022

Would it help if in global styles, Headings, Lists and Paragraphs were separated out with a subheading and/or descriptive text?

I don't like this approach, but if we go this route it should be made extremely clear in the UI that for the Heading, List, and Paragraph blocks, any changes to global styles impact all headings, lists, and paragraphs on the user's site. This includes the markup added by the respective blocks as well as any other heading, list, and paragraph markup added by other blocks.

Currently the UI reads "Customize the appearance of specific blocks and for the whole site." Based on this description, the <ul> markup in the Socials Icons block should not be impacted by styles added to the List block as I understand it.

This made me think of another issue. While #37941 and #37940 provide a solution to the perceived bug, similar solutions will need to be made for every block that uses heading, list, and/or paragraph markup. This puts a burden on third-party block developers as they will need to manually unset all these styles if their blocks include such markup.

@noisysocks
Copy link
Member

Hi! 👋 Friendly nudge that today (Monday) is the last day for this PR to be merged if it is to ship in WP 5.9. I'll begin backporting merged PRs on Tuesday morning Australian Eastern time.

@noisysocks noisysocks added Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Jan 17, 2022
@noisysocks
Copy link
Member

The 5.9 ship has sailed so moving this to 5.9.1 🙂

@oandregal
Copy link
Member Author

I see the original issue this intended to fix is now closed. I'm closing this as well.

@oandregal oandregal closed this Feb 9, 2022
@oandregal oandregal deleted the update/scope-list-styles-to-no-blocks branch February 9, 2022 09:38
@oandregal oandregal removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 this pull request may close these issues.

Global Styles: Setting color for List block impacts Post Template block
8 participants