WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#23560 closed defect (bug) (fixed)

Keyboard Accessibility of Add Media Panel

Reported by: grahamarmfield Owned by:
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.5.1
Component: Media Keywords:
Focuses: accessibility, javascript Cc:

Description

Trying to keep this trac specific I'm talking here about keyboard-only interaction with the new Add Media panel once you have opened it by actioning the Add Media link whilst editing a page or a post.

Accessibility Issues:

  • The most serious problem with accessibility in the Add Media functionality is that it is not possible to select any of the pre-uploaded media without using a mouse. This will effectively preclude any keyboard only or screen reader users from using this functionality.
  • When the Add Media panel first opens it is unclear where keyboard focus sits. This could be disconcerting for some users.
  • Reverse tabbing from the panel transfers focus back into the main page whilst leaving the panel open. Recommend that focus is kept cycling within the panel until the user either explicitly closes the Add Media panel or inserts some images.
  • It is possible to tab through the text links on the panel(s), but the Upload Files link does not show focus as obviously as other links as there is no outline.
  • When tabbing forwards from the Upload Files link there is a tab stop that is not at all visible. Believe this to be Media Library but it's not clear.
  • The Insert into Page link can receive focus even when inactive.

Separate trac tickets will be raised to cover the full screen reader and speech recognition software experience.

Attachments (40)

23560.diff (3.8 KB) - added by lessbloat 4 years ago.
23560.1.diff (4.4 KB) - added by lessbloat 4 years ago.
23560.2.diff (4.5 KB) - added by lessbloat 4 years ago.
23560.2.2.diff (5.1 KB) - added by lessbloat 4 years ago.
23560.3diff (5.1 KB) - added by lessbloat 4 years ago.
23560.3.diff (5.1 KB) - added by lessbloat 4 years ago.
23560.4.diff (5.1 KB) - added by atimmer 4 years ago.
23560.5.diff (8.2 KB) - added by lessbloat 4 years ago.
23560.6.diff (8.5 KB) - added by lessbloat 4 years ago.
23560.7.diff (8.5 KB) - added by lessbloat 4 years ago.
23560.8.diff (8.5 KB) - added by lessbloat 4 years ago.
23560.9.diff (8.8 KB) - added by bramd 3 years ago.
Refresh of 23560.8.diff
23560.9.2.diff (9.0 KB) - added by bramd 3 years ago.
Refresh of 23560.8
23560.10.diff (9.3 KB) - added by helen 3 years ago.
23560-left-side-select.diff (476 bytes) - added by lessbloat 3 years ago.
23560-image-selected.diff (484 bytes) - added by lessbloat 3 years ago.
23560-reverse-tabbing.diff (1.2 KB) - added by lessbloat 3 years ago.
23560-arrow-keys.diff (2.1 KB) - added by lessbloat 3 years ago.
23560.11.diff (429 bytes) - added by ericlewis 3 years ago.
23560.12.diff (4.4 KB) - added by celloexpressions 3 years ago.
merge previous outstanding patches, selection focus, initial focus fix
23560.13.diff (6.6 KB) - added by celloexpressions 3 years ago.
Focus states for selected (but inactive) attachments, ability to remove items from the selection.
23560.14.diff (6.9 KB) - added by celloexpressions 3 years ago.
Set focus as if a newly uploaded attachment was selected (which it is, automatically), when uploading.
23560.15.diff (6.8 KB) - added by celloexpressions 3 years ago.
focus first input in sidebar, not first focusable element
23560.colors.diff (404 bytes) - added by celloexpressions 3 years ago.
Key [29220] to color schemes where appropriate.
23560.16.diff (2.6 KB) - added by ericlewis 3 years ago.
23560.17.diff (2.6 KB) - added by ericlewis 3 years ago.
23560.18.diff (1.7 KB) - added by ericlewis 3 years ago.
media-modal-broken-colorschemes-selected-focus.png (144.7 KB) - added by celloexpressions 3 years ago.
23560.19.diff (500 bytes) - added by adamsilverstein 3 years ago.
focus on close btn when switching to edit image
23560.20.diff (444 bytes) - added by adamsilverstein 3 years ago.
media-modal-close focus in setState
23560.21.diff (842 bytes) - added by adamsilverstein 3 years ago.
focus close btn when switching to image-selection-edit mode and back
23560.22.diff (496 bytes) - added by adamsilverstein 3 years ago.
focus next/back arrows focus when navigating detail media grid w/keyboard
23560.23.diff (500 bytes) - added by adamsilverstein 3 years ago.
like the last, but prettier
23560.24.diff (1.8 KB) - added by adamsilverstein 3 years ago.
refresh and combine previous patches
23560.25.diff (2.1 KB) - added by adamsilverstein 3 years ago.
more focused selectors
23560-always-show-edit-selection-checkmarks.diff (857 bytes) - added by adamsilverstein 3 years ago.
edit-selection mode: make the close icons have a focus styling and always be visible
23560.26.diff (1.7 KB) - added by gcorne 3 years ago.
23560.27.diff (3.1 KB) - added by gcorne 3 years ago.
23560.28.diff (3.8 KB) - added by gcorne 3 years ago.
23560.29.diff (3.3 KB) - added by gcorne 3 years ago.

Download all attachments as: .zip

Change History (175)

#1 @toscho
4 years ago

  • Cc info@… added

#2 @grahamarmfield
4 years ago

The main problem with keyboard operability here is that there are no meaningful items in the media list that can receive focus.

The overall functionality of the media selection is like a series of checkboxes. For keyboard and screen reader and VR users the interface either needs to be coded like that, or potentially some ARIA techniques employed (however it is not certain they will work with VR).

Once the functionality is provided to select the media via keyboard, it is then not clear how quickly a keyboard user could navigate to the panel that contains the individual attributes for an image. Tabbing through the entire collection of media is not a viable option here - as especially on large sites with many files, the infinite scrolling will make this an awful experience.

