Make WordPress Core

Opened 5 years ago

Last modified 2 years ago

#47670 reviewing defect (bug)

RSS widget creates an accessibility problem when used more than once

Reported by: tpaw's profile tpaw Owned by: audrasjb's profile audrasjb
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Widgets Keywords: has-screenshots 2nd-opinion has-patch
Focuses: accessibility Cc:

Description

Please consider the following patch to improve accessibility.

Accessibility guidelines in WCAG's standard 2.4.4 Link Purpose requires that link text should provide a purpose or context for the link.

In the RSS widget, the link text for the link to the RSS feed itself is an image of the RSS icon; its alt text is "RSS" which programmatically determines the link text. This passes the referenced standard.

A problem occurs when multiple instances of the RSS widget are used. There are then multiple links with link text "RSS", each of which lead to different URLs. The text "RSS" then does not provide enough context for visitors to know what each "RSS" link leads to.

The solution is the make each RSS link text unique to each feed's title, thus providing the necessary context.

Simply prepending the title of the widget instance adds the necessary context to the link text without using any additional words which could has i18n issues.

In class-wp-widget-rss.php (lines 89-91 in 5.2.2):

From:

<?php
// lines 89-91 in wp-includes/widgets/class-wp-widget-rss.php (WP 5.2.2)

if ( $title ) {
        $title = '<a class="rsswidget" href="' . esc_url( $url ) . '"><img class="rss-widget-icon" style="border:0" width="14" height="14" src="' . esc_url( $icon ) . '" alt="RSS" /></a> <a class="rsswidget" href="' . esc_url( $link ) . '">' . esc_html( $title ) . '</a>';
}

To:

<?php
if ( $title ) {
        $title = '<a class="rsswidget" href="' . esc_url( $url ) . '"><img class="rss-widget-icon" style="border:0" width="14" height="14" src="' . esc_url( $icon ) . '" alt="' . esc_html( $title ) . ' RSS" /></a> <a class="rsswidget" href="' . esc_url( $link ) . '">' . esc_html( $title ) . '</a>';
}

No harm is done when only a single instance of the RSS widget is used because the RSS link text is simply more explicit.

Attachments (11)

47670.diff (966 bytes) - added by mukesh27 5 years ago.
Patch.
before-patch-feed-with-title.png (156.2 KB) - added by audrasjb 5 years ago.
Before 47670.1.diff: feed with title
before-patch-feed-without-title.png (160.4 KB) - added by audrasjb 5 years ago.
Before 47670.1.diff: feed without title
after-patch-feed-with-title.png (152.6 KB) - added by audrasjb 5 years ago.
After 47670.1.diff: feed with title
after-patch-feed-without-title.png (157.3 KB) - added by audrasjb 5 years ago.
After 47670.1.diff: feed without title
47670.1.diff (965 bytes) - added by audrasjb 5 years ago.
Remove double link in Widget RSS
Capture d’écran 2019-08-02 à 19.48.23.png (186.3 KB) - added by audrasjb 5 years ago.
Testing 47670.1.diff with another bundled theme
Twenty Seventeen.png (36.3 KB) - added by afercia 5 years ago.
Twenty Seventeen completely breaks
Twenty Nineteen.png (21.9 KB) - added by afercia 5 years ago.
Minor: Twenty Nineteen underline extends to the empty space
47670.tabindex.patch (5.4 KB) - added by sabernhardt 3 years ago.
47670.2.diff (5.3 KB) - added by sabernhardt 2 years ago.

Download all attachments as: .zip

Change History (36)

#1 @tpaw
5 years ago

  • Summary changed from RSS widget creates and accessibility problem when used more than once to RSS widget creates an accessibility problem when used more than once

#2 follow-up: @mukesh27
5 years ago

  • Keywords has-patch 2nd-opinion added

Hi @tpaw,

Welcome to WordPress Trac! Thanks for the ticket.

I think we have to remove that alt tag from RSS image because if anyone does not use feed title then image alt display space before RSS text, also RSS was not used any escaping function.

Remove alt text from the widget is a better solution.

@audrasjb can you check this?

@mukesh27
5 years ago

Patch.

#3 @audrasjb
5 years ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Yes added to my list!

#4 in reply to: ↑ 2 @tpaw
5 years ago

Replying to mukesh27:

I think we have to remove that alt tag from RSS image because if anyone does not use feed title then image alt display space before RSS text, also RSS was not used any escaping function.

Remove alt text from the widget is a better solution.

@audrasjb can you check this?

Thank you for reviewing this.

Unfortunately, removing the alt text means that one will then have an "empty" link, as it would effectively become an <a href="url"></a> in the accessibility DOM.

Links must have some sort of link text, whether it's regular text or alt text from an image, so your proposed solution will still violate WCAG accessibility guidelines.

By leaving the user-specified title in, at least there is a chance and ability for users to correctly adhere to WCAG. Without any link text, it's still basically the same problem of not meeting a11y guidelines.

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


5 years ago

#6 @afercia
5 years ago

Related: #46978.

@audrasjb
5 years ago

Before 47670.1.diff: feed with title

@audrasjb
5 years ago

Before 47670.1.diff: feed without title

@audrasjb
5 years ago

After 47670.1.diff: feed with title

@audrasjb
5 years ago

After 47670.1.diff: feed without title

@audrasjb
5 years ago

Remove double link in Widget RSS

#7 @audrasjb
5 years ago

  • Keywords has-screenshots added
  • Milestone changed from Awaiting Review to 5.3

