Make WordPress Core

Opened 6 years ago

Closed 20 months ago

Last modified 20 months ago

#47303 closed defect (bug) (fixed)

Consider removing extra trailing spaces from links with the `dashicons-external` icon

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: audrasjb's profile audrasjb
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: I18N Keywords: good-first-bug has-screenshots has-patch needs-testing commit
Focuses: Cc:

Description (last modified by SergeyBiryukov)

The handbook link added in [45170] has an extra space before the screen reader text and the dashicons-external icon:

The WordPress Hosting Team maintains a list of those modules, both recommended and required, in <a href=..." ...>the team handbook <span class="screen-reader-text">(opens in a new tab)</span><span class="dashicons dashicons-external" aria-hidden="true"></span></a>.

In #47300 it was brought up that the screen reader text is a hidden span, so it should NOT be preceded by any space.

Looking at other strings with the dashicons-external icon, they all have the space before the screen reader text and the icon:

  • Learn more about updating PHP
  • Read more about what WordPress requires to run.
  • Get help resolving this issue.
  • Read about debugging in WordPress.
  • Read more about why you should use HTTPS

Same for the strings added earlier in wp_dashboard_events_news(), wp_dashboard_php_nag(), and wp_direct_php_update_button():

  • Meetups
  • WordCamps
  • News
  • Learn more about updating PHP
  • Update PHP

Some of these links don't have an underline and the extra space looks OK (to me it actually looks better with the space before the icon, as is). Others, however, do have an underline and the extra space looks a bit weird.

For the links with an underline, we should either remove the trailing spaces or (preferably) split them so that the icon is linked separately.

Attachments (7)

link space underlined.png (109.5 KB) - added by afercia 6 years ago.
link in additional css .png (32.1 KB) - added by afercia 6 years ago.
Example in the Customizer (Additional CSS panel) where the space is within the visually hidden text. It just uses a wrong icon, but that's a separate issue.
47303.diff (0 bytes) - added by isabel_brison 6 years ago.
47303.2.diff (8.4 KB) - added by isabel_brison 6 years ago.
47303.3.diff (8.4 KB) - added by isabel_brison 5 years ago.
47303.4.diff (9.8 KB) - added by sabernhardt 21 months ago.
47303-fixed-white-spaces.png (39.5 KB) - added by oglekler 20 months ago.

Download all attachments as: .zip

Change History (29)

#1 @SergeyBiryukov
6 years ago

  • Description modified (diff)

#2 @desrosj
6 years ago

  • Keywords needs-patch good-first-bug added

#3 @afercia
6 years ago

  • Keywords has-screenshots added

I was looking at this a few days ago (see also screenshot below) because the "underlined space" doesn't look so pretty.

Ideally, the actual printed out text should have a space like if it was normal text. This way, screen readers can read it out respecting the separation between words. For example, this text without the space:

Read more about why you should use HTTPS(opens in a new tab)

Wouldn't be ideal, while this other one:

Read more about why you should use HTTPS (opens in a new tab)

is announced in an appropriate way. Seems to me the simplest option would be using a space within the visually hidden text. For example, this is what the bundled themes generally do:

__( 'Continue reading<span class="screen-reader-text"> "%s"</span>', 'twentynineteen' )

Then, the external dashicon could use some left margin.

@afercia
6 years ago

Example in the Customizer (Additional CSS panel) where the space is within the visually hidden text. It just uses a wrong icon, but that's a separate issue.

@isabel_brison
6 years ago

#4 @isabel_brison
6 years ago

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

First time contributor here! I have uploaded a patch for this issue taking into account @afercia's recommendations. I also noticed the icon was not quite aligned with the text so added an extra margin to align it. Would be great to get some feedback on it, code and design-wise :-)

#5 @talldanwp
6 years ago

  • Owner set to isabel_brison
  • Status changed from new to assigned

This ticket was mentioned in Slack in #design by estelaris. View the logs.


5 years ago

#7 @mukesh27
5 years ago

  • Keywords needs-design-feedback removed

Hi there, Remove the "needs-design-feedback" as decide in today's design weekly triage.

#8 @mukesh27
5 years ago

  • Keywords needs-refresh added

Hi there!

The patch needs an update so can @isabel_brison you please update it?

On small suggestion from my end.

Use -1px top margin instead of -2px.

.screen-reader-text + .dashicons-external {
   margin-top: -1px;
   margin-left: 4px;
}

#9 @mukesh27
5 years ago

  • Keywords needs-refresh removed

#10 @isabel_brison
5 years ago

Patch updated!

#11 @sabernhardt
22 months ago

  • Keywords needs-refresh added; needs-testing removed

#12 @sabernhardt
22 months ago

  • Milestone changed from Awaiting Review to 6.3

#13 @sabernhardt
21 months ago

  • Keywords needs-refresh removed

#14 @oglekler
21 months ago

  • Keywords needs-testing added

Needs testing with checking that patch is covers all places where needed.

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


21 months ago

#16 @oglekler
20 months ago

I tested the patch with current 6.3 Beta 1 version, and it works as expected, and I was unable to find any other place when this change is actually needed.

#17 @oglekler
20 months ago

  • Keywords commit added

I am marking this for review to commit.

#18 @audrasjb
20 months ago

  • Owner changed from isabel_brison to audrasjb

Self assigning for commit.

#20 @audrasjb
20 months ago

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

In 56211:

I18n: Improve the use of dashicons-external icon for external links.

This changeset modifies how the dashicons-external icon is used in external links by editing its styles and adding a space between the text and the icon
for better accessibility.

Props SergeyBiryukov, afercia, isabel_brison, mukesh27, sabernhardt, oglekler, audrasjb.
Fixes #47303.

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


20 months ago

Note: See TracTickets for help on using tickets.