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

Restore html scope in reset.scss selectors #61959

Closed
wants to merge 2 commits into from

Conversation

sabernhardt
Copy link
Contributor

Fixes: #57176

(simplest option)

What?

Raise the specificity of reset selectors to what they were before #46752 so that the CSS reset overrides styles in wp-admin/css/common.css.

Why?

The admin CSS bleed affects the iframed post editor in 'classic' themes that do not specify font-size, line-height and/or background-color for the body. In addition to the extra discrepancies between editor and front end, the smaller text is more difficult to read when editing post content, and the background reduces color contrast.

How?

Revert the reset.scss change from #46752.

Testing Instructions

  1. Activate a 'classic' theme such as Twenty Thirteen or Twenty Fourteen (those two currently have the wrong font size and background color).
  2. Create a new post and add blocks including a Paragraph and/or List.
  3. Check whether the post editor is using an iframe by using the browser inspector on text in a Paragraph block. If it is not inside an iframe, you may need to save the post and then deactivate plugins or go to Preferences and remove a panel such as Custom Fields.
  4. In the browser inspector, look for styles on the body element coming from common.css (or load-styles.php if SCRIPT_DEBUG is false). With the PR applied, the background, color, font-family, font-size and line-height properties should have a line through them (the min-width is not reset, but that seems to be fine).
    admin body styles crossed out, except for min-width

This option is the simple fix.
@sabernhardt sabernhardt requested a review from ajitbohra as a code owner May 24, 2024 16:23
Copy link

github-actions bot commented May 24, 2024

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 props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @peXed.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: peXed.

Co-authored-by: sabernhardt <sabernhardt@git.wordpress.org>
Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: carolinan <poena@git.wordpress.org>
Co-authored-by: karmatosed <karmatosed@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@carolinan carolinan requested a review from ellatrix May 28, 2024 08:15
@carolinan carolinan added [Type] Bug An existing feature does not function as intended CSS Styling Related to editor and front end styles, CSS-specific issues. labels May 28, 2024
@carolinan
Copy link
Contributor

This change solves the problem with the unexpected grey background color in classic themes.
What negative side effects do we need to test for?

@fabiankaegy
Copy link
Member

@aaronrobertshaw I think this one may be of interest to you.

@@ -7,7 +7,7 @@

// We use :where to keep specificity minimal.
// https://css-tricks.com/almanac/selectors/w/where/
:where(.editor-styles-wrapper) {
html :where(.editor-styles-wrapper) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From recent other PR's such as #61638 I think :root would be a better selector here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used html so this would be exactly as safe as it was in WordPress 6.3.

The only selector that I know needs an increased specificity is :where(.editor-styles-wrapper), so it can override the admin styles on body, and 0-0-1 would be enough for that element.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this is a reset stylesheet, I'm inclined towards the 0-0-1 specificity as well.

Perhaps we could further isolate the change here by extracting only the rules needed to override the common.css body styles under the restored selector. For example:

html :where(.editor-styles-wrapper) {
	/**
	* The following styles revert to the browser defaults overriding the WPAdmin styles.
	* This is only needed while the block editor is not being loaded in an iframe.
	*/
	font-family: serif; // unfortunately initial doesn't work for font-family.
	font-size: initial;
	line-height: initial;
	color: initial;
	// Many themes with white backgrounds load editor styles but fail to also provide
	// an explicit white background color, assuming a white editing canvas.
	// So to match browser defaults, we provide a white default here as well.
	background: #fff;
}

The rest could remain under :where(.editor-styles-wrapper).

There is also another PR around the editor styles wrapper that might be worth taking a look at to see how these changes might be affected.

Copy link
Contributor Author

@sabernhardt sabernhardt May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to isolate the one element, you could use the html & method I mentioned in #57176 (comment)

However, a PR like that is more complex than I should try to create within the browser, so someone else should make it.

Plus, the comment about not being in an iframe is inaccurate (with classic themes), so that sentence could be removed too.

@@ -7,7 +7,7 @@

// We use :where to keep specificity minimal.
// https://css-tricks.com/almanac/selectors/w/where/
:where(.editor-styles-wrapper) {
html :where(.editor-styles-wrapper) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
html :where(.editor-styles-wrapper) {
:root :where(.editor-styles-wrapper) {

@carolinan
Copy link
Contributor

How can we arrive at a final decision about how to solve?

@carolinan carolinan added the Needs Decision Needs a decision to be actionable or relevant label Jun 4, 2024
@sabernhardt
Copy link
Contributor Author

#62350 is the version that only creates html :where(.editor-styles-wrapper) once in the compiled stylesheet.

@sabernhardt
Copy link
Contributor Author

#62350 has been merged instead of this one.

@sabernhardt sabernhardt deleted the reset-html-selector branch June 24, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. Needs Decision Needs a decision to be actionable or relevant [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: iframed post editor can show some admin styles with classic themes
4 participants