Make WordPress Core

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#47120 closed defect (bug) (fixed)

Media modals: Upload errors and field information are not associated with their control

Reported by: afercia's profile afercia Owned by: antpb's profile antpb
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Media Keywords: wpcampus-report has-patch commit
Focuses: ui, accessibility Cc:

Description

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

Errors and field information are not associated with their control

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

#### Issue description

When users attempt to upload an image in the "Featured Image" modal
dialog, and the image is too large, an error message appears visibly.
However blind and low-vision users are not alerted with the
announcement, and focus remains on the upload button (except in Edge
browser, where focus goes back to the top of the page after a failed
upload).

The 2MB limit is also not programmatically associated with the "Select
Files" button, so blind and low-vision users may miss that limit unless
they happen to navigate past the button in exploration of the whole
modal.

Users who are zoomed-in or using magnification software may not see the
error as it may be outside their viewports. Screen reader users remain
on the "Select Files" button and need to navigate around to see if
there are any errors or if the upload was successful.

[![Screenshot of the error message and the 2MB limit informative
text](/images/17718_feature_image_bigly_yuge.png)]{.image-wrap}

##### Issue Code

    <button type="button" class="browser button button-hero" id="__wp-uploader-id-4" style="display: inline-block; position: relative; z-index: 1;">Select Files</button>


    ...


    <div class="upload-errors">


        <div class="upload-error">


            <span class="upload-error-filename">trump.gif</span>


            <span class="upload-error-message">trump.gif exceeds the maximum upload size for this site.</span>


        </div>


    </div>


    ...


    <div class="post-upload-ui">


        <p class="max-upload-size">Maximum upload file size: 2 MB.</p>


    </div>

#### Remediation Guidance

Put role=alert on the error, and on any additional error or success
messages that are added as users upload media. This will allow screen
reader users to know immediately that there is an error.

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).

Programmatically associate the informative text ("max 2MB size") with
the "Select Files" button using aria-labelledby. Do this by giving
the informative text container an id value and set this in order in
an aria-labelledby attribute on the "Select Files" button. Include
the button's own id token in the aria-labellebdy so that the
button's text isn't overridden.

##### Recommended Code

    <button type="button" class="..." aria-labelledby="__wp-uploader-id-4 post-upload-info" id="__wp-uploader-id-4" style="...">Select Files</button>


    ...


    <button type="button" class="...">


        <span class="screen-reader-text">Dismiss Errors</span>


    </button>


    ...


    <div class="upload-errors">


        <div class="upload-error" role="alert">


            <h3 class="...">trump.gif</h3>


            <p class="...">trump.gif exceeds the maximum upload size for this site.</p>


        </div>


    </div>


    ...


    <div class="post-upload-ui" id="post-upload-info">


        <p class="max-upload-size">Maximum upload file size: 2 MB.</p>


    </div>

#### Relevant standards

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](https://www.tenon.io) and funded by [WP Campus](https://wpcampus.org/). This issue is GUT-60 in Tenon's report

Attachments (6)

47120.png (124.4 KB) - added by afercia 5 years ago.
47120-2.png (166.5 KB) - added by afercia 5 years ago.
47120.diff (2.4 KB) - added by joedolson 3 years ago.
Partial patch.
47120.2.diff (3.4 KB) - added by joedolson 3 years ago.
Changes a role=alert to aria-live=assertive for consistency. In testing, role=alert double read notices.
47120.3.diff (2.2 KB) - added by joedolson 3 years ago.
Working patch!
47120.4.diff (2.2 KB) - added by joedolson 3 years ago.
Switch from _.defer to _.delay

Download all attachments as: .zip

Change History (83)

#1 @afercia
5 years ago

Note: this applies to all the media views when uploading attachments and upload errors are displayed, usually because of (and can be tested with):

  • file size exceeding upload limit
  • dangerous file extension (e.g. .exe)
  • file with no extension
  • empty file

#2 @afercia
5 years ago

  • Milestone changed from Future Release to 5.3

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


5 years ago

#4 @anevins
5 years ago

@afercia We still need to figure out where to move the focus. The remediation says "moving focus to errors", but at the same time it can be a bit unhelpful to move the focus to a containing DIV that lacks information. Do we move the focus to the element with the Alert role?

#5 @afercia
5 years ago

@anevins thanks for the ping.

For the announcement, I think it's OK to use role=alert or, better, wp.a11y.speak() with the assertive parameter, which is equivalent to an alert.

For the focus part, I think the remediation actually says the preferred solution would be bringing the error message "in view" close to the "Select Files" button. Re-reading:

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).

