Make WordPress Core

Opened 17 months ago

Closed 16 months ago

Last modified 16 months ago

#57561 closed defect (bug) (fixed)

Update conditional logic for editor_styles

Reported by: mikachan's profile mikachan Owned by: hellofromtonya's profile hellofromTonya
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)

Screenshot 2023-02-02 at 06.20.48.png (235.1 KB) - added by poena 17 months ago.
Before patch, the group has incorrect padding in the Site Editor (Twenty Twenty-Three)
Screenshot 2023-02-02 at 06.34.57.png (345.5 KB) - added by poena 17 months ago.
After patch, the theme's global padding settings (padding aware alignments) are respected.
tt2-before.png (546.6 KB) - added by hellofromTonya 16 months ago.
Twenty Twenty-Two (TT2) before PR 3916 is applied
tt2-after.png (550.4 KB) - added by hellofromTonya 16 months ago.
Twenty Twenty-Two (TT2) after PR 3916 is applied
tt3-before.png (551.0 KB) - added by hellofromTonya 16 months ago.
Twenty Twenty-Three (TT3) before PR 3916 is applied
tt3-after.png (556.0 KB) - added by hellofromTonya 16 months ago.
Twenty Twenty-Three (TT3) after PR 3916 is applied

Change History (22)

This ticket was mentioned in PR #3916 on WordPress/wordpress-develop by @mikachan.


17 months ago
#1

  • Keywords has-patch added

#2 @poena
17 months ago

  • Milestone changed from Awaiting Review to 6.2

@poena
17 months ago

Before patch, the group has incorrect padding in the Site Editor (Twenty Twenty-Three)

@poena
17 months ago

After patch, the theme's global padding settings (padding aware alignments) are respected.

#3 @poena
17 months 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*

Last edited 17 months ago by poena (previous) (diff)

#4 @mikachan
17 months 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.


16 months ago

#6 @costdev
16 months ago

  • Keywords gutenberg-merge added

#7 @sannevndrmeulen
16 months ago

Test Report

PR tested: https://github.com/WordPress/wordpress-develop/pull/3916

Steps to Reproduce or Test

  1. Activate Twenty Twenty-Three theme
  2. Enter the Editor
  3. Insert Separator block

Actual Results

When reproducing a bug/defect:

  • ❌ Error condition occurs.
  1. https://imgur.com/Qpdc371
  2. https://imgur.com/EnkeibO

When testing the bugfix patch:

  • ✅ Issue resolved with patch.
  1. https://imgur.com/RVbMCx4

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 @mikachan
16 months 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:


16 months 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 @hellofromTonya
16 months 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:


16 months 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 @mikachan
16 months 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?

@hellofromTonya
16 months ago

Twenty Twenty-Two (TT2) before PR 3916 is applied

@hellofromTonya
16 months ago

Twenty Twenty-Two (TT2) after PR 3916 is applied

@hellofromTonya
16 months ago

Twenty Twenty-Three (TT3) before PR 3916 is applied

@hellofromTonya
16 months ago

Twenty Twenty-Three (TT3) after PR 3916 is applied

#13 @hellofromTonya
16 months 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 @hellofromTonya
16 months 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.

#15 @hellofromTonya
16 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 55368:

Editor: Fix 'wp-block-library-theme' style enqueue conditions.

Fixes the conditions for when to enqueue the opinionated block styles (i.e. 'wp-block-library-theme' stylesheet):

  • the theme adds 'wp-block-styles' theme support;
  • and no editor styles are declared.

This resolves an issue with themes that do not add the 'wp-block-styles' theme support while not impacting themes that do.

Follow-up to [53419], [52069], [50761], [44157].

Props mikachan, costdev, glendaviesnz, hellofromTonya, jffng, mamaduka, ndiego, poena, sannevndrmeulen, scruffian.
Fixes #57561.

Note: See TracTickets for help on using tickets.