Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#47147 closed defect (bug) (fixed)

Status message not exposed to assistive technologies (in the Image Editor)

Reported by: anevins's profile anevins Owned by: afercia's profile afercia
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-screenshots wpcampus-report has-patch commit has-dev-note
Focuses: ui, accessibility Cc:

Description (last modified by afercia)

Moved from the WPCampus accessibility report issues on GitHub, see: https://github.com/WordPress/gutenberg/issues/15293

  • Severity:
    • Medium
  • Affected Populations:
    • Blind
    • Low-Vision
    • Cognitively Impaired
  • Platform(s):
    • Windows - Screen Reader
    • Mac - VoiceOver
    • Android - TalkBack
    • iOS - VoiceOver
  • Components affected:
    • Edit Media

Issue description
Within the Edit Media page, when activating the "Restore Image"
button, a message is shown above the image while the Restore button
itself disappears.

Since the button would have been focused at the time when activated by
keyboard, this causes the keyboard focus position to be lost and reset
to the top of the page.

The message itself is also not announced by screen readers, and may not
be visible to screen magnification users if it appears outside their
current view.

Issue Code

    <div class="imgedit-panel-content wp-clearfix">


        <div class="error">


            <p>Cannot save image metadata.</p>


        </div>


        <div class="imgedit-menu wp-clearfix">


            ...


        </div>


    </div>





    ...





    <div class="imgedit-panel-content wp-clearfix">


        <div class="updated">


            <p>Image restored successfully.</p>


        </div>


        <div class="imgedit-menu wp-clearfix">


            ...


        </div>


    </div>

Remediation Guidance
Add tabindex="-1" to the message, and then programatically focus
it when it appears. This will maintain a logical focus position, and
ensure that all visual users will see it, while assistive technologies
announce it.

Adding the alert role will also help to convey to screen readers
that this is an alert-type message.

Recommended Code

    <div class="imgedit-panel-content wp-clearfix">


        <div class="error" tabindex="-1" role="alert">


            <p>Cannot save image metadata.</p>


        </div>


        <div class="imgedit-menu wp-clearfix">


            ...


        </div>


    </div>





    ...





    <div class="imgedit-panel-content wp-clearfix">


        <div class="updated" tabindex="-1" role="alert">


            <p>Image restored successfully.</p>


        </div>


        <div class="imgedit-menu wp-clearfix">


            ...


        </div>


    </div>

Relevant standards

  • 2.1.1 Keyboard (Level A)

Note: This issue may be a duplicate with other existing accessibility-related bugs in this project. This issue comes from the Gutenberg accessibility audit, performed by Tenon and funded by WP Campus. This issue is GUT-73 in Tenon's report

Attachments (4)

57186829-bc9dfc00-6e9a-11e9-99be-b0975c99aea6.png (448.6 KB) - added by anevins 5 years ago.
47147.1.diff (2.3 KB) - added by ryanshoover 5 years ago.
Patch to address the issue
image editor.gif (848.9 KB) - added by afercia 4 years ago.
47147.diff (13.4 KB) - added by afercia 4 years ago.

Download all attachments as: .zip

Change History (48)

#1 @afercia
5 years ago

  • Description modified (diff)

#2 @afercia
5 years ago

Worth noting this applies only to notifications that are dynamically added in a page. Instead, the standard "admin notices" rendered on page load, wouldn't benefit from a role=alert or role=status. See #47111 and #46995.

#3 @afercia
5 years ago

  • Milestone changed from Awaiting Review to 5.3

#4 @afercia
5 years ago

Related: #47111.

This ticket was mentioned in Slack in #core-media by anevins. View the logs.


5 years ago

#6 @anevins
5 years ago

@afercia it seems that we're talking about 2 things;
1) Moving the keyboard focus from the "Restore Image" button which disappears visually;
2) Putting alert roles or status on the notifications

If there's a way to tie into the functionality that hides the submit button, that would be easiest for the focus handling bit

jQuery('.submit').on('click', function() {
   $(this).hide();
   $('.notification').attr('tabindex', '-1').focus();
});

#7 @afercia
5 years ago

@anevins it's indeed two issues.

Currently, when pressing "Restore Image", focus is moved to the first focusable element within the image editor container which is... the "Scale image" help button:

http://cldup.com/w-mQVVimC3.png

Worth reminding the media editor is used also within the media modal:

http://cldup.com/NC6NLEkK66.png