Ideally, I'd recommend to change the design. Alternatively, move focus but I wouldn't move it to the alert or to a DIV. An example can be seen here: https://www.tenon-ui.info/forms-full-demo

  • submit the form leaving all fields empty
  • focus is moved to the top, to a h2 heading with meaningful text

#6 @anevins
5 years ago

@afercia I'm not sure I understand. The Focus will still need to happen won't it? Because people will be left on the form's submission button and will have to navigate backwards to get to the form field in question. Is navigating backwards okay? I'm asking genuinely.

@afercia
5 years ago

#7 @anevins
5 years ago

Gotcha

@afercia
5 years ago

#8 @afercia
5 years ago

I've uploaded the image from the WPCampus complete technical report PDF document (329 pages) and one more image for clarification, as I think the image from the report identifies only one of the possible cases.

It's important to note that the media uploader behavior actually differs depending on the error type, for example:

  • file exceeding allowed size or file with no extension: the error is displayed inline below the "Select Files" button (first image)
  • empty file: the modal switches to the media grid and the error is displayed in the sidebar (second image)

This makes things a bit more complicated.

Regardless, in this case there's no text input field nor form submission. There's a "Select Files" button. In any case the error message should be displayed underneath the button otherwise it may be outside the viewport for some users or difficult to find for other users e.g. Braille-only users.

Note: When in doubt, please refer to the WPCampus complete report, as some details may have been list when moving issues from GitHub to Trac. The PDF is published on the WPCampus web site.

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


5 years ago

#10 @anevins
5 years ago

Hi @afercia We've mentioned this in the Media meeting and struggled to define a set of actions we need to take when assigning this to someone to own. Would you be able to help with this? Thanks!

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


5 years ago

#12 @joedolson
5 years ago

If you need help with guidance, I can also provide some help here. I'll track the ticket for questions; I can't easily make the media team meetings, however.

#13 @afercia
5 years ago

I would try to investigate:

  • why the modal needs to switch view to display some errors in the sidebar: any technical reason for this?
  • ideally, the view shouldn't change
  • the error should just appear "in place", as close as possible to the "Select Files" button

Re-reading the essential parts of the remediation suggested from the WPCampus report:

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).

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

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


5 years ago

#17 @audrasjb
5 years ago

  • Keywords needs-patch added

@mikeschroder does the core media team need some input from accessibility side?
It would be nice to have an owner from the Core media team here. Thanks :)

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


5 years ago

#19 @antpb
5 years ago

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

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 afercia. View the logs.


5 years ago

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


4 years ago

#23 @kirasong
4 years ago

Just wanted to check in!

@antpb: Do you want any more input or help with this one?

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


4 years ago

#25 @audrasjb
4 years ago

  • Keywords early added
  • Milestone changed from 5.3 to 5.4

Hi,

5.3 beta 3 is on Monday, and this ticket doesn't have any patch for the moment.
Since it will probably deserves some testing before commit, I don't think it's realistic to get this fixed in 5.3.

Moving the ticket to 5.4 early so it could be fixed on time for 5.4.

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

#30 @audrasjb
4 years ago

  • Milestone changed from 5.4 to 5.5

5.4 beta 1 is on Monday, and this ticket doesn't have any patch for the moment.
Since it will probably deserves some testing before commit, I don't think it's realistic to get this fixed in 5.4.
Moving the ticket to 5.5 early so hopefully it could be fixed on time for 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

#33 @johnbillion
4 years ago

  • Keywords early removed

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

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


4 years ago

#37 @afercia
4 years ago

  • Milestone changed from 5.5 to 5.6

This ticket was discussed during today's accessibility bug-scrub: the team agreed to move it to the next release cycle.

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


3 years ago

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


3 years ago

#40 @audrasjb
3 years ago

  • Milestone changed from 5.6 to Future Release

Moving to Future release as it didn't move during WP 5.6 alpha cycle.

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


3 years ago

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


3 years ago

#43 @antpb
3 years ago

  • Milestone changed from Future Release to 5.7

Here we are again, for real this time! This issue was discussed in the recent Media component meeting and we're all in agreement this has actionable next steps and just needs to be owned by someone with time. I'm going to continue bringing it up in meetings and document next steps as the come toward 5.7.

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


3 years ago

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


3 years ago

#46 @antpb
3 years ago

This was discussed in the recent Media component meeting. What is still remaining to be figured out here is how to pass the message to the location recommended in the report. Because of the media library structure I found that the location we are recommended moving it is difficult to receive the error message and in a separate file. I hope this is a lack of my JS skills and might be an easy fix, but any help you could provide would be much appreciated @joedolson Thank you!

#47 @joedolson
3 years ago

I'm pretty confident that this is possible; but there's no denying that it's complicated. I've been spending the last couple days exploring, and I've made some progress - mostly in learning how the media library is put together.

