Backport: Gutenberg 44731 by scruffian · Pull Request #3481 · WordPress/wordpress-develop

scruffian · 2022-10-17T11:28:12Z

This backports WordPress/gutenberg#44731

I couldn't get it to work locally because the CSS file didn't seem to be there, but maybe I'm testing it wrong?

ockham · 2022-10-17T11:35:40Z

I couldn't get it to work locally because the CSS file didn't seem to be there, but maybe I'm testing it wrong?

I haven't tested it yet, but did you npm i before building to make sure you've got the latest package versions?

scruffian · 2022-10-17T13:29:00Z

Yes, but I still didn't see the CSS file...

ockham · 2022-10-17T15:48:14Z

Hmm, I am getting

  | <br />
-- | --
  | <b>Warning</b>:  file_get_contents(/wp-includes/css/dist/block-library/classic.css): failed to open stream: No such file or directory in <b>/var/www/src/wp-includes/script-loader.php</b> on line <b>3696</b><br />
  | <br />
  | <b>Warning</b>:  Cannot modify header information - headers already sent by (output started at /var/www/src/wp-includes/script-loader.php:3696) in <b>/var/www/src/wp-admin/admin-header.php</b> on line <b>9</b><br />

src/wp-includes/script-loader.php

ockham · 2022-10-17T19:28:36Z

src/wp-includes/script-loader.php