Not ideal, can be certainly changed but the image editor code is pretty old and moving focus to the first focusable element seemed a good idea. In the case of notices appearing in the page, it should behave as suggested above. However, I'd tend to think we should favor a generalic, reusable, solution rather than ad-hoc implementations.

Relevant code is in src/js/_enqueues/lib/image-edit.js.

The second issue are the status notices that appear on the page. Instead of using a role=alert, I'd recommend to use wp.a11y.speak() which is the core implementation of ARIA live regions. Passing the assertive parameter to speak() would produce the equivalent announcement of an ARIA alert.

This ticket was mentioned in Slack in #core-media by anevins. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-media by anevins. View the logs.


5 years ago

@ryanshoover
5 years ago

Patch to address the issue

#10 @ryanshoover
5 years ago

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

@antpb I just uploaded a patch that will handle the core issue. Screen readers now announce all of the "status updates" that can come from editing a piece of media.

I didn't implement the focus aspect, as that collides with a focus set for [imgLoaded](https://github.com/WordPress/WordPress/blob/master/wp-admin/js/image-edit.js#L611)

Can you take a look and hopefully we can get this in before the beta 2 for 5.3?

This ticket was mentioned in Slack in #core-media by mike. View the logs.


5 years ago

#12 @kirasong
5 years ago

@ryanshoover Thanks so much for the patch!

This ticket came up in triage. Would you like to be listed as the owner?

#13 @afercia
5 years ago

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

#14 @ryanshoover
5 years ago

@mikeschroder Sure thing. I can own this. Doesn't look like I can assign myself :)

#15 @afercia
5 years ago

Assigning to @ryanshoover :)

#16 @ryanshoover
5 years ago

@joemcgill @antpb Just double checking that the media team can look at this patch in time for 5.3 :)

#17 @antpb
5 years ago

I think this looks great @ryanshoover ! Thank you so much! Patch applied clean and I'm running tests now. Will add commit and get a second commiter to review after I test.

#18 @antpb
5 years ago

  • Keywords commit added; needs-testing removed

#19 @antpb
5 years ago

  • Keywords commit removed

Ah removing commit, we may need to factor in the suggestion here: https://core.trac.wordpress.org/ticket/47147#comment:7

"
Instead of using a role=alert, I'd recommend to use wp.a11y.speak() which is the core implementation of ARIA live regions. Passing the assertive parameter to speak() would produce the equivalent announcement of an ARIA alert.
"

Last edited 5 years ago by antpb (previous) (diff)

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


5 years ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


5 years ago

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


5 years ago

#23 @audrasjb
5 years ago

  • Keywords early added
  • Milestone changed from 5.3 to 5.4

Hi,
Thanks for the work done on this ticket.
Unfortunately, we are now at Beta 3 in the release cycle. I'm moving this ticket to 5.4 early milestone.
Feel free to discuss it if you feel this ticket could land before 5.3 Release Candidate 1.

#24 @kirasong
5 years ago

Just a quick note that I think this is totally fine to land in 5.3 still, if someone has time to work on it.

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


4 years ago

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


4 years ago

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


4 years ago

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


4 years ago

#29 @audrasjb
4 years ago

  • Milestone changed from 5.4 to 5.5

Howdy,

This ticket still needs a patch and 5.4 beta 1 is tomorrow. Let's move this ticket to 5.5.

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


4 years ago

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


4 years ago

#32 @johnbillion
4 years ago

  • Keywords early removed

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


4 years ago

#34 @afercia
4 years ago

  • Keywords needs-refresh added
  • Owner changed from ryanshoover to afercia

#35 @afercia
4 years ago

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

Testing a bit the current approach, I'm not sure we can use the role="alert" pattern. Due to the way the image editor works (which is a bit inconsistent and shows its age) the alert is sometimes announced and sometimes not.

For example, testing with Firefox and NVDA:

  • the alert is announced when saving "normal" actions
  • it is not announced when scaling or restoring an image or in any other case where the AJAX response rebuilds the whole image editor HTML

Screen readers are able to announce an alert when it's "injected" in the DOM. But when there's a large DOM update and the updated region contains an alert, it seems they're not able to understand what is going on. The DOM update is just too large. I think a more solid approach would be using wp.a11y.speak() instead.

Moreover, as it's pretty common in the legacy WordPress codebase, most of the involved functions "do something" but then also return a value. For example:

  • wp_save_image() processes the image saving and then returns an object with some image data and the error / success message
  • wp_restore_imagerestores the image data and then then returns the error / success message
  • wp_image_editor() generates the whole Image Editor markup, which is returned by te main AJAX action image-editor