It's not absolutely critical that we move the error to a new location, and if we can't get it figured out, we should go ahead and move ahead regardless with the other solution offered by Tenon.io in their initial report by moving focus to the error.

I don't want to let the struggle with getting the perfect solution stop us from making what we have better.

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


3 years ago

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


3 years ago

#50 @joedolson
3 years ago

This ticket represents two separate issues:

a) Programmatically associate the informative txt with the Select Files button
b) Announce errors to screen readers and move focus.

The first is fairly simple, and is complete in 47120.diff

The second is trickier. I have a patch that seemed like it should work (using the 'move focus' option, rather than moving the error), but it doesn't work on the first upload; only on subsequent uploads. I haven't yet figured out why that is.

Adding aria-live="assertive" to upload-inline-status was sufficient to read out results in the media modal, and that error is already in close visual & DOM proximity.

If somebody with better knowledge of Backbone and JS can try and figure out why upload errors in the non-modal media library context don't trigger on the first error, I think that would resolve this issue.

@joedolson
3 years ago

Partial patch.

#51 @joedolson
3 years ago

To test:

1) Media > Add New

Navigate to 'Select Files'

  • Screen reader should output 'Select Files' and add the descriptive information shown below

Upload a file that triggers an error

  • Screen reader should report the error, focus should move to the error container.

Note: this currently only works on repeated errors, the first error does not trigger the alert or the focus move.

2) Add media to post: same tests within the Media Modal.

Note: in the media modal, the alert and focus move *do* work as expected with this patch.

@joedolson
3 years ago

Changes a role=alert to aria-live=assertive for consistency. In testing, role=alert double read notices.

#52 @joedolson
3 years ago

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

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


3 years ago

This ticket was mentioned in PR #962 on WordPress/wordpress-develop by adamsilverstein.


3 years ago
#54

Trac ticket:

#55 @adamsilverstein
3 years ago

Note: this currently only works on repeated errors, the first error does not trigger the alert or the focus move.

@joedolson I reviewed this and experienced the issue you mentioned. Even removing the focus on error I see the same issue.

I don't fully understand what is supports to trigger the speaking here, we don't call wp.a11y.speak() explicitly, so I guess there is a listener for changes to the error areas that is responsible? Is it possible to switch this to a more explicit call when the error is returned?

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


3 years ago

#57 @joedolson
3 years ago

The change is the addition of aria-live="assertive" on the container that receives the error, as well as the addition of tabindex='-1' on the parent container.

I can see that the tabindex is added, but yet the container doesn't receive focus & the message isn't spoken on the first error, only on subsequent errors. My theory is that some backbone refresh is occurring around the assignment of focus that disrupts this, but I just can't see what's happening.

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


3 years ago

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


3 years ago

@joedolson
3 years ago

Working patch!

#60 @joedolson
3 years ago

  • Keywords needs-testing added; dev-feedback removed

New patch uses wp.a11y.speak() and ._defer() to defer the error message and focus change until Backbone is finished rendering.

Could use some broader testing, but seems to do the trick. Testing process still same as above in https://core.trac.wordpress.org/ticket/47120#comment:51

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


3 years ago

#62 @adamsilverstein
3 years ago

@joedolson Excellent! I will give this another test.

#63 @adamsilverstein
3 years ago

@joedolson I am testing with voiceover in Chrome, let me know if I should try with Safari or another tool. Maybe my test setup is not a usual one for screen reader users?

  • Turn on voiceover.
  • Using my keyboard only, i upload an image that exceeds the max upload size.
  • The error is not spoken, instead the close button (which is now focused) is described

Screencast: https://share.getcloudapp.com/geu4W5dL

I was able to get it working by adding a bit more of a delay (add ,1000 after the function in the defer call). I'm not sure if that is required, and it does feel a bit fragile.

#64 @joedolson
3 years ago

@adamsilverstein I'd consider VoiceOver with Chrome a very low priority combination. The most relevant combinations to test are JAWS/Chrome; NVDA/Firefox; NVDA/Chrome; JAWS/IE and VoiceOver/Safari.

Every combination of AT/browser will work a bit differently, and VoiceOver is the furthest outside the norm, and best with Safari.

I'll test with a delay in other environments, and we can see. But no, VoiceOver/Chrome is not a particularly common set up for screen reader users - if this causes problems in other environments, I'd give preference to more common pairings. Did you experiment with a shorter delay? That seems pretty long.

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

#65 @poena
3 years ago

I tested the view from the Add New button in the Media Library (wp-admin/upload.php).

47120.3.diff, Windows 10, Chrome Version 88.0.4324.150 with NVDA

When the Select Files button is focused, the information about the max file size is read correctly.