Perhaps an accessibility mode is required here - as per existing widgets functionality, and that proposed for Custom Menu builder (see #14045).

This issue is being raised on make.wordpress.org/accessibility for some collective thought.

The separate trac tickets raised for screen reader and VR experience are: #23561 and #23562.

#3 @grahamarmfield
4 years ago

  • Cc graham.armfield@… added

@lessbloat
4 years ago

#4 follow-up: @lessbloat
4 years ago

23560.diff​ addresses some of these items, including:

  • Keeping the focus in the media modal once it's open
  • Moves focus to #wpbody-content once the modal is closed (needs to be an element that is present on every page where the media modal is used)
  • Made it so the media items could be tabbed to. Added role=checkbox, and the file name as the title (so that visually impaired users will know what they are focused on)
  • When focused on an item, enter selects and deselects it. Shift + enter allows multiple selection.
  • I added tabindex=-1 to the non-visible tab stop, and to the plus/minus link which shows up after you've selected an item
  • Added an empty alt to the image
  • Added an outline to the "upload files" and "media library" links

Please let me know how this works out.

A couple of observations after looking at this:

  • Adding labels around the items will be a fair amount of work, since the focus is actually on LI tags, and I'm fairly certain we can't just wrap a label around an LI.
  • If someone has a lot of media items, they will need to tab through all of them to get to the right column. Is there some keyboard combo we could use as a shortcut to get you there?

#5 in reply to: ↑ 4 ; follow-up: @grahamarmfield
4 years ago

Replying to lessbloat:

23560.diff​ addresses some of these items, including:

Appreciate the work you're doing on this. I don't have the facility to test this myself as I don't have a suitable environment, but I have some comments I'd like to make:

  • Keeping the focus in the media modal once it's open

Excellent.

  • Moves focus to #wpbody-content once the modal is closed (needs to be an element that is present on every page where the media modal is used)

OK for now. Perhaps we can discuss further longer term.

  • Made it so the media items could be tabbed to. Added role=checkbox, and the file name as the title (so that visually impaired users will know what they are focused on)

... and ...

  • Adding labels around the items will be a fair amount of work, since the focus is actually on LI tags,

Since the purpose of the attributes that I'm proposing is to allow items in the list to get focus and to behave like a checkbox grouping it doesn't really matter which element the attributes sit on. So if the <li> item is the actual one that responds to click then you could put the attributes on that.

All the attributes that I specified will need to be employed to get this to work in screen readers and for keyboard users. See the example code at end of this comment.

Please also note that using the title attribute to label items is no guarantee that screen reader users will get that information. Some screen readers voice title attributes but not all do, and those that do only voice them in certain situations.

  • When focused on an item, enter selects and deselects it. Shift + enter allows multiple selection.

You will also need to toggle selection by listening for the space bar presses. This ARIA technique works for screen readers by 'fooling' the screen reader that this is a checkbox. In standard forms the checkbox state is toggled by space bar. Users will expect enter key to submit the form they are in.

Whether the enter key or space bar has been pressed, the aria-checked attribute must be updated to reflect the state - true or false.

  • I added tabindex=-1 to the non-visible tab stop, and to the plus/minus link which shows up after you've selected an item

This may be superfluous, but I'd have to test for real.

  • Added an empty alt to the image

Excellent

  • Added an outline to the "upload files" and "media library" links

Excellent

Please let me know how this works out.

A couple of observations after looking at this:

  • Adding labels around the items will be a fair amount of work, since the focus is actually on LI tags, and I'm fairly certain we can't just wrap a label around an LI.

Understand the problem here. I have a solution for this which is to add the aria-label attribute to the <li> too and make it's value equal to the media file's title. This will remove the need for the label. See code example below.

  • If someone has a lot of media items, they will need to tab through all of them to get to the right column. Is there some keyboard combo we could use as a shortcut to get you there?

It's a valid concern for keyboard-only users and one we should address in the future. I'd be cautious about adding extra shortcuts. Screen reader users can use the Attachment Details heading to jump to the input fields (it's a facility within screen readers to use headings as navigational items).

Example code to try
Please note that this has changed from the original example I gave on the make.wordpress.org/accessibility blog (http://make.wordpress.org/accessibility/2013/03/28/add-media-panel-and-wordpress-3-6-a-simple-solution/) to avoid using a <label> element, and to attach new attributes to <li> element instead of image.

Unselected:

<li class="attachment save-ready" tabindex="0" role="checkbox" aria-checked="false" aria-label="Media file title">

Selected:

<li class="attachment save-ready details selected" tabindex="0" role="checkbox" aria-checked="true" aria-label="Media file title">

I have tested this markup in isolation with NVDA and it behaves as I'd expect.

@lessbloat
4 years ago

#6 in reply to: ↑ 5 @lessbloat
4 years ago

Replying to grahamarmfield:

Example code to try
Please note that this has changed from the original example I gave on the make.wordpress.org/accessibility blog (http://make.wordpress.org/accessibility/2013/03/28/add-media-panel-and-wordpress-3-6-a-simple-solution/) to avoid using a <label> element, and to attach new attributes to <li> element instead of image.

Unselected:

<li class="attachment save-ready" tabindex="0" role="checkbox" aria-checked="false" aria-label="Media file title">

Selected:

<li class="attachment save-ready details selected" tabindex="0" role="checkbox" aria-checked="true" aria-label="Media file title">

I have tested this markup in isolation with NVDA and it behaves as I'd expect.

23560.1.diff​ should do it I think.

#7 @grahamarmfield
4 years ago

I'm a bit green with jquery but the diff makes sense. Is there somewhere you could put it so that I can test it? Thanks.

@lessbloat
4 years ago

@lessbloat
4 years ago

#8 follow-up: @lessbloat
4 years ago

23560.2.2.diff adds the ability to select an item with either the space bar, or the enter key. It also adds better focus styles to the items (especially in IE).

#9 in reply to: ↑ 8 ; follow-up: @grahamarmfield
4 years ago

Replying to lessbloat:

23560.2.2.diff adds the ability to select an item with either the space bar, or the enter key. It also adds better focus styles to the items (especially in IE).

Both key changes - thanks for that.

I'm still having problems selecting with NVDA active but that's possibly down to how NVDA reacts to ARIA - I'll investigate further. If I use the NVDA 'pass-through' key then I can get it to select.

One other suggestion for improved user experience for screen readers: Currently you are presenting the name of the image in the aria-label attribute. In the cases of the images in the test site they are matt4.jpg etc. But they could of course be DSC0011.jpg etc.

How feasible would it be to pull out the individual titles for each of the images instead of the actual filename? That way, if I've added a meaningful title and/or alternate text when I upload the image then that will be externalised when I'm selecting the file to be included in the page.

Cheers for what you've done here.

#10 in reply to: ↑ 9 ; follow-up: @lessbloat
4 years ago

Replying to grahamarmfield:

How feasible would it be to pull out the individual titles for each of the images instead of the actual filename? That way, if I've added a meaningful title and/or alternate text when I upload the image then that will be externalised when I'm selecting the file to be included in the page.

What percentage of the time will there be a title though? I'd imagine a lot of the time, there is no title, and the user just adds the image without adding one.

The options I see are, we could:

A) Keep using file name (would work all of the time, but some file names make little sense, as you pointed out)
B) Use title all of the time (I'd say the vast majority of files won't have a title, and so the aria description will be left blank)
C) Use the file name, but when the title is there, use that instead (this could be a bit confusing)
D) Always show the file name, and if there is a title, add it in there as well (this could end up being rather long)

Let me know which one you prefer. Thanks.

#11 in reply to: ↑ 10 ; follow-up: @grahamarmfield
4 years ago

Replying to lessbloat:

What percentage of the time will there be a title though? I'd imagine a lot of the time, there is no title, and the user just adds the image without adding one.

The options I see are, we could:

A) Keep using file name (would work all of the time, but some file names make little sense, as you pointed out)
B) Use title all of the time (I'd say the vast majority of files won't have a title, and so the aria description will be left blank)
C) Use the file name, but when the title is there, use that instead (this could be a bit confusing)
D) Always show the file name, and if there is a title, add it in there as well (this could end up being rather long)

Let me know which one you prefer. Thanks.

Could we not do whatever the default action is for displaying the title in the Attachment Details panel? - Which I think corresponds roughly to C). The image or file name minus the .jpg or .pdf etc is the default title, unless it has been updated at some point by the user.

For me this makes the most sense.

@lessbloat
4 years ago

@lessbloat
4 years ago

#12 in reply to: ↑ 11 @lessbloat
4 years ago

  • Keywords needs-testing has-patch added

