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

[Heading block] Per-level styling of heading blocks does not work #49070

Closed
costdev opened this issue Mar 14, 2023 · 18 comments
Closed

[Heading block] Per-level styling of heading blocks does not work #49070

costdev opened this issue Mar 14, 2023 · 18 comments
Assignees
Labels
[Block] Heading Affects the Headings Block 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

@costdev
Copy link
Contributor

costdev commented Mar 14, 2023

Description

Originally reported on Trac 57904.

As reported in the Miscellaneous Editor changes in WordPress 6.2, one should be able to style the h1-h6 elements of a core/heading block differently to the global h1-h6 elements via theme.json:

{
	"styles": {
		"elements": {
			"h1": {
				"color": {
					"background": "blue"
				}
			}
	        },
		"blocks": {
			"core/heading": {
				"elements": {
					"h1": {
						"color": {
							"background": "pink"
						}
					}
				}
			}
		}
	}
}

However, this does not work as the CSS that is generated by the WordPress style engine is as follows:

.wp-block-heading h1{background-color: pink;}

Where it should be more like:

h1.wp-block-heading{background-color: pink;}

Tested in 6.2-RC1-55503 with Twenty Twenty-Three.

Step-by-step reproduction instructions

  1. Open the Twenty Twenty-Three theme's theme.json in your favorite code editor.
  2. Find "styles" > "blocks" on about line 291.
  3. Add the following json to as the first element under "blocks" (above "core-navigation"):
"core/heading" : {
  "elements": {
	"h1": {
	  "color": {
		"background": "pink"
	  }
	}
  }
},
  1. Save the file.
  2. Launch your test suite.
  3. On the front-end, open your browser's Dev Tools interface.
  4. In the "Inspector" search for .wp-block-heading-inline-css. 🐞 Bug occurs.

Expected results
The generated CSS for the h1 should be:

h1.wp-block-heading{background-color: pink;}

Actual results
The generated CSS for the h1 is incorrectly set as:

.wp-block-heading h1{background-color: pink;}

The full style (at the time of this comment):

<style id="wp-block-heading-inline-css">
h1.has-background,h2.has-background,h3.has-background,h4.has-background,h5.has-background,h6.has-background{
  padding:1.25em 2.375em;
}
.wp-block-heading h1{background-color: pink;}
</style>

Screenshots, screen recording, code snippet

No response

Environment info

Environment details from @hellofromtonya:

OS: macOS
Web Server: wp-env (Docker)
WordPress: 6.2-RC1-55503
Browser: Firefox 110.0.1, Edge, and Chrome
Theme: Twenty Twenty-Three
Active Plugins: None

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

@hellofromtonya
Copy link
Contributor

As this was introduced in 6.2 and is not working, it can be fixed in 6.2 RC3 if ready in time. Please advise @adamziel @Mamaduka @nickcernis

@hellofromtonya hellofromtonya added the [Type] Bug An existing feature does not function as intended label Mar 14, 2023
@hellofromtonya
Copy link
Contributor

Related to #42122 (comment).

@hellofromtonya hellofromtonya changed the title Per-level styling of heading blocks does not work [Heading block] Per-level styling of heading blocks does not work Mar 14, 2023
@hellofromtonya hellofromtonya added [Block] Heading Affects the Headings Block Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Mar 14, 2023
@annezazu annezazu moved this from ❓ Triage to 📥 Todo in WordPress 6.2 Editor Tasks Mar 15, 2023
@scruffian
Copy link
Contributor

I think this is a very involved fix. At the moment element selectors as output using this:

static::append_to_selector( $el_selector, $selector . ' ', 'left' )

In this specific case (and only in this case) we need to output the selector using this:

static::append_to_selector( $el_selector, $selector . '', 'right' )

The difficult part is determining when to use this selector. I think what we need is some config inside the block.json for the heading block.

"selectors" {
    "elements": {
        "h1": {
            "toAppend": "",
            "position": "right"
        }
    }
}

This seems very verbose, I'm sure there's a better way. Pinging @aaronrobertshaw because of this work on #46496.

@aaronrobertshaw
Copy link
Contributor

Thanks for the ping @scruffian 👍

In this specific case (and only in this case)

Given the impact of the bug and the fact it sounds very specific to the heading block edge case. I think getting in a quick fix to buy us some time on a more elegant and flexible solution makes sense. I've knocked up a quick fix in #49189.

I think what we need is some config inside the block.json for the heading block.

Such an approach might provide the most flexibility overall.

As for the proposed shape, I think we'd likely avoid nesting elements under the selectors. We already have style/editorStyle, and now with #46496, we'll also have selectors/editorSelectors. Perhaps elementSelectors would fit there in terms of consistency. Maybe more so, given the different shape and processing of elements.