When the upload fails, the focus is on the button that dismisses the error.

I am not able to see a pattern for when the message is read out loud and when it is not.
It does not seem to be related to if it is the first or second error. Sometimes it is read out on the first error.

The error is always written in the NVDA Speach viewer, but I don't always hear it.

When the upload fails and the error is read out, it reads the title of the page before it reads the error,
and after the error, it reads the title of the page again:

Media Library ‹ My New WordPress Site — WordPress - Google Chrome
gutenberg (1).zip exceeds the maximum upload size for this site. 
Media Library ‹ My New WordPress Site — WordPress  document
Version 0, edited 3 years ago by poena (next)

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


3 years ago

@joedolson
3 years ago

Switch from _.defer to _.delay

#67 @joedolson
3 years ago

  • Keywords commit added; needs-testing removed

In testing, I didn't find that adding an argument to _.defer did anything - and reading the documentation, it doesn't seem like it *should* have done anything; did you also switch from _.defer to _.delay, @adamsilverstein?

Regardless, I did testing both ways, using JAWS/Chrome, JAWS/Firefox, NVDA/Chrome and NVDA/Firefox, and found that using delay was a lot more consistent - it seemed to be generally read in numerous situations where the deferred function wasn't.

I find delay to be less elegant; it's just a magic number delay that one hopes will work, where defer should wait until the call stack is clear, but in practice, it seems to work a lot better.

I think we should commit this using _.delay; if we can find a better solution later, that's fine, but this appears to work in *most* cases.

#68 @adamsilverstein
3 years ago

Great! makes sense that delay would work better, defer may not pick up the activity happening in the screen reader, outside the browser. I'll give this another test to see how it works for my setup.

@poena Excellent, thank you for testing! If you can, please test the latest patch version and see if the behavior is improved for you.

I think we should commit this using _.delay; if we can find a better solution later, that's fine, but this appears to work in *most* cases.

+1 to committing this, certainly improves the situation. Do you think in the future browser or screen reader APIs might improve the situation here? If so, maybe add a code comment here that explains the delay could possibly be removed in the future.

#69 @poena
3 years ago

Hi, I tested 47120.4.diff.
It now reads the error out loud on every failed upload.
But it reads the information for the Select Files button first.

Speech Viewer output:
Media Library ‹ My New WordPress Site — WordPress - Google Chrome
Media Library ‹ My New WordPress Site — WordPress document
button Select Files Maximum upload file size: 2 MB.
Media Library ‹ My New WordPress Site — WordPress - Google Chrome
gutenberg (2).zip exceeds the maximum upload size for this site.
Media Library ‹ My New WordPress Site — WordPress document
button Select Files Maximum upload file size: 2 MB.
clickable Dismiss Errors button

I wanted to ask if this patch should also fix the wp-admin/media-new.php view,
that is reached from the menu under Media > Add New

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


3 years ago

#71 @adamsilverstein
3 years ago

Thanks @poena for re-testing!

But it reads the information for the Select Files button first.

Hmm, I noticed we call the focus first, then speak the error. Perhaps these should be reversed, what do you think @joedolson? I'll try it that way in my testing to see if that changes the spoken order.

#72 @joedolson
3 years ago

The delay is so that all other reading will be finished by the time the error message is read. Ideally, the 'Select Files' button should not be read at all. I suspect that the reason the ._defer call doesn't work is because Backbone does something to add a second callstack after the callstack including the defer is finished, and that is a refresh of the file uploader view. Fixing that would probably require a significant refactor of the media library, however.

The focus/speak order was intentional; moving focus and then calling the error ensure that the new focus won't interrupt the wp.a11y.speak call. I haven't tested switching the order in combination with the ._delay function, however.

I would only expect that to change the order of 'Dismiss Errors' and the error message itself; it would have no impact on the ordering of 'Select Files'.

I wanted to ask if this patch should also fix the wp-admin/media-new.php view, that is reached from the menu under Media > Add New

Yes. That view is a bit wonkier, however; the calls are slightly different, and messier.

#73 @adamsilverstein
3 years ago

The focus/speak order was intentional; moving focus and then calling the error ensure that the new focus won't interrupt the wp.a11y.speak call.

Ah, makes sense.

+1 from me @joedolson!

#74 @joedolson
3 years ago

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

In 50352:

Media: Associate upload errors and field with controls.

Adds an explicit relationship between the upload button and the maximum upload limit, moves focus to the error dismiss button if an error occurs, and adds a call to wp.a11y.speak to report the error after it occurs.

Props afercia, anevins, antpb, adamsilverstein, poena
Fixes #47120

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


3 years ago

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


3 years ago

Note: See TracTickets for help on using tickets.