In 47670.1.diff:

  • Use only one link for both image and title of the RSS feed
  • Use an empty alt attribute since we doesn't need it anymore

Ping @afercia for code review.

Also moving this ticket to 5.3 milestone.

@audrasjb
5 years ago

Testing 47670.1.diff with another bundled theme

#8 @audrasjb
5 years ago

  • Keywords 2nd-opinion removed

#9 @mukesh27
5 years ago

@audrasjb Is it possible to remove rsswidget class from the text as same class used in parent link?

$title = '<a class="rsswidget" href="' . esc_url( $url ) . '"><img class="rss-widget-icon" style="border:0" width="14" height="14" src="' . esc_url( $icon ) . '" alt="" /> <span class="rsswidget rsswidget-text">' . esc_html( $title ) . '</span></a>'; 

Replace to

$title = '<a class="rsswidget" href="' . esc_url( $url ) . '"><img class="rss-widget-icon" style="border:0" width="14" height="14" src="' . esc_url( $icon ) . '" alt="" /> <span class="rsswidget-text">' . esc_html( $title ) . '</span></a>'; 

#10 @afercia
5 years ago

For reference: recommended technique from the Web Content Accessibility Guidelines (WCAG):

Combining adjacent image and text links for the same resource
https://www.w3.org/WAI/WCAG21/Techniques/html/H2.html

#11 @afercia
5 years ago

  • Keywords needs-patch added; has-patch removed

I'm afraid it's a bit more complicated than expected. The two links can be styled by the themes. For example, Twenty Seventeen targets the first <a> child. Other themes may use any sort of CSS selectors including type (element) selectors so it's not just a matter of CSS classes: changes to the markup will likely break some themes. See screenshots below.

The only alternative option I can think of is removing the href from the icon link but that would make the icon non-clickable.

@afercia
5 years ago

Twenty Seventeen completely breaks

@afercia
5 years ago

Minor: Twenty Nineteen underline extends to the empty space

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


5 years ago

#13 @sabernhardt
5 years ago

It took me a while to realize that the two links have different URLs. The image link gives the feed URL itself, and the text link directs to the source's website.

So the alt text could be something like this to accommodate multiple feeds:

sprintf( esc_attr__( 'Access Feed URL for %s' ), $title )

Another option: use that new text string as an aria-label instead (with the translated "RSS" as alt text), so anyone who might turn images off would not see the full title twice.

$title = '<a class="rsswidget" href="' . esc_url( $url ) . '" aria-label="' . sprintf( esc_attr__( 'Access Feed URL for %s' ), $title ) . '"><img class="rss-widget-icon" style="border:0" width="14" height="14" src="' . esc_url( $icon ) . '" alt="' . esc_attr__( 'RSS' ) . '" /></a> <a class="rsswidget" href="' . esc_url( $link ) . '">' . esc_html( $title ) . '</a>';

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


5 years ago

#15 @audrasjb
5 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from 5.3 to Future Release

This ticket still needs some work and improvements. Also, it could be probably better to remove completely the RSS icon as it's theme territory.
Moving it to Future release.

#16 @sabernhardt
5 years ago

While I think there are cases where the feed URL is valuable to include, I realize that it makes little sense to have both links within the same heading tag. Also, anyone can add a separate link to the feed URL if they intend to make it available.

[temporary suggestion removed]

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

#17 @sabernhardt
3 years ago

I proposed a filter and/or widget options on #52224, which might be good enough to fix this.

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


3 years ago

#19 follow-up: @sabernhardt
3 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 5.9

After noticing that the admin toolbar's extra profile link currently uses a negative tabindex (since r19207), that attribute seems viable in this case, too.

The feed link also would need aria-hidden="true" to hide it (with NVDA).

To minimize the changes in 47670.tabindex.patch, I kept the direct feed URL (instead of matching the text link) plus the untranslated alt text.

If the aria-hidden and tabindex attributes make sense here, they could be added to the more elaborate proposal (#52224) as well. Both these tickets could close with one commit if that filter option suffices.

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

#20 in reply to: ↑ 19 @tpaw
3 years ago

Replying to sabernhardt:

After noticing that the admin toolbar's extra profile link currently uses a negative tabindex (since r19559), that attribute seems viable in this case, too.

The feed link also would need aria-hidden="true" to hide it (with NVDA).

I believe that adding both tabindex="-1" and aria-hidden="true" attribute on the RSS icon's wrapping a tag (not on the img itself) is an elegant and standards-based solution to remove the linked icon from the Accessibility DOM. This would solve the issue of duplicate link text leading to different URIs violating the WCAG 2.4.4 standard.

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


3 years ago

@sabernhardt
2 years ago

#22 @sabernhardt
2 years ago

The filter from #52224 is available to remove the icon link in classic widgets, and the block widgets already did not include it, so this error is avoidable. In case using the negative tabindex is still a good idea when the link remains in the widget, I updated the patch.

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


2 years ago

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


2 years ago

#25 @joedolson
2 years ago

  • Milestone changed from 5.9 to Future Release

If the two links had duplicate targets, this would be a viable path - hiding a duplicate link is reasonable. However, with two different links, we've now created a link that's only accessible to sighted mouse users, so it's a whole new problem.

I propose that instead we focus on improving the labeling of the RSS icon. We have the title of the feed from the feed source, so we could use an alt attribute in the format RSS Feed: feed source title; then each link has a unique title, as long as there aren't multiple widgets pointing to the same feed. (Which I'd consider outside the scope of what we can account for.)

Note: See TracTickets for help on using tickets.