Replying to grahamarmfield:

Could we not do whatever the default action is for displaying the title in the Attachment Details panel? - Which I think corresponds roughly to C). The image or file name minus the .jpg or .pdf etc is the default title, unless it has been updated at some point by the user.

For me this makes the most sense.

OK, I understand now. 23560.3.diff​ should do it.

#13 @grahamarmfield
4 years ago

Fantastic, thanks.

#14 follow-up: @lessbloat
4 years ago

  • Milestone changed from Awaiting Review to 3.6

#15 in reply to: ↑ 14 ; follow-up: @grahamarmfield
4 years ago

Replying to lessbloat:
Did you apply the latest diff to that test site you had? I'm testing it now and the aria-label value seems to still be always set to the filename and suffix - even when I've added a human-meaningful title, caption, and alt attribute.

#16 in reply to: ↑ 15 ; follow-up: @lessbloat
4 years ago

Replying to grahamarmfield:

Did you apply the latest diff to that test site you had?

I did not. I'm hoping we can just get this committed so that everyone can test it.

#17 in reply to: ↑ 16 @grahamarmfield
4 years ago

Replying to lessbloat:

I did not. I'm hoping we can just get this committed so that everyone can test it.

OK. Thanks for letting me know.

#18 @ticaiolima
4 years ago

Hello guys,

This is my first test with the WordPress source code, and I've tested the 23560.3.diff, and I didn't get some problems solved:

  1. The button "Add Media" isn't being focused. I've tested it in the wp-admin/post-new.php?post_type=page, wp-admin/post-new.php and QuickPress Widget;
  1. I can't select an uploaded media file with the keyboard. The itens aren't being focused;

I've been testing in a MacBook Pro with Mac OSX 10.7 with PHP 5.3.15 and mysql Ver 14.14 Distrib 5.1.58 and the browsers Firefox 19.0.2 and Google Chrome 28.0.1469.0

I'm testing this issue in a right way? Sorry if I'm doing something wrong.
Thank you.

#19 follow-up: @ocean90
4 years ago

  • Milestone changed from 3.6 to Future Release

#20 in reply to: ↑ 19 ; follow-up: @grahamarmfield
4 years ago

Replying to ocean90:
Not sure why this has been dropped out of 3.6, I thought the changes made by @lessbloat were committed. Members of the make.wordpress.org/accessibility group acknowledge that the changes made so far are not complete, but they do enable users of more modern screen readers to actually add some media - a significant step forward from the 3.5 version which is unusable by anyone who isn't using a mouse.

More work to build on @lessbloat's solution will need to be done in 3.7.

I would urge that this change be reinstated in 3.6.

#21 in reply to: ↑ 20 @sharonaustin
4 years ago

Replying to grahamarmfield:

Replying to ocean90:
Not sure why this has been dropped out of 3.6, I thought the changes made by @lessbloat were committed. Members of the make.wordpress.org/accessibility group acknowledge that the changes made so far are not complete, but they do enable users of more modern screen readers to actually add some media - a significant step forward from the 3.5 version which is unusable by anyone who isn't using a mouse.

More work to build on @lessbloat's solution will need to be done in 3.7.

I would urge that this change be reinstated in 3.6.

+1 on reinstating this change in 3.6 if at all possible.

#22 @esmi
4 years ago

Is a reason why lessbloat's patch wasn't committed?

@atimmer
4 years ago

#24 @grahamarmfield
4 years ago

Patch tested in following browsers on Windows 7:

  • Chrome 31
  • Firefox 25
  • IE10

It is possible to tab to all the elements in the media file list. And they can be selected and deselected using the space bar. This is a good result for sighted users who rely on keyboard interaction. Focus visibility is the same as browser defaults.

Additionally, the functionality is enhanced when using screen readers - NVDA 2013.2 and Jaws 14. The screen reader announces the media file's title and whether or not the element is selected.

This solution is a pragmatic solution and will not suit users with some older versions of screen readers. But the situation is a lot better than the current keyboard inaccessible add media panel.

Further enhancements and issues about tab order in other areas will be handled in separate trac tickets.

The patch needs a refresh.

#25 @grahamarmfield
4 years ago

  • Keywords needs-refresh added; needs-testing removed

#26 follow-up: @neil_pie
4 years ago

There appears to be an issue with the Featured Image view in the current patch ( 23560.4.diff ):

When selecting an image either via keyboard or mouseclick, the focus drops from the selected element and onto the Body element. This is a problem for users navigating the pane via the keyboard as it means that pressing tab/ shit+tab does not transfer focus to the next / previous item in the list as one would expect.

Steps to replicate:

  1. Click on 'Add Media' in the edit post screen
  2. Navigate to the 'Set Featured Image' view
  3. Select an image either by tab/space or by mouseclick
  4. Press tab

This is happening for me in the latest Chrome, Safari and Firefox on OSX Mavericks.

Note that in the two webkit browsers, the bug is far more apparent. This is because when the focus returns to the body, pressing tab moves focus onto the skiplinks in the main window. When replicating the bug in Firefox, pressing tab after selecting an item moves focus to the second media item in the list or the next focusable element when there is no second media item.

Some debugging info:

The focus dropoff happens when the media items collection is destroyed and rebuilt in backbone. This disposal doesn't seem to happen in the other two views which is why the issue is isolated to Set Featured Image.

Possible solutions:

  1. Re-apply focus once the collection has been rebuilt
  2. Prevent disposal in the first place if it can be avoided

I'm not sure which of the above two solutions is preferable or even possible, but should I figure it out I'll submit a patch

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

#27 in reply to: ↑ 26 @grahamarmfield
4 years ago

Replying to neil_pie:

There appears to be an issue with the Featured Image view in the current patch ( 23560.4.diff ):

When selecting an image either via keyboard or mouseclick, the focus drops from the selected element and onto the Body element. This is a problem for users navigating the pane via the keyboard as it means that pressing tab/ shit+tab does not transfer focus to the next / previous item in the list as one would expect.

Is this an issue with the patch itself, or just what actually happened before?

@lessbloat
4 years ago

#28 @lessbloat
4 years ago

  • Keywords needs-testing added; needs-refresh removed
  • Milestone changed from Future Release to 3.8

23560.5.diff​:

  • starts with a refresh of .3
  • Fixes featured image loss of focus bug mentioned by neil_pie
  • Removes wp.media.view.FocusManager as much of it appeared to be unused/unnecessary
  • Adds additional keyboard support for various actions within the media modal (moving to gallery, canceling gallery, clearing images, deleting an image)

If you spot any bugs, please ping me on IRC, or make a note on this ticket. Hoping to see this make 3.8.

#29 @nacin
4 years ago

I would love for this to be in 3.8 but this looks like a ton of code churn.

Is there a downside to keeping wp.media.view.FocusManager? That would allay a lot of concerns, I think. "much of it appeared to be unused" is not particularly comforting. :-) It is also more or less public API, so I wouldn't want to be accidentally breaking anything. Could you go into more detail as to how it appears to be unnecessary?

I don't think jQuery UI is used here at all, so we'll have to use 9 instead of $.ui.keyCode.TAB. To be honest, not sure why $.ui.keyCode is ever much benefit as long as there is a comment describing the character being intercepted.

I think prop needs to be used instead of attr for aria-checked.