Additionally, looking at this switch statement in the image-editor AJAX action: https://github.com/WordPress/wordpress-develop/blob/25133cf3f1bc2ac3c2294a291396ebddb80b0e98/src/wp-admin/includes/ajax-actions.php#L2603-L2618

  • when saving: the wp_save_image() message object is passed as JSON encoded response
  • when scaling: the wp_save_image() message object is not passed as JSON encoded response, instead it's passed to wp_image_editor() which returns a response containing the whole HTML blob
  • when restoring: the wp_restore_image() message object is not passed as JSON encoded response, instead it's passed to wp_image_editor() which returns a response containing the whole HTML blob

Basically the image-editor AJAX action response is different depending on the cases. Sometimes it contains a message that could be used on the JS side by wp.a11y.speak(). Sometimes it just returns the HTML blob.

In order to use wp.a11y.speak() reliably, the response should always return an object that contains also a message. This would require some refactoring of the (pretty old) code responsible for the various responses. Will attempt a patch.

#36 @afercia
4 years ago

  • Summary changed from Status message not exposed to assistive technologies to Status message not exposed to assistive technologies (in the Image Editor)

I'm attaching an animated GIF to better illustrate the role="alert" approach doesn't work (when restoring) even when setting focus on the alert (because of the too large DOM update). Tested with Firefox and NVDA. Focus on the NVDA speech viewer, where it's clear the alert isn't announced automatically.

Last edited 4 years ago by afercia (previous) (diff)

@afercia
4 years ago

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


4 years ago

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


4 years ago

@afercia
4 years ago

#39 follow-up: @afercia
4 years ago

  • Keywords has-patch needs-dev-note added; needs-patch removed

47147.diff takes a broader approach and does the following:

  • changes the response format of wp_ajax_image_editor() so that it is possible to get the messages and pass them to wp_send_json_error() and wp_send_json_success()
  • in order to do so, it uses output buffering to split the HTML from the messages
  • I'd like the part above to be reviewed by the Media component maintainers or alternatively from the release Core Tech, /Cc @azaozz @SergeyBiryukov

Additionally:

  • uses wp.a11y.speak() to announce messages to assistive technology
  • announce existing messages for the scale and restore actions
  • uses wp-i18n and entirely removes imageEditL10n
  • improves focus management: focus is moved to the admin alert notices, if any, or to the first tabbable element within the Image editor

Hint: To test the accessibility part of the patch and see when focus is set to the admin alert notices, a temporary style for focus may help. You can add somewhere in some CSS used on the page something along these lines:
:focus { outline: 2px solid red !important; }

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


4 years ago

#41 in reply to: ↑ 39 @SergeyBiryukov
4 years ago

Replying to afercia:

47147.diff takes a broader approach and does the following:

  • changes the response format of wp_ajax_image_editor() so that it is possible to get the messages and pass them to wp_send_json_error() and wp_send_json_success()
  • in order to do so, it uses output buffering to split the HTML from the messages

Looks good to me.

#42 @afercia
4 years ago

  • Keywords commit added

From an accessibility perspective, and for history, it is worth to quote an important note from a similar issue reported by the WPCampus accessibility report on Gutenberg. See https://core.trac.wordpress.org/ticket/47120 and https://github.com/WordPress/gutenberg/issues/15302

Consider moving focus to either the error container or the error
container close button when the error appears. This will ensure that
Braille-only and screen magnification users are alerted that an error
has occurred.

Normally, moving focus to errors like this is not advised, however
currently the error and the informative text are visually far away from
the "Select Files" button, and neither Braille nor screen
magnification software announce status messages. A better solution is to
change the design to bring the error visually directly under the
"Select Files" button (while still also using a live region on the
error message).

A major redesign of the Image Editor is tracked in #50523. For now, we opted for a simpler approach.

#43 @afercia
4 years ago

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

In 48375:

Accessibility: Media: Improve accessibility of the status and error messages in the Image Editor.

  • improves focus management by moving focus to the notices, if any, or to the first "tabbable" element
  • this avoids a focus loss and helps Braille-only and screen magnification users to be aware of the messages
  • adds an ARIA role alert to all the notices
  • uses wp.a11y.speak() to announce messages to assistive technology
  • this way, all visual users will see the messages while assistive technology users will get an audible message
  • uses wp.i18n for translatable strings in wp-admin/js/image-edit.js

Props anevins, ryanshoover, antpb, SergeyBiryukov, afercia.
See #20491.
Fixes #47147.

Note: See TracTickets for help on using tickets.