Make WordPress Core

Opened 14 months ago

Closed 5 months ago

Last modified 5 months ago

#58443 closed defect (bug) (fixed)

Twenty Nineteen Button is having issue with line height.

Reported by: nidhidhandhukiya's profile nidhidhandhukiya Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.2.2
Component: Bundled Theme Keywords: has-testing-info has-patch has-screenshots
Focuses: Cc:

Description

Steps to reproduce the issue :-

  1. Activate Twenty Nineteen theme.
  2. Choose Button block.
  3. Add some text.
  4. Apply line height.

You can able to see when you apply line height it will add space on above the button.

I have attached video for better unnderstanding.
Video URL :- https://share.cleanshot.com/Sy247vzrHM6gWH69y5D1

Attachments (2)

58443.patch (2.2 KB) - added by nidhidhandhukiya 14 months ago.
58443.1.patch (1.0 KB) - added by nidhidhandhukiya 10 months ago.

Download all attachments as: .zip

Change History (45)

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


14 months ago

#2 @monzuralam
14 months ago

  • Keywords has-testing-info needs-testing has-patch needs-screenshots added

@nidhidhandhukiya, Thanks for your ticket.

We are looking for testing report. More than one test report is a helpful way to validate across different environments.

Last edited 14 months ago by monzuralam (previous) (diff)

#3 @monzuralam
14 months ago

  • Keywords needs-screenshots removed

#4 @monzuralam
14 months ago

  • Keywords has-screenshots added

#5 @shailu25
13 months ago

@monzuralam

Test Report for - https://core.trac.wordpress.org/attachment/ticket/58443/58443.patch Patch.

Environment:
============
WordPress - V6.2.2
Theme - Twenty Nineteen
Os: Windows
Browser: Chrome

Backend:
========
Before Patch Backend: https://prnt.sc/VoB3ZIbiqwZO
After Patch Backend : https://prnt.sc/FIOdY2k8XG-q

Frontend:
=========
Before Patch Frontend: https://prnt.sc/plXeZ-xUnuRt
After Patch Frontend : https://prnt.sc/UZacP-yMFEKm

https://core.trac.wordpress.org/attachment/ticket/58443/58443.patch patch is working fine.

Last edited 13 months ago by shailu25 (previous) (diff)

#6 @hrrarya
13 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/58443/58443.patch

Environment

  • OS: macOS 12.6.7
  • Web Server: Nginx
  • PHP: 7.4.33
  • WordPress: 6.3-beta3-56130-src
  • Browser: Brave - Version 1.52.129 Chromium: 114.0.5735.198 (Official Build) (x86_64)
  • Theme: Twenty Nineteen
  • Active Plugins: - No Active Plugins

Actual Results

  • ✅ Issue resolved with patch.

Supplemental Artifacts

Before patch backend - https://prnt.sc/MXqw7vmabIZb
After patch backend - https://prnt.sc/EzQ4gft0tHZZ

Before patch frontend - https://prnt.sc/M_UeEnkMFhZN
After patch frontend - https://prnt.sc/MYSuzufmPrSo

#7 @karmatosed
10 months ago

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

I am going to commit this as can see it fixed with the patch, thank you.

#8 @karmatosed
10 months ago

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

In 56580:

Twenty Nineteen: Fixes button line height.

Line height was broken for button block and adding spacing.

Props nidhidhandhukiya, shailu25.
Fixes #58443.

This ticket was mentioned in Slack in #core-committers by costdev. View the logs.


10 months ago

#10 @karmatosed
10 months ago

In 56581:

Twenty Ninteen: Revert changes to 'style-rtl.css'.

This revert changes to Twenty Nineteen 'style-rtl.css' file made in [56580] as it auto-generated during the build process.

Unprops karmatosed.
Props costdev.
See #58443.

#11 @karmatosed
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#12 @karmatosed
10 months ago

  • Owner karmatosed deleted
  • Status changed from reopened to assigned

On the basis of consulting regarding some failed tests this needs a little more of a solid fix at the SASS file level. Just fixing the CSS means that tests fail for example for the rtl files which shouldn't be changed.