+	// only handles external files or theme files.
+	$editor_settings['styles'][] = array(
+		'css'            => file_get_contents( $classic_theme_styles ),
+		'__unstableType' => 'theme',


Maybe this should be core? It's arguably Core that's providing this styling 🤔

cc/ @oandregal @jorgefilipecosta for Global Styles expertise 😊

I'll tentatively change it to core since IMO it better represents where the styling is coming from.

Since this is targeting classic themes, it shouldn't have any actual implications, since this is for Global Styles -- which are unavailable for Classic Themes.

Thanks, that's a good idea.

👋 Looked at where __unstableType is used. It's only at https://github.com/WordPress/gutenberg/blob/88a1f7ae18bea3a13f6d1e507e3c25090b76cb08/packages/edit-post/src/editor.js#L165

If I'm reading the original PR correctly, this is used to decide whether to render a particular stylesheet when the user deactivates "theme styles". If the stylesheet needs to be disabled by that check, we should use "theme", otherwise "core" is fine. See check in this screen:

These styles are the "default" ones set for buttons by core, they aren't connected to a specific theme, so I think core is correct.

ockham

Thanks! This LGTM and fixes the issue for me:

In the post editor, with Twenty Ten active:

ockham · 2022-10-17T21:24:34Z

Ugh, unit tests are failing because they cannot locate that file:

file_get_contents(/var/www/src/wp-includes/css/dist/block-library/classic.css): failed to open stream: No such file or directory

I'll look into this tomorrow. Not sure if something is off with the way we're building things for unit tests, or if there's another reason 😕

scruffian · 2022-10-18T09:37:41Z

We found another bug with the original solution, which I have a fix for here: WordPress/gutenberg#45063

I'll also backport that in this PR

ockham · 2022-10-19T10:07:22Z

Ugh, unit tests are failing because they cannot locate that file:
file_get_contents(/var/www/src/wp-includes/css/dist/block-library/classic.css): failed to open stream: No such file or directory
I'll look into this tomorrow. Not sure if something is off with the way we're building things for unit tests, or if there's another reason 😕

Some quick thoughts on this:

My current working hypothesis is that maybe we're not building @wordpress package JS and CSS before running PHP unit tests.
We have other functions (e.g. wp_default_styles) that register styles that might (?) not exist at the time. How do they do it? (Or is it all classic stylesheets are readily available at the time that tests are run?)
Are there any helpers we can use instead of file_get_contents that will avoid the "not found" error at test time (via some indirection?)
Finally, do we need to mock that file, or file_get_contents when run for that file, or all of wp_add_editor_classic_theme_styles?

scruffian · 2022-10-19T10:09:29Z

Would it be simpler to just use a static CSS file?

ockham · 2022-10-19T10:41:46Z

Would it be simpler to just use a static CSS file?

Probably 🤔 Maybe due to my ignorance -- there might be established patterns to deal with built CSS in a scenario like this, I'm just unaware of it.

Would we lose anything significant if we made it static?

scruffian · 2022-10-19T10:42:36Z

Would we lose anything significant if we made it static?

Nothing at all

scruffian · 2022-10-19T10:52:02Z

Done in 20fed67, lets hope the tests pass!

cbravobernal · 2022-10-19T12:16:01Z

Using TwentyTwenty Twelve, it seems that the font color of the buttons is different 😢
The CSS specificity seems to be the same on all themes, I tried with 2010, 2011, 2012, and 2013, so maybe that error is not 100% related to this backport.
Also, the theme CSS file is loading correctly in all of them.

Editor:

Frontend:

scruffian · 2022-10-19T13:09:50Z

I added a commit to fix Twenty Twelve

Need to re-review with the latest fixes

ockham

Thank you @scruffian! LGTM

audrasjb

[committer double signoff]
The changeset looks good to me as well.

Self assigning for commit.

Edit: Not yet. See the comment below.

audrasjb

Hmm, I think the PR is missing RTL styles, at least for Twenty Twenty.

sabernhardt · 2022-10-20T16:04:00Z

Yes, Twenty Twenty also needs the RTL stylesheet, but block styles would belong in editor-style-block.css instead of classic.

Twenty Twelve does not have an RTL stylesheet for the editor.

scruffian · 2022-10-20T16:45:35Z

Yes, Twenty Twenty also needs the RTL stylesheet, but block styles would belong in editor-style-block.css instead of classic.

The styles are already in editor-style-block.css, but the problem is that the editor block styles aren't provided through add_editor_style as they are usually. This means they don't get prefixed with editor-styles-wrapper, so they don't have the same specificity as the ones coming from this new CSS file. An alternative would be to manually add this prefix to the rules inside editor-style-block.css which I am happy to do if you think that approach is better.

sabernhardt · 2022-10-20T17:56:57Z

The new inline styles come after editor-style-block.css with equal specificity. If you make the selectors slightly more specific for lines 927-928 and the font size adjustment at lines 1308-1309, that should be enough to correct the original state without changing the theme's Classic Editor stylesheet.

:root .editor-styles-wrapper .wp-block-button__link,
:root .editor-styles-wrapper .wp-block-file__button {

Twenty Twenty does not change text color on hover, but you could add the underline in editor-style-block.css, without :root. (Button block elements are not focusable in the editor.)

.editor-styles-wrapper .wp-block-button__link:hover,
.editor-styles-wrapper .wp-block-file__button:hover {
	text-decoration: underline;
}

…tyles

scruffian · 2022-10-21T10:30:54Z

@sabernhardt thanks that's super helpful. I have updated editor-style-blocks and revert the changes to the classic styles. I also updated the RTL stylesheet. Thanks again for all your help.

ockham

I retested Twenty Twenty in Hebrew to verify that RTL styling works ✅

I also tested Twenty Ten and Twenty Twelve to verify that button styling still works there ✅

Changes look good to me 👍

audrasjb · 2022-10-24T13:06:55Z

[committer double signoff]

Yeah thanks for the changes. Looks good to me now 👍

ockham · 2022-10-25T09:47:42Z

Commit message draft:

Editor: Correctly apply Button block styles for classic themes

In [54358], styling for Button blocks that had been removed from classic themes was reintroduced. However, it was added with a global scope, whereas editor styles are usually added with a `.editor-styles-wrapper` selector, which makes them more specific.

This change modifies the way that classic theme styles are added so that they also get wrapped in an `.editor-styles-wrapper` selector to match specificity.

Furthermore, adjust specificity for some Button block related styling in the editor for the Twenty Twelve and Twenty Twenty themes.

Merges [https://github.com/WordPress/gutenberg/pull/44731 Gutenberg PR 44731] into trunk.

Follow-up to [54358].
Props scruffian, cbravobernal, sabernhardt, audrasjb.
See #56467.

ockham · 2022-10-25T12:30:38Z

Committed to Core trunk in r54687. Awaiting second approval to backport to the 6.1 branch.

ockham · 2022-10-25T13:45:50Z

Committed to Core trunk in r54687. Awaiting second approval to backport to the 6.1 branch.

Merged to the 6.1 branch in r54688.

ockham mentioned this pull request Oct 17, 2022

WP6.1 Button block Themes based on block-library css will have a different style WordPress/gutenberg#44659

Closed

ockham reviewed Oct 17, 2022

View reviewed changes

src/wp-includes/script-loader.php Outdated Show resolved Hide resolved

ockham reviewed Oct 17, 2022

View reviewed changes

ockham previously approved these changes Oct 17, 2022

View reviewed changes

scruffian mentioned this pull request Oct 18, 2022

Classic themes: Only load classic theme button styles for themes that don't use theme.json WordPress/gutenberg#45063

Closed

ockham approved these changes Oct 20, 2022

View reviewed changes

audrasjb approved these changes Oct 20, 2022

View reviewed changes

audrasjb self-assigned this Oct 20, 2022

audrasjb requested changes Oct 20, 2022

View reviewed changes

scruffian and others added 5 commits October 21, 2022 10:12

Backport: Gutenberg 44731

88eb888

Pick changes from Gutenberg 45063

4fbe181

Fix path

96efadd

Remove baseURL

0136138

Simplify

3ac78f5

ockham and others added 4 commits October 21, 2022 10:12

Set __unstableType to core

865ce54

add core settings to the start of the array so themes can override them

2878b54

Add classic theme styles as a static file not in a package

48b9f9e

add a button text color for the editor

98d03a5

scruffian force-pushed the backpoort-44731 branch from 8ed4c10 to 98d03a5 Compare October 21, 2022 09:12

scruffian added 2 commits October 21, 2022 11:25

revert changes to the classic editor styles and update block editor s…

4f17c05

…tyles

update RTL

0b9b628

ockham approved these changes Oct 24, 2022

View reviewed changes

audrasjb approved these changes Oct 24, 2022

View reviewed changes

ockham closed this Oct 25, 2022

Conversation

scruffian commented Oct 17, 2022

Uh oh!

ockham commented Oct 17, 2022

Uh oh!

scruffian commented Oct 17, 2022

Uh oh!

ockham commented Oct 17, 2022

Uh oh!

Uh oh!

ockham Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

ockham Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

ockham Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

scruffian Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

oandregal Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

scruffian Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

ockham left a comment

Choose a reason for hiding this comment

Uh oh!

ockham commented Oct 17, 2022

Uh oh!

scruffian commented Oct 18, 2022

Uh oh!

ockham commented Oct 19, 2022

Uh oh!

scruffian commented Oct 19, 2022

Uh oh!

ockham commented Oct 19, 2022

Uh oh!

scruffian commented Oct 19, 2022

Uh oh!

scruffian commented Oct 19, 2022

Uh oh!

cbravobernal commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scruffian commented Oct 19, 2022

Uh oh!

ockham left a comment

Choose a reason for hiding this comment

Uh oh!

audrasjb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

audrasjb left a comment

Choose a reason for hiding this comment

Uh oh!

sabernhardt commented Oct 20, 2022

Uh oh!

scruffian commented Oct 20, 2022

Uh oh!

sabernhardt commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scruffian commented Oct 21, 2022

Uh oh!

ockham left a comment

Choose a reason for hiding this comment

Uh oh!

audrasjb commented Oct 24, 2022

Uh oh!

ockham commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ockham commented Oct 25, 2022

Uh oh!

ockham commented Oct 25, 2022

cbravobernal commented Oct 19, 2022 •

edited

Loading

audrasjb left a comment •

edited

Loading

sabernhardt commented Oct 20, 2022 •

edited

Loading

ockham commented Oct 25, 2022 •

edited

Loading