WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#37382 closed defect (bug) (fixed)

Shiny Updates: error messages with HTML and wp.a11y.speak()

Reported by: afercia Owned by: afercia
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: Plugins Keywords: has-screenshots shiny-updates has-patch commit
Focuses: accessibility, javascript Cc:

Description

With the new Shiny Updates, some error messages dispatched to the aria-live regions may contain HTML, e.g. links. For example:

Installation failed: An unexpected error occurred. Something may be wrong with WordPress.org or this server&#8217;s configuration. If you continue to have problems, please try the <a href="https://wordpress.org/support/">support forums</a>.

Though the message gets inserted in the aria live region as a text string, and thus it should be safe, having screen readers read out "less than ... href equal http slash slash etc. ... greater than support forums ..." or something like that is far from ideal:

https://cldup.com/5sUQsA3p9B.jpg

Ideally, any HTML should be stripped out from the message string as already done in a couple of cases in updates.js for example $( '<p />' ).html( message ).text().
See also #35191.

Side note: I'm not so sure error messages returned in a response should contain HTML in the first place. I can understand that, historically, they were meant to be injected as HTML, for example in the notices on top of the screen. But now that these messages are going to be used in different places and for different purposes maybe it's time to re-think them since there's the need to distinguish when a message is "plain text" or "rich text with HTML" (or maybe provide both versions in the response?).

Attachments (2)

37382.diff (2.2 KB) - added by adamsilverstein 4 years ago.
37382.2.diff (546 bytes) - added by adamsilverstein 4 years ago.

Download all attachments as: .zip

Change History (13)

#1 follow-up: @swissspidy
4 years ago

Yay, another Shiny Updates bug! :-)

I think we should strip the HTML for the aria-live region and think about dealing with those scenarios differently in a future release.

The An unexpected error occurred. … error string is part of a number of files and often gets displayed using trigger_error(), but sometimes is passed as a WP_Error object. We need to better understand why it currently is like that.

#2 in reply to: ↑ 1 @afercia
4 years ago

Replying to swissspidy:

I think we should strip the HTML for the aria-live region and think about dealing with those scenarios differently in a future release.

+1

#3 follow-up: @adamsilverstein
4 years ago

@afercia I started working on a patch replacing all out errorMessage passing to wp.speak to convert HTML to text as you suggested.

I also thought that perhaps this functionality should be in wp.speak itself - would there ever be a use case where passing HTML into wp.speak would be useful? Perhaps for speaking out a code snippet?

#4 @adamsilverstein
4 years ago

In 37382.diff: ensure all errorMessage use for wp.speak is stripped of HTML.

37382.2.diff is an alternate approach that strips HTML from any string passed to wp.wpeak

#5 @adamsilverstein
4 years ago

  • Keywords 2nd-opinion added; needs-patch removed

#6 in reply to: ↑ 3 ; follow-up: @afercia
4 years ago

Replying to adamsilverstein:
@adamsilverstein hi. Thanks for having a look into this.

I also thought that perhaps this functionality should be in wp.speak itself

Yep, I've considered this option, and while it would resolve the technical issue in an efficient way, it wouldn't guarantee that the messages passed to wp.speak.a11y() should preferably be short and meaningful. Ideally, messages for wp.speak.a11y() should be carefully crafted case by case and we shouldn't assume already existing messages fit this specific use case.

would there ever be a use case where passing HTML into wp.speak would be useful? Perhaps for speaking out a code snippet?

Hm difficult to say but probably very unlikely.

#7 in reply to: ↑ 6 ; follow-up: @swissspidy
4 years ago

@adamsilverstein hi. Thanks for having a look into this.

I also thought that perhaps this functionality should be in wp.speak itself

Yep, I've considered this option, and while it would resolve the technical issue in an efficient way, it wouldn't guarantee that the messages passed to wp.speak.a11y() should preferably be short and meaningful. Ideally, messages for wp.speak.a11y() should be carefully crafted case by case and we shouldn't assume already existing messages fit this specific use case.

True, but definitely something for a new ticket :-)

would there ever be a use case where passing HTML into wp.speak would be useful? Perhaps for speaking out a code snippet?

Hm difficult to say but probably very unlikely.

That would probably be against the "short and meaningful" rule of thumb. Therefore 37382.2.diff seems preferable.

#8 in reply to: ↑ 7 @afercia
4 years ago

  • Keywords has-patch commit added; 2nd-opinion removed

Replying to swissspidy:

37382.2.diff seems preferable.

Makes sense :) and seems to be the most efficient solution since it solves the issue also in other admin screens. See for example the Import screen in the screenshot below, no more HTML in the aria live message. Notice also the multiple... notices on top of the page, probably something to consider in a separate ticket.

https://cldup.com/hBe2Y9qTo1.png

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


4 years ago

#10 @afercia
4 years ago

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

#11 @afercia
4 years ago

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

In 38115:

Accessibility: Ensure only text is sent to aria-live messages.

While messages passed to wp.speak.a11y() should preferably be meaningful,
short, and carefully crafted case by case, this will ensure any HTML tags will
be stripped out from the message string.

Props adamsilverstein.
Fixes #37382.

Note: See TracTickets for help on using tickets.