@nidhidhandhukiya I wanted to give you the opportunity to work on this patch considering you originally did but happy to also tackle this if not.

cc @costdev and @spacedmonkey for those who aided diagnosing why the tests were not passing regarding this issue.

#13 @sabernhardt
10 months ago

  • Keywords commit removed

Using Switching a specific value to inherit was not the best way to fix custom line-height.
Correction: When the theme—or block library styles—specify the desired value on a parent element, then using inherit could be highly appropriate.

On the front end, all button-like elements have had a line-height of 1.2 since initial commit, and it needs to stay that value when users do not select their own. This ticket did not change the front-end style.css and the RTL was reverted, so the current state is still only wrong with a custom line-height.

The editor's value of 1.8 did not match the front, but removing the entire line can be better than changing to inherit.

Any Some of the customizable properties probably could be moved to the wp-block-button or wp-block-buttons element instead of the link.
Correction: The em-based font-size is one property that probably should not be moved to another element because that would change the size for anyone who has set a value in custom CSS or who is satisfied with the relative difference in size using the block setting.

If tests are still giving any errors, that might be related to the missing space after the colon in style-editor.scss.

Last edited 9 months ago by sabernhardt (previous) (diff)

#14 @sabernhardt
10 months ago

  • Milestone changed from Awaiting Review to 6.4

#15 @harshgajipara
10 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/58443/58443.1.patch

Environment

OS: Windows
PHP: 8.1.9
WordPress: 6.3.1
Browser: Chrome
Theme: Twenty Nineteen
Plugins: None activated

Actual Results:

Before Patch:
✅ In the editor and frontend, space showing above button when line height applied.
Backend: https://prnt.sc/lQl2hEgLCwi8
Frontend: https://prnt.sc/Jeo1j1zraYLL

After Patch
✅ In the editor and frontend, space removed above button when line height applied.
Backend: https://prnt.sc/WtJaVDtD0I7m
Frontend: https://prnt.sc/9CH_svNz2k2H

#16 @nicolefurlan
10 months ago

  • Keywords changes-requested added

@nidhidhandhukiya thank you for reporting and working on this! Would you be able to review #comment:13 and make changes to the patch?

This ticket was mentioned in PR #5429 on WordPress/wordpress-develop by fnpen.


10 months ago
#17

I moved line-height default definition from .wp-block-button__link to .wp-block-button, as a result, it allowed for overriding by style and block editor.

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

#18 @fnpen
10 months ago

@nicolefurlan, I made changes according to the #comment:13.

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


10 months ago
#19

I moved line-height default definition from .wp-block-buttonlink to .wp-block-button, as a result, it allowed for overriding by style and block editor.

Trac ticket: [)](https://core.trac.wordpress.org/ticket/58443 [core.trac.wordpress.org/ticket/58443]

#20 @oglekler
10 months ago

  • Keywords needs-testing added; changes-requested removed

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


10 months ago

#22 @nicolefurlan
10 months ago

Pinging previous testers to see if they can test the updated patch :)

cc @shailu25, @hrrarya, @harshgajipara

#23 @nicolefurlan
9 months ago

@poena pinging you to see if you could rally come folks to test this one out so it could potentially make it into RC1 next week :)

#24 @shailu25
9 months ago

Test Report

Patch Tested: https://github.com/WordPress/wordpress-develop/pull/5431

Environment:

WordPress - 6.4 beta2
OS - Windows
Browser - Chrome
Theme: Twenty Nineteen
PHP - 8.1.2
Active Plugin - None

Actual Results:

  • After applying the patch line height issue will be resolved.✅

Screenshots:

Back-End: https://prnt.sc/ZDqvaBTbW_Lr

Front-End:
Before Patch Frontend : https://prnt.sc/VIB-qf5LC_8b
After Patch Frontend : https://prnt.sc/qOjDtEb6rO1X

Last edited 9 months ago by shailu25 (previous) (diff)

@sabernhardt commented on PR #5431:


9 months ago
#25

It is very late in this release cycle for revising style-editor.scss too, but could you set line-height: $font__line-height-heading; on .wp-block-button and remove the line-height from .wp-block-button__link there for consistency?

