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

Add html to reset styles for the .editor-styles-wrapper container #62350

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

sabernhardt
Copy link
Contributor

Fixes: #57176

(similar to option in #61959, but only increasing the specificity for one selector instead of the whole group)

What?

Raise the specificity of only the :where(.editor-styles-wrapper) selector in reset.scss 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?

  1. Add html to the :where(.editor-styles-wrapper) ruleset (in the compiled stylesheet).
  2. Remove inaccurate comment that says the reset is unnecessary in an iframe. Enqueuing the admin styles makes the reset necessary.

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
Also removes comment about not needing it within an iframe.
Copy link

github-actions bot commented Jun 5, 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: ellatrix <ellatrix@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.

// 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;
}
Copy link
Member

Choose a reason for hiding this comment

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

The admin styles seem to specifically target the body. Could we move these particular styles out of the here(.editor-styles-wrapper) block and under body, maybe with a link to which styles are being reset?

Also when did this break? Where you able to pinpoint the specific commit or release?

Copy link
Member

Choose a reason for hiding this comment

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

Cc @aaronrobertshaw in case it rings a bell

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The selector needs at least 0-0-1 specificity to override body, which it had before removing html in 46752.

I made #61959 to restore that selector exactly as it was for the whole set, and Aaron already suggested to isolate the change to only the first selector of the stylesheet.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, we should probably change the selector to:

div:where(.editor-styles-wrapper),
body:where(.editor-styles-wrapper)

So that it overrides the admin styles for both an iframed and non-iframe editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

html :where(.editor-styles-wrapper) applied to either element in WordPress 6.3, and I just made a quick edit directly in wp-includes\css\dist\block-library\reset.css with 6.6 beta to confirm that it still covers both elements.

iframe:

iframe editor with corrected background and font size

non-iframe:

non-framed editor with element inspector open

(the theme for these screenshots is Twenty Thirteen)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Restoring is what I tried first :) #61959

Copy link
Member

Choose a reason for hiding this comment

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

Yes, so we only want it to affect the top level styles. I guess I meant, why not take out that section and do it in a separate block? I find that a bit more readable than html &.

@ellatrix ellatrix added Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta [Type] Bug An existing feature does not function as intended labels Jun 6, 2024
@ellatrix ellatrix merged commit 58932ed into WordPress:trunk Jun 24, 2024
65 of 66 checks passed
@github-actions github-actions bot added this to the Gutenberg 18.7 milestone Jun 24, 2024
ellatrix added a commit that referenced this pull request Jun 25, 2024
…62350)

Also removes comment about not needing it within an iframe.

Unlinked contributors: peXed.

Co-authored-by: sabernhardt <sabernhardt@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: karmatosed <karmatosed@git.wordpress.org>
@ellatrix
Copy link
Member

I just cherry-picked this PR to the wp/6.6-rc-1 branch to get it included in the next release: 9f0380f

@ellatrix ellatrix added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Jun 25, 2024
ellatrix added a commit that referenced this pull request Jun 25, 2024
…62350)

Also removes comment about not needing it within an iframe.

Unlinked contributors: peXed.

Co-authored-by: sabernhardt <sabernhardt@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: karmatosed <karmatosed@git.wordpress.org>
@sabernhardt sabernhardt deleted the reset-css-editor-styles-wrapper branch June 26, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Type] Bug An existing feature does not function as intended
2 participants