Make WordPress Core

Opened 5 years ago

Closed 15 months ago

Last modified 15 months ago

#45925 closed enhancement (fixed)

Twenty Nineteen: Consider a custom hover/underline style

Reported by: allancole's profile allancole Owned by: danfarrow's profile danfarrow
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: good-first-bug has-patch
Focuses: accessibility, css Cc:

Description

Originally reported by @afercia in the Twenty Nineteen GitHub repo:

The underline style on links using the Hoefler Text font is very thin in the Firefox browser and almost not readable, compared to other browsers (see attachments).

We should consider replacing the underline with a custom border that can be more consistent across browsers and font stacks.

More details here: https://github.com/WordPress/twentynineteen/pull/186#issuecomment-431418217

Original ticket here: https://github.com/WordPress/twentynineteen/issues/278

Attachments (6)

Safari.png (222.0 KB) - added by allancole 5 years ago.
Hoefler Text underline in Safari
Firefox.png (262.4 KB) - added by allancole 5 years ago.
Hoefler Text underline in Firefox
css-text-decoration.45925.diff (6.7 KB) - added by danfarrow 3 years ago.
Add text-decoration-thickness: 2px and text-decoration-skip-ink: auto to declaration blocks containing text-decoration: underline. Rebuild style-rtl.css and style.css
css-text-decoration.45925.3.diff (6.7 KB) - added by danfarrow 3 years ago.
Add text-decoration-thickness: 2px and text-decoration-skip-ink: auto to declaration blocks containing text-decoration: underline. Rebuild style-rtl.css. Rebuild style.css, retaining the block-patterns tag
css-text-decoration.45925.4.diff (6.0 KB) - added by danfarrow 3 years ago.
[Previous diff was not fixed - this one is the fixed diff!] Add text-decoration-thickness: 2px and text-decoration-skip-ink: auto to declaration blocks containing text-decoration: underline. Rebuild style-rtl.css. Rebuild style.css, retaining the block-patterns tag
45925.patch (5.5 KB) - added by multidots1896 16 months ago.
Added patch

Download all attachments as: .zip

Change History (44)

@allancole
5 years ago

Hoefler Text underline in Safari

@allancole
5 years ago

Hoefler Text underline in Firefox

#1 @desrosj
5 years ago

  • Milestone changed from Awaiting Review to Future Release

#2 @sabernhardt
3 years ago

  • Focuses accessibility css added

I think it's probably too late to switch to a border or similar styling.

However, both Firefox and Safari added support for text-decoration-thickness last year. Perhaps that could be set to 2 pixels (or an em-based measurement) for more consistency between fonts.

Maybe we also could include text-decoration-skip-ink for any browsers that support the property.

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


3 years ago

#4 @sabernhardt
3 years ago

  • Keywords good-first-bug added
  • Owner set to danfarrow
  • Status changed from new to assigned

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


3 years ago

@danfarrow
3 years ago

Add text-decoration-thickness: 2px and text-decoration-skip-ink: auto to declaration blocks containing text-decoration: underline. Rebuild style-rtl.css and style.css

#6 @danfarrow
3 years ago

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

While looking into this ticket I soon realised that this is an issue on Mac Firefox only.

I have uploaded a patch (my first!) but as I don't have access to a Mac I'm unable to test it.

Also I rebuilt style.css & style-rtl.css as part of the patch. I'm not sure if that was the right approach - if anybody could advise I'd be very grateful.

This ticket was mentioned in Slack in #accessibility by danfarrow. View the logs.


3 years ago

#8 @sabernhardt
3 years ago

@danfarrow Thanks for the patch!

As @desrosj noted on Slack, committers can compile the stylesheets if they are not included within the patch. Including those still can help people test patches, though.

I haven't tested (on Mac), but I see the compiled style.css removed "block-patterns" from the Tags list.

#9 @danfarrow
3 years ago

@sabernhardt That's very helpful, thank you!

I didn't spot the tag change (although it is obvious now you mention it), maybe my local repo needs updating?

I'll figure it out tomorrow

@danfarrow
3 years ago

Add text-decoration-thickness: 2px and text-decoration-skip-ink: auto to declaration blocks containing text-decoration: underline. Rebuild style-rtl.css. Rebuild style.css, retaining the block-patterns tag

@danfarrow
3 years ago

[Previous diff was not fixed - this one is the fixed diff!] Add text-decoration-thickness: 2px and text-decoration-skip-ink: auto to declaration blocks containing text-decoration: underline. Rebuild style-rtl.css. Rebuild style.css, retaining the block-patterns tag

#10 @danfarrow
3 years ago

@sabernhardt It appears that the block-patterns tag is missing from the repo's style.scss so I will create have created a new ticket for that #52159