#26 @sabernhardt
9 months ago

I needed to edit two statements in #comment:13 because they were not entirely accurate.

PR 5431 is an improvement but not thorough. It adds support for the line height setting on the front end when applied to individual Button blocks, yet not when trying to set a line height on the parent Buttons block. Whether that is good enough or should be applied to .wp-block-buttons instead, I would like at least the same change in the theme's editor styles.

For quicker testing, you could copy and paste a set of Button blocks. The first set focuses on the default typography styles and custom line height, and the second set includes several other custom typography options (Twenty Nineteen still would not support a custom font weight or text decoration).

Version 0, edited 9 months ago by sabernhardt (next)

#27 @nicolefurlan
9 months ago

  • Keywords changes-requested added
  • Milestone changed from 6.4 to 6.5

@fnpen would you be able to update your PR?

I'm going to bump this to 6.5 since this patch needs changes and it will need further testing.

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


5 months ago
#28

Moves line-height to Buttons container and removes the duplicate outline

Trac 58443

#29 @poena
5 months ago

  • Keywords changes-requested removed

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6069.

Environment

  • OS: macOS 14.2.1
  • Web Server: Nginx
  • PHP: 8.1.23
  • WordPress: 6.5-alpha-56966-src
  • Browser: Chrome ersion 121.0.6167.160

Actual Results

  • ✅ Issue resolved with patch.

Additional Notes

  • The default line height is correct in both the editor and front.
  • The highlight can be adjusted both on the buttons block and button block:

For example the line height on the buttons can be 2.5 while an individual button inside it can have a line height set to 0. Both line heights are correct in the editor and front.

#30 @huzaifaalmesbah
5 months ago

Test Report

Patch tested: PR 6069

Environment

  • WordPress: 6.5-alpha-56966-src
  • PHP: 8.2.15
  • Server: nginx/1.25.3
  • Database: mysqli (Server: 8.0.36 / Client: mysqlnd 8.2.15)
  • Browser: Chrome 121.0.0.0
  • OS: macOS
  • Theme: Twenty Nineteen 2.7
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.1.0

Actual Results

Line height issue resolved with patch ✅

#31 @poena
5 months ago

  • Keywords commit added; needs-testing removed

@audrasjb commented on PR #6069:


5 months ago
#32

I restarted the jobs, since the Theme ZIP Build test for Twenty Nineteen was failing

@audrasjb commented on PR #6069:


5 months ago
#33

This test is still failing

#34 @audrasjb
5 months ago

@poena @sabernhardt looks like the Theme ZIP Build test for Twenty Nineteen is failing

@poena commented on PR #6069:


5 months ago
#35

Do I understand correctly from the error report that the rtl css files need to be re-built from the scss files and added to the PR?

@sabernhardt commented on PR #6069:


5 months ago
#36

Yes, the RTL stylesheet needed to be built.

#37 @SergeyBiryukov
5 months ago

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

In 57599:

Twenty Nineteen: Correct line height for Button block.

This commit moves line-height to the Buttons container and removes the duplicate outline property.

Props sabernhardt, nidhidhandhukiya, monzuralam, shailu25, hrrarya, karmatosed, harshgajipara, nicolefurlan, fnpen, oglekler, poena, huzaifaalmesbah, audrasjb.
Fixes #58443.

@SergeyBiryukov commented on PR #5431:


5 months ago
#38

Thanks for the PR! Merged in r57599.

@SergeyBiryukov commented on PR #6069:


5 months ago
#39

Thanks for the PR! Merged in r57599.

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


5 months ago
#40

Restores transition property to a single line, as its was before r57599.

Trac 58443

#41 @sabernhardt
5 months ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

reopening to fix tests

#42 @SergeyBiryukov
5 months ago

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

In 57604:

Twenty Nineteen: Restore transition property to a single line.

Follow-up to [57599].

Props sabernhardt.
Fixes #58443.

@SergeyBiryukov commented on PR #6095:


5 months ago
#43

Thanks for the PR! Merged in r57604.

Note: See TracTickets for help on using tickets.