Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#54250 closed defect (bug) (fixed)

Twenty Twenty One: Editor Buttons margins incompatible with gap

Reported by: stacimc's profile stacimc Owned by: audrasjb's profile audrasjb
Milestone: 5.9.1 Priority: normal
Severity: normal Version: 5.9
Component: Bundled Theme Keywords: has-testing-info has-screenshots has-patch commit fixed-major
Focuses: Cc:

Description

Twenty Twenty One has a few style rules in the editor that will causes issues when the Buttons block is refactored to use gap to control vertical spacing in between buttons, rather than using margins. (https://github.com/WordPress/gutenberg/pull/34546)

It enforces a 0px margin on the Buttons container, and applies additional margin to the first and last children of the Buttons block. With margins removed from individual buttons, this causes:

  • Insufficient space between Buttons container and adjacent blocks
  • The first button in a Buttons group is vertically offset from its siblings
[data-block].wp-block-buttons {
	margin-top: 0;
	margin-bottom: 0;
}

[data-block].wp-block-buttons .wp-block-button:first-child {
	margin-top: var(--global--spacing-vertical);
}

[data-block].wp-block-buttons .wp-block-button:last-child {
	margin-bottom: var(--global--spacing-vertical);
}

Proposed Solution:

  • Remove the rule setting 0px margin on the container, allowing it to fall back to default spacing
  • Remove the rules setting extra margin-top/bottom on first/last buttons as it is no longer necessary

Attachments (2)

Screen Shot 2021-10-12 at 12.14.18 PM.png (64.7 KB) - added by stacimc 3 years ago.
Bug: There is no margin in between Buttons containers, and the first button in each group is vertically offset
54250.patch (426 bytes) - added by nidhidhandhukiya 2 years ago.
This issue is happen only on the editor side it is working fine in user side. I have the solution of this issue.

Download all attachments as: .zip

Change History (24)

@stacimc
3 years ago

Bug: There is no margin in between Buttons containers, and the first button in each group is vertically offset

#1 @Boniu91
3 years ago

  • Keywords needs-testing-info added

Hello @stacimc thank you for creating the ticket.

Could you provide us with the exact step by step instructions that are used to reproduce the issue?

It'll be easier for the Test team to follow exactly the same steps as you do.

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 years ago

#3 @aristath
3 years ago

Steps to reproduce and test

  1. Use the twentytwentyone theme.
  2. In style.css, add this:
    div {
            background-image: url("./assets/images/Daffodils.jpg") !important;
    }
    
    Check and verify that the image is everywhere.
  3. In functions.php add the following:
    add_filter( 'should_load_separate_core_block_assets', '__return_true' );
    add_filter( 'styles_inline_size_limit', function( $size ) {
            return 1000000; // Extreme, but this is just for the purpose of a test.
    });
    add_action( 'wp_enqueue_scripts', function() {
            wp_style_add_data( 'twenty-twenty-one-style', 'path', get_theme_file_path( 'style.css' ) );
    }, 20 );
    

Without the patch: the image no longer shows as the background.
With the patch: the image should show.

#4 @Boniu91
3 years ago

  • Keywords has-testing-info added; needs-testing-info removed

#5 @stacimc
3 years ago

Since writing this ticket, the Buttons gap support was merged (https://github.com/WordPress/gutenberg/pull/34546). It refactors Buttons to use gap for vertical spacing rather than margins, and forces all margins on Button blocks to 0. This causes the described regressions to vertical spacing.

To test:

  1. Use the Twenty Twenty One theme
  2. Insert a Buttons block into a new post, and add several buttons to it, enough that they take up multiple rows.
  3. Insert a second Buttons block immediately afterward, and add several buttons to it.
  4. Observe that there is no margin in between the two Buttons containers.
  5. Inspect the first Button in either group and observe this CSS rule:
    [data-block].wp-block-buttons .wp-block-button:first-child {
    	margin-top: var(--global--spacing-vertical);
    }
    

The first-child (and corresponding last-child) rule do not cause a visual bug because they are overridden by the block, but these styles could be deleted. The lack of margin between Buttons containers does cause a visual issue.

#6 @audrasjb
2 years ago

  • Milestone changed from Awaiting Review to 5.9

Moving for 5.9 consideration.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#8 @audrasjb
2 years ago

  • Keywords needs-patch added
  • Milestone changed from 5.9 to 5.9.1

Moving to 5.9.1 as WP 5.9 RC 1 is today and this still needs a patch and broader testing.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#10 @audrasjb
2 years ago

  • Keywords needs-testing added

Now that 5.9 was released, the next step is to see if it’s still possible to reproduce the issue, and to provide a few screenshots/steps to help reproduce the issue.

This ticket was mentioned in Slack in #core by ironprogrammer. View the logs.


2 years ago

#12 @ironprogrammer
2 years ago

  • Keywords has-screenshots added

I was able to reproduce this issue with these updated instructions.

tl;dr -- What to Check When Reproducing the Issue

It's important that the Buttons blocks be vertically adjacent to one another. To verify this, open List View to check the nesting of the Buttons blocks. If the blocks are separated by any other block type, or within different Groups, use the List View to drag the Buttons blocks so that they are adjacent. Refer to the screenshots below for comparison.

⚠️ Note that the Gap setting this ticket originally refers to appears to have been removed from 5.9.

To reproduce:

  1. Install WordPress 5.9.
  2. Activate Twenty Twenty-One theme.
  3. Add a new Post.
  4. Enter a title and press Return, or just advance to the first content block location (press Tab or place the cursor in the "Type / to choose a block" field).
  5. Type /buttons + Return to insert a Buttons block, or use the "plus" button to the right of the insert block area and select "Buttons".
  6. Insert multiple buttons into this block, for instance by typing "button" + Return repeatedly. Insert enough buttons that they wrap into the next line.
  7. Use the Down cursor to advance to the next block insertion area, and add another Buttons block by typing /buttons + Return. Alternatively, click the "plus" button to the right and below the current block, and select another Buttons block.
  8. Insert a few buttons into this block.
  9. Note that there is no margin between the first Buttons block and the one immediately below it.

Screenshots

WordPress 5.9 (Repro)

https://cldup.com/9mUDxGTCuj.png

WordPress 5.8.3 (Regression)

https://cldup.com/iYEB0N4SY4.png

@nidhidhandhukiya
2 years ago

This issue is happen only on the editor side it is working fine in user side. I have the solution of this issue.

This ticket was mentioned in PR #2268 on WordPress/wordpress-develop by ironprogrammer.


2 years ago
#13

  • Keywords has-patch added; needs-patch removed

Removes overrides that prevent site editor updates in 5.9 from controlling vertical margins via var(--global--spacing-vertical).

Trac ticket: https://core.trac.wordpress.org/ticket/54250

#14 @ironprogrammer
2 years ago

Thank you, @nidhidhandhukiya, for pointing out the margin.

Backtracking from that selector group reveals that the spacing discrepancy relates to margins between Buttons blocks themselves, rather than for individual buttons. I've opened PR 2268 to address this in the Twenty Twenty-One (TT1) theme.

This update makes TT1 behave like previous non-FSE themes, where control over editor block margins is controlled by bundled editor styles. The fix falls in line with @stacimc's proposed solution, though is technically separate from blockGap support. Visually, the margins are consistent with legacy themes like Twenty Twenty, Twenty Nineteen, etc.

Twenty Twenty-One with Fix Applied

https://cldup.com/YW_bxXvn1G.png

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


2 years ago

#17 @Boniu91
2 years ago

Working as expected for me.

Test Report

Env

  • WordPress 5.9
  • Chrome 98.0.4758.82
  • Windows 10
  • Theme: Twenty Twenty One

Steps to test

  1. Add more than 5 buttons to one block
  2. Add more than 5 buttons to the block below the first one
  3. The view in the block editor is exactly the same as the view on the frontend
  4. Checked different screen sizes

Screenshot:
https://screenrec.com/share/Ev9ULJFSK8

Last edited 2 years ago by Boniu91 (previous) (diff)

#18 @audrasjb
2 years ago

  • Keywords commit added; needs-testing removed
  • Owner set to audrasjb
  • Status changed from new to assigned

Thanks for testing!
Self-assigning for commit.

#19 @audrasjb
2 years ago

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

In 52726:

Twenty Twenty-One: Allow editor styles to control block margins.

This change removes some overrides that prevented site editor updates in 5.9 from controlling vertical margins via var(--global--spacing-vertical).

Props stacimc, Boniu91, aristath, ironprogrammer, nidhidhandhukiya.
Fixes #54250.

#21 @audrasjb
2 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#22 @audrasjb
2 years ago

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

In 52727:

Twenty Twenty-One: Allow editor styles to control block margins.

This change removes some overrides that prevented site editor updates in 5.9 from controlling vertical margins via var(--global--spacing-vertical).

Props stacimc, Boniu91, aristath, ironprogrammer, nidhidhandhukiya.
Merges [52726] to the 5.9 branch.
Fixes #54250.

Note: See TracTickets for help on using tickets.