#57561 closed defect (bug) (fixed)
Update conditional logic for editor_styles
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | 6.1 |
Component: | Script Loader | Keywords: | has-patch has-testing-info commit |
Focuses: | Cc: |
Description
Currently the logic for including editor_styles
is inconsistent in some places where it is called. We should make this consistent so that both the editor and the front end receive styles using the same logic.
An example of an inconsistency this is currently causing is the width separator block on the homepage of Twenty Twenty-Three. As Twenty Twenty-Three does not include any editor styles, the opinionated block styles (wp-block-styles
) are included in the editor but not on the front end. This is because the following logic is being used:
if ( ! is_array( $editor_styles ) || count( $editor_styles ) === 0 ) { $wp_edit_blocks_dependencies[] = 'wp-block-library-theme'; }
But this should also include a check for wp-block-styles
, for example:
if ( current_theme_supports( 'wp-block-styles' ) && ( ! is_array( $editor_styles ) || count( $editor_styles ) === 0 ) ) { $wp_edit_blocks_dependencies[] = 'wp-block-library-theme'; }
This was originally opened in the editor, but I believe the changes will need to be separately merged with Core anyway, so I've opened a ticket here as well.
Editor PR: https://github.com/WordPress/gutenberg/pull/45351
Attachments (6)
Change History (22)
This ticket was mentioned in PR #3916 on WordPress/wordpress-develop by @mikachan.
2 years ago
#1
- Keywords has-patch added
@
2 years ago
Before patch, the group has incorrect padding in the Site Editor (Twenty Twenty-Three)
@
2 years ago
After patch, the theme's global padding settings (padding aware alignments) are respected.
#3
@
2 years ago
- Keywords needs-testing needs-testing-info added
- Version set to 6.1
I have copied these testing instructions from the Gutenberg PR:
Testing Instructions
Activate Twenty Twenty-Three or a theme that doesn't support
wp-block-styles
AND doesn't include add_editor_style( 'style.css' );
In the Site Editor, insert a Separator block, see the difference in width between the editor and front end
Example separator markup for copy/pasting:
<!-- wp:separator --> <hr class="wp-block-separator has-alpha-channel-opacity"/> <!-- /wp:separator -->
Expected result with the patch applied:
The editor and front should match.
@mikachan These testing instructions are for the "happy path", do you have instructions for what we need to test in other types of themes, to make sure there are no regressions for them?
-The logic in the condition should be clear, but we also thought that about the code pre-patch *smile*
#4
@
2 years ago
Thanks for testing this out, @poena!
Great point, here's a "sad path" to test for:
Ensure themes that opt into wp-block-styles
still include the correct styles on both the front end and the editor. Check that the theme.css ('wp-block-library-theme-css') is included where expected. The blocks that use this CSS are listed here: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/theme.scss
This ticket was mentioned in Slack in #core by costdev. View the logs.
2 years ago
#7
@
2 years ago
Test Report
PR tested: https://github.com/WordPress/wordpress-develop/pull/3916
Steps to Reproduce or Test
- Activate Twenty Twenty-Three theme
- Enter the Editor
- Insert Separator block
Actual Results
When reproducing a bug/defect:
- ❌ Error condition occurs.
When testing the bugfix patch:
- ✅ Issue resolved with patch.
FYI, while testing the solution I noticed that the Separator splits into two bars depending on the screen width (see https://imgur.com/RVbMCx4).
#8
@
2 years ago
Thank you for testing, @sannevndrmeulen!
FYI, while testing the solution I noticed that the Separator splits into two bars depending on the screen width (see https://imgur.com/RVbMCx4).
It looks like there's an issue open for this for the editor: https://github.com/WordPress/gutenberg/issues/31424. It's been open a while, I'll see if I can take a look at it soon.
@hellofromTonya commented on PR #3916:
2 years ago
#9
Tests for failing for
Tests_Dependencies_Styles::test_block_styles_for_editing_without_theme_support Failed asserting that false is true.
for the last assertion in this test:
{{{php
public function test_block_styles_for_editing_without_theme_support() {
Confirm we are without theme support by default.
$this->assertFalse( current_theme_supports( 'wp-block-styles' ) );
wp_default_styles( $GLOBALSwp_styles? );
$this->assertFalse( wp_style_is( 'wp-block-library-theme' ) );
wp_enqueue_style( 'wp-edit-blocks' );
$this->assertTrue( wp_style_is( 'wp-block-library-theme' ) );
}
}}}
Looking at the code in this PR, the 'wp-block-library-theme'
style is no longer enqueued if the theme doesn't support 'wp-block-styles'
. This means the last assertion in this test should expect the state the state not to change, i.e. assertFalse()
.
In reading the intent of the original Gutenberg PR:
This PR is an attempt to never allow the
wp-block-library-theme
CSS to be loaded under the conditions we introduced in https://github.com/WordPress/gutenberg/pull/44640.
this does appear to be the intent, i.e. if the theme doesn't support 'wp-block-styles'
, then wp_style_is( 'wp-block-library-theme' )
should be false
.
I'll update the unit test accordingly.
#10
@
2 years ago
- Keywords has-testing-info added; needs-testing needs-testing-info removed
- Owner set to hellofromTonya
- Status changed from new to reviewing
This ticket is a synchronization to merge released PRs/work from Gutenberg into Core. However, the Gutenberg PR has not yet been merged into Gutenberg nor released in a Gutenberg release.
@mikachan is the goal to simultaneous test in Gutenberg and Core?
@mikachan commented on PR #3916:
2 years ago
#11
Thank you for updating that test, @hellofromtonya 🙇♂️
if the theme doesn't support 'wp-block-styles', then wp_style_is( 'wp-block-library-theme' ) should be false.
This is correct 👍
#12
@
2 years ago
is the goal to simultaneous test in Gutenberg and Core?
I don't believe the fix needs to be tested in Gutenberg (i.e. included in the plugin and tested by users before being merged into Core), as it's a fix for something that already exists in Core.
The reason I opened this patch in both Core and Gutenberg was just because I wasn't sure where the fix should be applied. I started the work in Gutenberg, but then I realised that I was only editing compat files, so I opened a Core patch just in case this was more appropriate.
If we were to merge the fix in Core, would the Gutenberg PR still be required?
#13
@
2 years ago
Test Report
Patch: https://github.com/WordPress/wordpress-develop/pull/3916
Env:
- OS: macOS
- Browser: Firefox, Edge, and Chrome
- Plugins: none
- Theme: switched between TT2 and TT3
Twenty Twenty-Two (TT2) is setting theme support for 'wp-block-styles'
whereas Twenty Twenty-Three (TT3) is not. Tested using both themes to validate the patch fixes TT3 without impacting TT2.
Results
TT3
Before applying the patch
I am able to reproduce the issue ❌.
As shown in tt3-before.png, notice:
- the
hr
visually looks the different between the post editor and front-end. - in the post editor, the
/wp-includes/css/dist/block-library/theme.css
stylesheet is loaded.
After applying the patch
The issue is resolved ✅
As shown in tt3-after.png, notice:
- the
hr
visually looks the same in the post editor and front-end ✅. /wp-includes/css/dist/block-library/theme.css
is no longer loaded into the post editor ✅ .
TT2
Before applying the patch
As shown in tt2-before.png, notice:
- the
hr
visually looks the same in the post editor and front-end. - in the post editor, the
/wp-includes/css/dist/block-library/theme.css
stylesheet is loaded.
After applying the patch
There's no difference ✅
As shown in tt2-after.png, notice:
- the
hr
visually looks the same in the post editor and front-end. /wp-includes/css/dist/block-library/theme.css
is no longer loaded into the post editor.
Summary
The patch:
- fixes TT3 (a theme does not add
'wp-block-styles'
theme support) ✅ - does not impact TT2 (a theme that does add
'wp-block-styles'
theme support) ✅
#14
@
2 years ago
- Keywords commit added; gutenberg-merge removed
Ready for commit
. Prepping now.
Note: Though there's a PR in Gutenberg, this ticket is not really a gutenberg-merge
. Why? As @mikachan explains, the bug should be fixed here and did not need to go through Gutenberg first. Removing the gutenberg-merge
keyword.
@hellofromTonya commented on PR #3916:
2 years ago
#16
Committed via https://core.trac.wordpress.org/changeset/55368.
Trac ticket: https://core.trac.wordpress.org/ticket/57561