I have manually removed that section from the diff and have uploaded a new version ready for testing - if anybody on a Mac is able to have a look that would be great, thank you!

Last edited 3 years ago by danfarrow (previous) (diff)

#11 @sabernhardt
2 years ago

  • Milestone changed from Future Release to 5.9

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 years ago

#13 @sabernhardt
2 years ago

  • Keywords needs-refresh added; needs-testing removed

Somehow the RTL stylesheet has a thickness of 20px instead of 2.

#14 @danfarrow
2 years ago

Oh whoops somehow I failed to notice that, sorry! I will try again

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


2 years ago

#16 @audrasjb
2 years ago

  • Milestone changed from 5.9 to Future Release

Moving to Future release pending a refreshed patch.

Reminder: we simply need to replace the 20px thickness to 2px.

This ticket was mentioned in PR #2309 on WordPress/wordpress-develop by Neychok.


22 months ago
#17

  • Keywords needs-refresh removed

The original diff submitted by @danfarrow did not work for me, so I recreated the diff while fixing the 20px in style-rtl.

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

#18 @neychok
22 months ago

Hey @danfarrow , your diff did not work for me, so I just applied the changes again and created a PR while also changing the 20px to 2px in style-rtl.css

#19 @danfarrow
22 months ago

  • Keywords needs-testing added

@neychok Wonderful, thank you! Where did those last 3 months go?!

#20 @audrasjb
22 months ago

Thanks @neychok! I added a small change request in your PR :)

#21 @neychok
22 months ago

Thank you @audrasjb for the review, I did the change you requested

#22 @sabernhardt
17 months ago

  • Milestone changed from Future Release to 6.1

The PR is updated (Thanks!). When testing, check the focus style in the entry content area, comments section, the calendar widget, and another place such as the top navigation.

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

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


17 months ago

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


17 months ago

@multidots1896
16 months ago

Added patch

#25 follow-up: @audrasjb
16 months ago

@multidots1896 thanks for the new patch, however could you please also explain why do you think it would be better to use your proposal instead of the pull request proposed a few months ago? The implementation is slightly different, but it's hard to understand the purpose of this new patch while we already have a PR for this issue. Thanks :-)

#26 in reply to: ↑ 25 @multidots1896
16 months ago

Hi @audrasjb

Yes, I have alternate solution

text-underline-offset: auto;

is different property and it is usable. but

text-decoration-thickness: 2px;


is remain same in your PR.so that's why i have applied patch.

Thanks

Last edited 16 months ago by multidots1896 (previous) (diff)

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


16 months ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


16 months ago

This ticket was mentioned in Slack in #accessibility by chaion07. View the logs.


15 months ago

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


15 months ago

#31 @sabernhardt
15 months ago

Perhaps we should simplify this and only commit the change common to both patches, the text-decoration-thickness property.

During an Accessibility bug scrub, @joedolson said "I don't see how setting a property to the default value fixes anything" in regard to text-underline-offset: auto in 45925.patch. However, auto is also the default value for text-decoration-skip-ink. Setting that might override either none or all if someone customizes the skip-ink style (in a browser that supports all).

#32 @audrasjb
15 months ago

@sabernhardt so if I understand correctly, the ideal patch would be https://github.com/WordPress/wordpress-develop/pull/2309/files, without text-decoration-skip-ink: auto;, right?

#33 @maartenj
15 months ago

Test Report

This report validates that the indicated patch addresses the issue.

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

Environment

  • OS: macOS 12.6
  • Web Server: Nginx
  • PHP: 7.4.30
  • WordPress: 6.1-alpha-53344-src
  • Browser: Firefox 104.0.2
  • Theme: Twenty Nineteen

Actual Results

  • ✅ Issue resolved with patch.

#34 @sabernhardt
15 months ago

Yes, I think removing text-decoration-skip-ink from PR 2309 would improve it.

#35 @neychok
15 months ago

Should I go ahead and update my PR by removing text-decoration-skip-ink?

#36 @audrasjb
15 months ago

  • Keywords needs-testing removed

@sabernhardt thanks!
@neychok I already updated your PR. Waiting for unit tests to pass, then I'll commit the changeset.

#37 @audrasjb
15 months ago

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

In 54171:

Twenty Nineteen: Define underline thickness for links.

This changeset adds text-decoration-thickness: 2px; to all underlined link to ensure that underline thickness stay consistent across browsers. This fixes an issue where the underline style on links using the Hoefler Text font was too thin in Firefox.

Props allancole, sabernhardt, danfarrow, audrasjb, neychok, multidots1896, maartenj.
Fixes #45925.

Note: See TracTickets for help on using tickets.