Is there a reason why we are focusing on media-modal-close to keep the focus inside the media modal? Where does the focus go, otherwise? It seemed that this is what the focusManager should be tasked with doing. Keeping it and decorating it with this kind of stuff might be better than axing it.

What is the isIE class meant to target? All IE, including IE10? IE11?

#30 follow-up: @nacin
4 years ago

  • Milestone changed from 3.8 to Future Release

This wasn't slated for 3.8 until yesterday; bumping.

@lessbloat
4 years ago

#31 in reply to: ↑ 30 ; follow-up: @lessbloat
4 years ago

Replying to nacin:

Is there a downside to keeping wp.media.view.FocusManager? That would allay a lot of concerns, I think.

Brought the main media.view.FocusManager structure back in .6.

Could you go into more detail as to how it appears to be unnecessary?

recordTab & updateIndex attempt to track the current tabindex. From my perspective, this is overkill, as it's not really necessary to make this all work.

I don't think jQuery UI is used here at all, so we'll have to use 9 instead of $.ui.keyCode.TAB. To be honest, not sure why $.ui.keyCode is ever much benefit as long as there is a comment describing the character being intercepted.

Cool. I removed all references to $.ui.

I think prop needs to be used instead of attr for aria-checked.

Nice catch. Fixed these.

Is there a reason why we are focusing on media-modal-close to keep the focus inside the media modal? Where does the focus go, otherwise? It seemed that this is what the focusManager should be tasked with doing. Keeping it and decorating it with this kind of stuff might be better than axing it.

Done. FocusManager.focus() now puts the focus on the first left menu item instead calling media-modal-close directly.

What is the isIE class meant to target? All IE, including IE10? IE11?

Currently downloading a Win 8 ISO to check it in IE10, IE11. It's definitely needed <=IE8.

This wasn't slated for 3.8 until yesterday; bumping.