I'd be reluctant to try and work element selectors into #46496 at this late stage but exploring them as a follow-up sounds good.

For a fix to land in 6.2, we won't be able to rely on a new feature such as the stabilized and extended selectors API, so that might be another reason to try and sneak #49189 in.

What do you think?

@Mamaduka
Copy link
Member

Looking at the original PR from @adamziel and the associated issue - #42082, I'm more convinced that there's a mistake in the dev-note.

The original PR fixes styles "leaking" from the Heading block definition to global heading elements, and it doesn't contain any changes to WP_Theme_JSON, that would support per-level styling.

The styles.blocks.{blockName}.elements target child elements of the block and not the block itself. However, with #49189, we might promote the behavior that only works for a single block.

@aaronrobertshaw
Copy link
Contributor

Taking a closer look at the original PR and issue leads me to the same conclusion @Mamaduka. Nice catch!

It seems like a copy-and-paste error in the dev note's snippet, just failing to remove the styles.blocks.core/heading.elements wrapper around the block's color styles. This issue and the fact the dev note's already been published probably means we need to clarify the wording more as well.

However, with #49189, we might promote the behavior that only works for a single block.

Agreed. I'll close #49189 for the reasons you've pointed out 👍

@ntsekouras
Copy link
Contributor

After looking at these threads, I also believe is a mistake in the dev note. @adamziel can you verify? I think we need to correct the dev note and close this one.

@scruffian
Copy link
Contributor

I agree, the example given should style heading elements inside heading blocks, which is not possible. I suggest we update the dev note to use the cover block as an example, as it does support heading elements. Thanks everyone.

@github-project-automation github-project-automation bot moved this from 📥 Todo to ✅ Done in WordPress 6.2 Editor Tasks Mar 21, 2023
@hellofromtonya
Copy link
Contributor

Reopening until the Dev Note is updated.

@hellofromtonya
Copy link
Contributor

cc @bph the dev note needs to be updated with this sample code @scruffian provided. Quoting it here:

Let's use this example for the dev note:

{
	"styles": {
		"elements": {
			"h1": {
				"color": {
					"background": "blue"
				}
			}
	        },
		"blocks": {
			"core/cover": {
				"elements": {
					"h1": {
						"color": {
							"background": "pink"
						}
					}
				}
			}
		}
	}
}

This will mean that H1 elements inside a cover block will have a pink background, but all other H1s will have a blue background.

@wongjn
Copy link

wongjn commented Mar 21, 2023

I'm not sure the example illustrates the impact of the change to add the wp-block-heading CSS class to every Heading block. The example would work in WP 6.1 if I am not mistaken?

@scruffian
Copy link
Contributor

Yes, you're right, I have reopened #42082, I'm not sure how to proceed.

@Mamaduka
Copy link
Member

Thank you, @scruffian!

I will close this issue. The dev note is up to date.

@scruffian
Copy link
Contributor

Maybe we don't need a dev note at all since we didn't actually fix anything?

@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label May 11, 2023
@Lovor01
Copy link
Contributor

Lovor01 commented Oct 12, 2023

I am still experiencing the issue (WP 6.3.1) - elements node inside the core/heading block does not work at all, i.e. this does not apply specific styles to h1, h2, h3:

			"core/heading": {
				"typography": {
					"fontFamily": "var(--wp--preset--font-family--title-font)",
					"fontWeight": "700",
					"lineHeight": "1.2"
				},
				"elements": {
					"h1": {
						"typography": {
							"fontSize": "4.375rem",
							"textTransform": "uppercase"
						}
					},
					"h2": {
						"typography": {
							"fontSize": "2.5rem"
						}
					},
					"h3": {
						"typography": {
							"fontFamily": "var(--wp--preset--font-family--content-font)"
						}
					}
				}
			},

@bph
Copy link
Contributor

bph commented Oct 12, 2023

@Lovor01 Could you please create a new issue. That way more people see you are still having trouble and also probably new set of environment information.

@aaronrobertshaw
Copy link
Contributor

Thanks for providing the theme.json snippet @Lovor01 👍

elements node inside the core/heading block does not work at all, i.e. this does not apply specific styles to h1, h2, h3:

Earlier in this issue's discussion, it was noted that the heading block didn't yet support "per-level" element styling. When creating the new issue as @bph suggested, it might need to be as an enhancement or feature request.

@scruffian
Copy link
Contributor

Reported here: #55417

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Heading Affects the Headings Block 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
No open projects
Development

Successfully merging a pull request may close this issue.

10 participants