Sorry, my fault. I had no idea that code freeze was so close. :-( 3.9 it is...

#32 in reply to: ↑ 31 @grahamarmfield
4 years ago

Replying to lessbloat:

This wasn't slated for 3.8 until yesterday; bumping.

Sorry, my fault. I had no idea that code freeze was so close. :-( 3.9 it is...

I will attempt to test this patch tomorrow - thanks for the input.

@lessbloat
4 years ago

#33 @lessbloat
4 years ago

23560.7.diff uses LTE IE 8 conditional for IE specific CSS.

#34 follow-up: @grahamarmfield
4 years ago

Patches 23560.6 and 23560.7 are no longer adding the aria-label="$title" and aria-checked="false" attributes into the <li> that contains the media file. So although focus is able to rest on the individual items, screen readers have nothing to voice.

In these two patches, the command to add the attribute takes the form this.$el.prop('aria-label'... whereas in 23560.5 the command is this.$el.attr('aria-label'... which does add the attributes successfully.

If you can provide a patch that combines the various focus updates in .6 and .7 along with the attribute setting from .5 then we may have something that ticks all the boxes.

@lessbloat
4 years ago

#35 in reply to: ↑ 34 @lessbloat
4 years ago

Replying to grahamarmfield:

If you can provide a patch that combines the various focus updates in .6 and .7 along with the attribute setting from .5 then we may have something that ticks all the boxes.

23560.8.diff​ takes us back to using .attr() for aria-label values.

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

#36 @grahamarmfield
4 years ago

Testing 23560.8 with JAWS14 on IE10. All appears to behave as expected, including seemingly a fix for #25100. I can action the Add Media button and focus is in the modal popup. I can then tab around within the media files and JAWS announces the media file name. I can select/deselect using the space key. The functionality to add a gallery and to select the featured image for a post all appears to work.

So that's good. Testing with NVDA to follow after reboot.

#37 @grahamarmfield
4 years ago

Currently unable to test with NVDA at the moment. Be good if someone else could give that a go.

#38 @sharonaustin
4 years ago

  • Cc sharonaustin added

#39 @nacin
3 years ago

  • Component changed from Accessibility to Media
  • Focuses accessibility added

This ticket was mentioned in IRC in #wordpress-ui by grahamarmfield. View the logs.


3 years ago

#41 @GrahamArmfield
3 years ago

  • Keywords needs-refresh added

The patch needs a refresh before further testing can be done.

@bramd
3 years ago

Refresh of 23560.8.diff

@bramd
3 years ago

Refresh of 23560.8

#42 @bramd
3 years ago

  • Status changed from new to closed

Please check 23560.9.2.diff, I accidentally uploaded 23560.9.diff which is an incomplete patch.

#43 follow-up: @bramd
3 years ago

  • Status changed from closed to reopened

Whoops, it seems Trac has some accessibility issues as well. Second time that I as a screenreader user closed a ticket without wanting to do so...

#44 in reply to: ↑ 43 @helen
3 years ago

Replying to bramd:

Whoops, it seems Trac has some accessibility issues as well. Second time that I as a screenreader user closed a ticket without wanting to do so...

I've done the same during accessibility testing - reported it on Meta Trac :) https://meta.trac.wordpress.org/ticket/273#comment:5. Feel free to add any other issues you come across there - it's about general accessibility problems with Trac.

This ticket was mentioned in IRC in #wordpress-ui by grahamarmfield. View the logs.


3 years ago

#46 @sharonaustin
3 years ago

By way of follow-up to the promised review from last week's meeting https://irclogs.wordpress.org/chanlog.php?channel=wordpress-ui&day=2014-02-19&sort=asc#m159871and Graham Armfield's recommendation to try using NVDA 2013.3, I again tested on a 3.8.1 test site, on a Windows machine, going out of my way to test on Firefox, which has had known issues.

Using NVDA 2013.3 in browse mode, I found no accessibility blockers, even in the rich text area using Firefox. Using Version 2013.3 I was able to escape out of keyboard traps using (Shift+).

Bottom line, I found absolutely no blockers because of WordPress, at least when using NVDA 2013.3. Some accessibility blockers (keyboard traps) still exist with 2013.1, but even those disappear with testing in my 3.9 test site, I assume because of the upgraded TinyMCE.

The only problems I REALLY had is that NVDA 2013.3 kept crashing my station; for that reason, I did not test on Rian's site, I tested my own. It was all I could do to make it through a complete session. 2013.3 is in beta (as I understand it) so I expect even those "bugs" to be worked out soon. But again, I found no issues because of WordPress.

To all the developers, thank you so much for what you've done here.

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

#47 @sharonaustin
3 years ago

Sorry, another relevant link to above:
http://community.nvda-project.org/ticket/3145

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

This ticket was mentioned in IRC in #wordpress-ui by grahamarmfield. View the logs.


3 years ago

This ticket was mentioned in IRC in #wordpress-ui by davidakennedy. View the logs.


3 years ago

#50 @sharonaustin
3 years ago

Is it possible to get this refreshed? I would like to revisit this with a fresh test, on trunk. Thank you in advance for any help (or advice) you can provide.

This ticket was mentioned in IRC in #wordpress-ui by ericandrewlewis. View the logs.


3 years ago

This ticket was mentioned in IRC in #wordpress-dev by ericandrewlewis. View the logs.


3 years ago

#53 @ocean90
3 years ago

  • Milestone changed from Future Release to 4.0

Before ircbot takes over this ticket, let's fixed it now.

#54 @sharonaustin
3 years ago

Stunning work by the developers. A huge, huge thanks for the incredible work they've done with this.

Latest test was performed using a version of trunk 18512 using the instructions found here:
http://make.wordpress.org/core/handbook/installing-wordpress-locally/installing-via-svn/
The files were uploaded to this location, where testing took place using NVDA 2013.3 on a Windows machine using Chrome.
http://red-hound.com/tic23560/

There was nothing that was not available to me via keyboard. This is a major, huge improvement over previous versions.

One "headache" was the Add Media feature. It's a modal, and at least in my testing, when trying to add images while looking at the modal screen for Add Media, NVDA would read all of the links in the "background" for the post before reading aloud anything in the Add Media screen. Is it possible to somehow keep focus within the modal when one is working with it?
(Not that we are unappreciative of the improvements made to date...that the Add Media screen works is a major accomplishment! Thank you!)

While everything is accessible by keyboard, not everything is announced. For example, before even logging in, just reading the splash page of the test site, one is taken through various lists on the sidebar, but there is no announcement as to what those lists are other than "complimentary landmark heading" (instead of, for example, "Recent Comments", "Archives" or "Meta"). Another example: While in the "Edit Image" modal of the "Add Media" pane, I was able to navigate by keyboard to the "Image Crop" link, and it was announced as a "link" in NVDA, but there was no indication as to what that link was.

As I see it, the major issues for improvement to date are: keeping focus within modals while working inside of them, and adding labels to links so that screen readers can announce what the links actually are. Most of the links are labeled, (and therefore announced by NVDA), so I personally would not consider these issues showstoppers.

Thanks again for such terrific work by all the developers.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


3 years ago

This ticket was mentioned in IRC in #wordpress-ui by helen. View the logs.


3 years ago

@helen
3 years ago

#57 @helen
3 years ago

In 28607:

At long last, improved keyboard accessibility for the media modal.

props lessbloat, grahamarmfield, sharonaustin, bramd.
see #23560.

#58 @helen
3 years ago

  • Keywords has-patch needs-testing needs-refresh removed

Ticket is still open, as I believe there are a few more things that could be made even better. We can do screen reader improvements here.

I noticed one thing - the .attachment.selected and .attachment.details items (that is, selected items, the former only actually showing up when multiple items are selected) use box shadow styling and so don't have focus styles. I tried giving them the same focus style as normal but it looked wrong, and negatively affected the visual styling when clicking.

#59 @helen
3 years ago

In 28617:

Fix a jshint error introduced in [28607]. see #23560.

#60 follow-up: @helen
3 years ago

Other observations, please let me know if they are correct:

  • When selecting a tab along the left side, focus should be transferred into the main panel. Edit: #25101.
  • When selecting an image, focus should be transferred to the right details panel.
  • Reverse tabbing out of the right details panel seems like it should take me back to the item in the list that was being edited.
  • When in the media list, arrow keys should move between items, not scroll the page.
Last edited 3 years ago by helen (previous) (diff)

#61 @joedolson
3 years ago

I agree with all those observations; they seem like sensible changes to me.

I've noticed a few odd behaviors in Chrome - when NVDA is enabled, I can't access the modal with the keyboard; when NVDA is disabled, it's not a problem. When selecting an image, spacebar jumps to bottom of screen after selecting image.

Chrome is definitely not going to give the best experience with NVDA, so while that's odd, it may not be readily fixable; but the keyboard behavior on spacebar should be fixable.

Keyboard focus is *really* subtle; that could definitely use some improvement.

This ticket was mentioned in IRC in #wordpress-ui by helen. View the logs.


3 years ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


3 years ago

This ticket was mentioned in IRC in #wordpress-ui by _Redd. View the logs.


3 years ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


3 years ago

#66 @helen
3 years ago

#28704 was opened for arrow key navigation, in case we need to punt that piece to the future (don't want to, for the record). Trying to get developer interest here, since tasks are laid out.

This ticket was mentioned in IRC in #wordpress-ui by accessiblejoe. View the logs.


3 years ago

#68 in reply to: ↑ 60 ; follow-up: @lessbloat
3 years ago

Replying to helen:

Other observations, please let me know if they are correct:

  • When selecting a tab along the left side, focus should be transferred into the main panel. Edit: #25101.
  • When selecting an image, focus should be transferred to the right details panel.
  • Reverse tabbing out of the right details panel seems like it should take me back to the item in the list that was being edited.
  • When in the media list, arrow keys should move between items, not scroll the page.
  • 23560-left-side-select.diff - “When selecting a tab along the left side, focus should be transferred into the main panel.”
  • 23560-image-selected.diff - “When selecting an image, focus should be transferred to the right details panel.”
  • 23560-reverse-tabbing.diff - “Reverse tabbing out of the right details panel seems like it should take me back to the item in the list that was being edited.”
  • 23560-arrow-keys.diff - “When in the media list, arrow keys should move between items, not scroll the page.”

This ticket was mentioned in IRC in #wordpress-ui by philipjohn. View the logs.


3 years ago

#70 @joedolson
3 years ago

Additional observations: When creating a Gallery using the keyboard, it's possible to navigate to the selected images at the bottom of the modal (thumbnails next to "n selected"), but they have no announced text, no hover state, and no focus state.

#71 in reply to: ↑ 68 @ericlewis
3 years ago

Replying to lessbloat:

We should be aware if we add this to media.view.MenuItem._click, media.view.RouterItem also extends from media.view.MenuItem, so clicking router links (Upload Files, Media Library) will also get this functionality.

Also, a more solid way to select a region's view's jQuery element is this.controller.content.get().$el

@ericlewis
3 years ago

#72 @ericlewis
3 years ago

Not sure what the intent was with the code in the else block here:

if ( 27 === event.which && this.$el.is(':visible') ) {
        this.escape();
        event.stopImmediatePropagation();
} else {
        // Keep focus inside the media modal
        this.focusManager;
}

but it doesn't invoke anything, just references the focusManager property. attachment:23560.11.diff removes it.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


3 years ago

@celloexpressions
3 years ago

merge previous outstanding patches, selection focus, initial focus fix

#74 @celloexpressions
3 years ago

  • Keywords has-patch needs-testing added

23560.12.diff merges lessbloat's four recent patches and ericlewis' patch along with doing the following:

  • Initial focus is invisible (the modal container div receives focus) - set it to the close button which is the first visible, focusable element in the panel. Fixes the last of #25100, I believe, as well as a bunch of bugs resulting from the previous approach.
  • Add focus states to "n Selected" thumbnails (selecting them bring them up in attachment details).

All of the issues from 68 are fixed, including #25101 and #28704.

Remaining issues:

  • Focus gets lost outside the modal when entering and exiting image-edit mode (which is only partially keyboard-accessible), when entering and exiting edit-selection mode (edit link next to "n selected", and probably any time the contents of the modal are changed.
  • Focus is lost outside the modal after uploading a file.
  • Collections cannot be re-ordered with the keyboard. Best solution would probably be to use the arrows for moving items and tab for changing the selected one to move. This would be awesome to fix in 4.0, but probably low priority unless someone does a patch soon.

This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.


3 years ago

@celloexpressions
3 years ago

Focus states for selected (but inactive) attachments, ability to remove items from the selection.

#76 @celloexpressions
3 years ago

Also, no way to remove items from the selection using keyboard. And no focus states on selected items. 23560.13.diff fixes both issues. Only remaining bug with it appears to be that you can't un-select the attachment that's currently active (it's details are in the sidebar), which isn't a big issue right now.

@celloexpressions
3 years ago

Set focus as if a newly uploaded attachment was selected (which it is, automatically), when uploading.

@celloexpressions
3 years ago

focus first input in sidebar, not first focusable element

#77 @helen
3 years ago

In 29220:

Keyboard accessibility for the media modal:

  • Arrow keys navigate between items in the grid.
  • Transfer focus into the panel when selecting a tab along the side.
  • Transfer focus into the details sidebar when selecting an item and vice versa.
  • Set initial focus on the close button so that it is visible.

props celloexpressions, lessbloat, ericlewis. fixes #25100, #25101, #28704. see #23560.

#78 follow-ups: @celloexpressions
3 years ago

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

Known issues as of [29220]:

  • Focus is lost going into/out of image edit mode.
  • Focus is lost going into/out of edit-selection mode.
  • Can't remove items from edit-selection mode, which is the main point of edit-selection mode.
  • Can't re-order collection items. Could do some awesomeness with arrow keys, but we could also leave this out for now.
  • IE11 and Firefox: this breaks using "enter" to open an item's details in the media grid (media library). But using spacebar still works. Unsure why.
  • IE11: back-tabbing out of the details sidebar doesn't return focus to the active item's tile.

#79 follow-up: @celloexpressions
3 years ago

Also:

  • When navigating between media-grid item detail views (for example, via the arrow keys), focus should be set to the appropriate arrow key, not the close button. That matches the behavior of the theme-details modal and theme-install, and is less confusing given the bold focus styling on the arrows and close button.

#80 @helen
3 years ago

#28969 was marked as a duplicate.

#81 in reply to: ↑ 79 ; follow-up: @helen
3 years ago

Replying to celloexpressions:

  • When navigating between media-grid item detail views (for example, via the arrow keys), focus should be set to the appropriate arrow key, not the close button.

Reported in #28969 (closed as dupe).

#82 in reply to: ↑ 81 @adamsilverstein
3 years ago

That makes sense.

Where should the focus go when opening the modal initially? Close button isn't great, especially with the focus highlighting in css.

Replying to helen:

Replying to celloexpressions:

  • When navigating between media-grid item detail views (for example, via the arrow keys), focus should be set to the appropriate arrow key, not the close button.

Reported in #28969 (closed as dupe).

#83 @joedolson
3 years ago

The close button is a good place for initial focus; it allows the quickest possible correction from opening the modal in error.

If there was a single action that could be performed by the modal, so that you could logically move focus to the place where you perform that single action, that would be preferable, but there's so much going on and possible to do in the add media panel that the close button is a better place to start.

@celloexpressions
3 years ago

Key [29220] to color schemes where appropriate.

#84 @celloexpressions
3 years ago

23560.colors.diff keys [29220] to color schemes where necessary to match existing styles, but not where we use the generic blue outline focus indicator.

#85 follow-up: @lancewillett
3 years ago

Sharing feedback sent in by a screen reader user and a11y consultant Jose María Ortiz (https://twitter.com/jmortizsilva). He sent to me via email (in Spanish), I'm translating here into English as he'd like to contribute his notes.


I’ve tested the Media Grid "instert existing items to a post" workflow in Safari 4.0 on Mac and IE, Firefox, and Chrome on Windows with various screen readers: VoiceOver 6.0 (333.11), Jaws 15, and NVDA 2014.2.

In all cases I’m unable to select an image in the Media Grid without using a mouse. In both Safari and Firefox I can see that checkboxes are there to select, but I cannot use the keyboard to actually check the boxes, I have to use a mouse.

In IE11 the screen reader doesn’t even detect the checkboxes, the only thing it sees are the links to “Deselect”, so it’s impossible to select an image.


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

#86 in reply to: ↑ 78 ; follow-up: @ericlewis
3 years ago

Replying to celloexpressions:

  • Focus is lost going into/out of image edit mode.

Where should focus land when entering image edit mode? Also, are keyboard users going to be able to use the image editor?

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

@ericlewis
3 years ago

#87 in reply to: ↑ 78 @ericlewis
3 years ago

Replying to celloexpressions:

  • IE11 and Firefox: this breaks using "enter" to open an item's details in the media grid (media library). But using spacebar still works. Unsure why.

In attachment:23560.16.diff, use the normalized property event.which everywhere, a cross-browser normalized version of keyCode (see jQuery event object docs)

#88 @ericlewis
3 years ago

  • Keywords has-patch added; needs-patch removed

#89 in reply to: ↑ 78 @ericlewis
3 years ago

Replying to celloexpressions:

  • Can't re-order collection items. Could do some awesomeness with arrow keys, but we could also leave this out for now.

What do you mean here? Media grid lists items reverse cron on post_date by default, there's no way to re-order items.

@ericlewis
3 years ago

#90 @ericlewis
3 years ago

attachment:23560.17.diff is follow-up to attachment:23560.16.diff without the debugger statement.

@ericlewis
3 years ago

#91 in reply to: ↑ 85 @ericlewis
3 years ago

Replying to lancewillett:

I’m unable to select an image in the Media Grid without using a mouse. In both Safari and Firefox I can see that checkboxes are there to select, but I cannot use the keyboard to actually check the boxes, I have to use a mouse.

attachment:23560.18.diff allows you to deselect by keyboard navigation. Needs some more work.

#92 in reply to: ↑ 86 @celloexpressions
3 years ago

Replying to ericlewis:

Replying to celloexpressions:

  • Focus is lost going into/out of image edit mode.

Where should focus land when entering image edit mode? Also, are keyboard users going to be able to use the image editor?

It's not particularly usable, but focus should at least stay in the modal for now. Probably either the close button or the back button.

  • Can't re-order collection items. Could do some awesomeness with arrow keys, but we could also leave this out for now.

What do you mean here? Media grid lists items reverse cron on post_date by default, there's no way to re-order items.

This would be when editing galleries, playlists, etc. in the modal.

This ticket was mentioned in IRC in #wordpress-ui by davidakennedy. View the logs.


3 years ago

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.


3 years ago

#95 @celloexpressions
3 years ago

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

Current Status:

Needs patch:

  • Focus is lost going into/out of image edit mode. needs patch to set focus to close or back button
  • Focus is lost going into/out of edit-selection mode. needs patch. this is accessed in the bottom toolbar when multiple items are selected, next to "clear"
  • Can't remove items from edit-selection mode, which is the main point of edit-selection mode. needs patch, probably make the close icons have a tabindex and focus styling and always be visible
  • Can't re-order collection items. Could do some awesomeness with arrow keys, but we could also leave this out for now. needs patch or decision to punt (makes galleries and playlists not very usable with the keyboard, using arrows would offer a better experience than drag and drop for a lot of users who know about it)
  • IE11: back-tabbing out of the details sidebar doesn't return focus to the active item's tile.needs patch
  • When navigating between media-grid item detail views (for example, via the arrow keys), focus should be set to the appropriate arrow key, not the close button. That matches the behavior of the theme-details modal and theme-install, and is less confusing given the bold focus styling on the arrows and close button. needs patch

Has patch, needs testing:

  • IE11 and Firefox: this breaks using "enter" to open an item's details in the media grid (media library). But using spacebar still works. Unsure why. 23560.17.diff should fix this, needs testing
  • Broken colorschemes: 23560.colors.diff should fix it, although a recent mobile-media commit appeared to touch something similar, so needs testing.

Waiting for other stuff to happen:

  • Image selection in media grid: since that's changing again, not sure whether or not 23560.18.diff or something else will be needed.

This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.


3 years ago

#97 @celloexpressions
3 years ago

A lot of the things that we fixed here a few weeks ago have been regressed ([29473] fixed one). Most notably, focus is no longer set to the appropriate place (editing details) when selecting an attachment, which is the only way to get to editing details or the primary button without tabbing through the entire media library (including infinite scroll).

I was going to start to break this up into individual tickets for the remaining unpatched items (and test & confirm as I go), but now I'm thinking that we may need to just punt this entirely to Future Release as mobile media and media grid modal work keep inadvertently breaking improvements made here.

I just went through another round of testing, and all of the issues listed in 95 still apply. Additionally, I found the following issues:

  • Can't trigger modal-close with esc. I thought that was functional at one point, and that's one way to work around all of the lost-focus situations if we don't fix them in 4.0.
  • Selected attachments in toolbar don't have focus styling, making it very difficult to figure out where focus is when trying to get past them to the primary action button for the modal. This was originally fixed in [29220] (last hunk of media-views.css in that changeset).
  • Check/unselect icons are focusable but not actionable; pressing enter when the thumbnail is focussed toggles its selection, though.
  • Selected and focused attachments are not keyed to colorschemes. Selected but unfocused attachments display incorrectly. Selected and focused but not active attachments are not keyed to color schemes. See screenshot. This also affects non-keyboard users pretty significantly.

#98 @joedolson
3 years ago

I just want to voice the opinion that I'm very unhappy with the idea of punting accessibility on this ticket yet again. This has been getting punted repeatedly since it was raised, and it's clearly an endemic problem: the accessibility team does not have enough people to continually be verifying that given problems haven't regressed.

I really appreciate those people who are checking this (thank you @celloexpressions!), but I really think it's important that these issues get addressed, not punted.

#99 @toscho
3 years ago

Then let’s put back the code that broke the existing fixes. There is no point making something nice if you cannot use it anyway.

#100 follow-up: @wonderboymusic
3 years ago

Can someone make a patch?

This ticket was mentioned in IRC in #wordpress-ui by davidakennedy. View the logs.


3 years ago

This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.


3 years ago

#103 in reply to: ↑ 100 @adamsilverstein
3 years ago

Will try as soon as time allows, hopefully tonight.

Replying to wonderboymusic:

Can someone make a patch?

@adamsilverstein
3 years ago

focus on close btn when switching to edit image

#104 @adamsilverstein
3 years ago

23560.19.difffocus on close btn when switching to edit image - not sure thats the best spot?

also, not certain which issues above remain.

#106 in reply to: ↑ 105 ; follow-up: @GrahamArmfield
3 years ago

Replying to adamsilverstein:

chrome keyboard tests: https://cloudup.com/cyNqYfMiXbf

Are we perhaps getting mixed up here between the Add Media functionality that was the original subject of this ticket, and the new Media grid being introduced in 4.0 which is the subject of that 'video'.

#107 @celloexpressions
3 years ago

Yes, so this ticket's primary & initial focus is on the add-media modal, but since it's also been adopted for the media library grid now, those issues have been addressed here also (arrow-key-nav we got for free in the library since it's shared code, for example).

All of the issues in comment 95 and 97 still apply. Most are for the library, only a couple are for the grid. 23560.19.diff fixes that first issue for the grid, but not the library. I wonder if there's a spot within the view itself opening/rendering where we could set it, so that it would work for both grid and modal? I'm pretty sure they use the same view.

#108 in reply to: ↑ 106 @adamsilverstein
3 years ago

Yes I was, thanks for setting me straight! Trying again with the media modal :)

Replying to GrahamArmfield:

Replying to adamsilverstein:

chrome keyboard tests: https://cloudup.com/cyNqYfMiXbf

Are we perhaps getting mixed up here between the Add Media functionality that was the original subject of this ticket, and the new Media grid being introduced in 4.0 which is the subject of that 'video'.

@adamsilverstein
3 years ago

media-modal-close focus in setState

#109 @adamsilverstein
3 years ago

In 23560.20.diff - focus the media close button at the end of setState, effectively focusing the close button when opening modal, switching to edit mode, and switching back out of edit mode with either cancel or back action.

This change does not affect the focus in the media grid.

Fixes:

  • Focus is lost going into/out of image edit mode.

@adamsilverstein
3 years ago

focus close btn when switching to image-selection-edit mode and back

#110 follow-up: @adamsilverstein
3 years ago

In 23560.21.diff:
Includes .20 focus, plus, keeps focus on close button when switching into/out of edit selection mode

Fixes:

  • Focus is lost going into/out of edit-selection mode

#111 in reply to: ↑ 110 @adamsilverstein
3 years ago

Regaring this item: "Can't remove items from edit-selection mode" - you can uncheck items by tabbing to the checkbox field and hitting space or enter, am I misunderstanding the issue or is this fixed?

Replying to adamsilverstein:

In 23560.21.diff:
Includes .20 focus, plus, keeps focus on close button when switching into/out of edit selection mode

Fixes:

  • Focus is lost going into/out of edit-selection mode

@adamsilverstein
3 years ago

focus next/back arrows focus when navigating detail media grid w/keyboard

#112 @adamsilverstein
3 years ago

*note: this one is media grid related:

23560.22.diff: standalone, focus on the left or right arrow when keyboard navigating the media grid detail view

addresses this:

  • When navigating between media-grid item detail views (for example, via the arrow keys), focus should be set to the appropriate arrow key, not the close button.

@adamsilverstein
3 years ago

like the last, but prettier

#113 @adamsilverstein
3 years ago

23560.23.diff add some missing whitespace to .22

#114 @adamsilverstein
3 years ago

  • "Can't trigger modal-close with esc" <- this seems to work correctly for me in both firefox and chrome, can you still reproduce?
  • "Selected attachments in toolbar don't have focus styling (This was originally fixed in #29220 )" - are you talking bulk edit mode in the media grid? the styling seems pretty clear here now, even when naving over a selected item; see - https://cloudup.com/cgZ-YMh82Gt

This ticket was mentioned in IRC in #wordpress-dev by ocean90. View the logs.


3 years ago

#116 @adamsilverstein
3 years ago

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

For anyone testing the latest patches, note that you need to apply both 23560.21.diff & 23560.23.diff to get the complete set of fixes for the issues covered in this ticket.

#117 @wonderboymusic
3 years ago

Just noticed the global jQuery selectors from [29220] - these are a no-no. I will review all of those.

This ticket was mentioned in IRC in #wordpress-ui by davidakennedy. View the logs.


3 years ago

#119 @wonderboymusic
3 years ago

In 29555:

Initial cleanup after [29220]:

  • Alter/remove some jQuery global selectors, views should only manage themselves
  • Trigger an event for attachments when arrow keys are pressed: attachment:keydown:arrow
  • media.view.Attachments should listen for attachment:keydown:arrow
  • Favor scoped selectors over globals

See #23560.

#120 @wonderboymusic
3 years ago

In 29556:

Cleanup after [29220]:

  • media.view.Attachment should not reach outside itself with global jQuery selectors
  • media.view.Attachment.Details will re-render when media.view.Attachment's single status changes. Add a ready callback to apply initial input focus

See #23560.

#121 @wonderboymusic
3 years ago

In 29558:

Cleanup after [29220]:

media.controller.Library is State/Backbone Model and should not touch views.

See #23560.

#122 @adamsilverstein
3 years ago

looks like my patches may need a review, taking a look.

@adamsilverstein
3 years ago

refresh and combine previous patches

#123 @adamsilverstein
3 years ago

In 23560.24.diff - refresh:

  • add left/right arrow focus styling
  • focus left/right arrow when keyboarding left/right in media grid image detail screen
  • focus on close icon when going into/out of edit-selection mode - with either cancel or back action (multi item select in insert image modal)

@adamsilverstein
3 years ago

more focused selectors

#124 @adamsilverstein
3 years ago

In 23560.25.diff:

  • more focused selectors for close icon focus, searching from this (or self)... (as mentioned by wonderboymusic in IRC, we can't use selectors like $( '.media-modal' ) ).

#125 @wonderboymusic
3 years ago

In 29560:

Media Grid/Modal Keyboard navigation improvements:

  • Add focus to arrows on Next/Previous in the grid's modal on left/right keypress, and add the necessary CSS for :focus
  • When in a disabled input in the grid modal, allow the left/right keys to work
  • Make the image editor return a $.Deferred so that there isn't a race condition with UI loading.
  • Assign focus when the edit image mode is rendered so that the modal can be closed on Esc press

Props wonderboymusic, adamsilverstein (for the initial patch).
See #23560.

#126 @wonderboymusic
3 years ago

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

This ticket has way too much going on, I almost blacked out several times reading it.

We should do one or both of the following:

  • New patches should only pertain to one issue at a time.
  • New tickets should be created for individual items.

Most of the patches uploaded are too far-reaching and lose sight of the role they are playing. Most of the commits I have done today were cleaning up code that came in off the streets and ended up in places it shouldn't have been.

I'm down to keep slogging through this, but we should take it slow so we can test everything properly.

NOTE:

  • If you are in a class that does not extend Backbone.View, there is little to no chance it should contain a jQuery selector
  • If you are in class that DOES extend Backbone.View, your selectors should probably look like:

this.$( '.yep' )

As we tackle more issues, it should also be clear what is happening as pertains to Grid AND Modal.

#127 @rianrietveld
3 years ago

@wonderboymusic totally agree, we discussed this in the accessibility team IRC chat last wednesday. I think it's best to see which problems are fixed and what stil needs to be done. Then we can make new tickets (one issue at a time) for what's yet to do and close this dragon of a ticket. Not a good thing if devs black out trying to fix stuff.

Next monday at 17:00 -19:30 UTC we've planned a working meeting in #wordpress-ui IRC to sort this out. Everyone is most welcome to help.

#128 @adamsilverstein
3 years ago

Completely agree, this ticket is overwhelming. Thanks for the cleanup and selector notes, points taken :)

The remaining issues I see in testing from the comments above are:

  • Focus is lost going into/out of Edit Image mode. set focus to close or back button - applies to both grid view and insert image modal
  • Focus is lost going into/out of edit-selection mode. this is accessed in the bottom toolbar when multiple items are selected, next to "clear"
  • Can't remove items from edit-selection mode, which is the main point of edit-selection mode. needs patch, probably make the close icons have a tabindex and focus styling and always be visible

Also noticed this:

Can't tab backwards (shift-tab) from Edit Image link to image grid.

I'll give you the patches discretely.

@adamsilverstein
3 years ago

edit-selection mode: make the close icons have a focus styling and always be visible

#129 @helen
3 years ago

  • Keywords needs-testing needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

If you have something outstanding, make a new ticket. I am closing this one and opening new ones - we are in a better place than we were previously, even with frustrations over re-breaking things we've fixed. This ticket is a monster and, like wonderboymusic, I am finding it impossible to parse what's going on. It also makes it harder to punt on specific pieces as needed.

In any case, related: #29303, #29304.

#131 @gcorne
3 years ago

While looking at some other focus-related issues, I came across some code that creates orphaned Backbone instances unnecessarily. Rather than creating new object instances, it would be much better to use the FocusManager instance hanging of the MediaFrame Modal instance. 23560.26.diff addresses this.

@gcorne
3 years ago

@gcorne
3 years ago

@gcorne
3 years ago

@gcorne
3 years ago

#132 @gcorne
3 years ago

23560.29.diff addresses the following:

  1. Adjusts the focus method to target the menu for the current modal
  2. After discussing with @nacin and @helen, we decided not to focus the close link by default because it is visually distracting when not navigating via keyboard. Instead, focus is placed on the modal.
  3. Refactors the FocusManager so that the method names and the functionality matches more closely with how it currently works. The changes in 4.0 to the FocusManager change the purpose of the focus manager entirely — instead of being a way to save and restore focus state, the new FocusManager is about constraining keyboard navigation via the tab key to inside the modal. I don't think that it is very likely that 3rd-party devs are using the constructor, but the potential BC break did cross my mind.

I also have seen an intermittent issue when on the Upload tab in FF where the constrained tabbing is too constrained, which makes it impossible to keyboard navigate via tab to the modal menu. Not sure why/how that happens, but my best guess is that somehow the order of the FF focus ring doesn't match the order of the tabbable elements as returned by jQuery.

#133 @wonderboymusic
3 years ago

In 29602:

Media: cleanup recent changes to media.view.FocusManager.

  • Adjusts the focus method to target the menu for the current modal
  • Refactor the FocusManager so that the method names and the functionality matches more closely with how it currently works. The changes in 4.0 to the FocusManager change the purpose of the focus manager entirely — instead of being a way to save and restore focus state, the new FocusManager is about constraining keyboard navigation via the tab key to inside the modal.

Props gcorne.
See #23560.

#134 @nacin
3 years ago

"Add focus to arrows on Next/Previous in the grid's modal on left/right keypress, and add the necessary CSS for :focus"

When navigating between media-grid item detail views (for example, via the arrow keys), focus should be set to the appropriate arrow key, not the close button. That matches the behavior of the theme-details modal and theme-install, and is less confusing given the bold focus styling on the arrows and close button.

This is ugly, jarring, and feels very unnecessary for accessibility, good UI, good UX, or anything really.

I'm not going to reopen this ticket since it's way too long, but if you open a dialog, the focus probably shouldn't be on a close button, or a navigation button. It should be on whatever the first appropriate element is, which may be but is not likely to be a close or navigation button. Along those lines, hitting left or right shouldn't put focus on the navigation keys — if anything, it makes the screen less accessible as they end up on a link they by their very use of the arrow key they do not need to have focus on.

The theme details dialog does not focus on the close button, or on an arrow, for the record. It focuses on the main element: the Activate button. Since the theme installer is a 100% overlay, it does happen to focus on Close.

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


3 years ago

Note: See TracTickets for help on using tickets.