WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#30468 closed defect (bug) (fixed)

wplink modal accessibility

Reported by: afercia Owned by: azaozz
Milestone: 4.5 Priority: high
Severity: normal Version: 4.0
Component: Editor Keywords: has-patch has-screenshots
Focuses: ui, accessibility, javascript Cc:

Description

After #28897, mainly focused on keyboard accessibility, I'd like to propose a second accessibility round for the wplink modal, this time more focused on screen readers. In the proposed patch:

  • added aria live region, aria roles, aria attributes
  • use role "application" on the link listings to avoid key strokes conflicts with screen readers, focus is handled
  • better focus management
  • avoid to move focus in one potentially confusing case
  • changed two non-links in buttons
  • use CSS class 'modal-open' on the body
  • CSS refinements
  • wplink.js braces

Tested with Firefox + NVDA and (quickly) with Chrome + ChromeVox.

Seems almost everything gets read out nicely. Accessibility Team and everyone interested, please test everything :) Any thoughts more than welcome.

Please remember to test when "link to existing content" is open and:

  • showing recent items (default view)
  • doing a search with results
  • doing a search with no results

Attachments (5)

30468.patch (16.8 KB) - added by afercia 4 years ago.
30468.2.patch (16.1 KB) - added by afercia 4 years ago.
Refreshed patch, also removes the missing semicolons fix already committed in r30550
30468.3.patch (16.2 KB) - added by afercia 4 years ago.
30468.4.patch (16.5 KB) - added by afercia 4 years ago.
30468.5.patch (4.5 KB) - added by afercia 3 years ago.

Download all attachments as: .zip

Change History (35)

@afercia
4 years ago

#1 @afercia
4 years ago

  • Keywords has-patch needs-testing dev-feedback added

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


4 years ago

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


4 years ago

#4 follow-up: @ceo
4 years ago

Tested with VoiceOver in Safari on Yosemite. In the Insert link window, when typing a search, VO will read the results. If the search doesn't have a result, VO applicably announces: "no results found."

FWIW, I can only activate the Insert link window by using VO nav keys to get to the toolbar and then the specific button in the toolbar. Opening the web rotor doesn't show the link buttons, whether they are active or not.

#5 in reply to: ↑ 4 @afercia
4 years ago

Replying to ceo: thanks @ceo

FWIW, I can only activate the Insert link window by using VO nav keys to get to the toolbar and then the specific button in the toolbar. Opening the web rotor doesn't show the link buttons, whether they are active or not.

Once you've selected some text inside the editor, there's a keyboard shortcut Alt+Shift+A to open the link modal. Also Ctrl+K (on Windows) should work.

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


4 years ago

#7 @afercia
4 years ago

Meanwhile, the modal-open CSS class was addressed in 30707 so this patch needs a refresh.

@afercia
4 years ago

Refreshed patch, also removes the missing semicolons fix already committed in r30550

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


4 years ago

#9 @iseulde
4 years ago

  • Component changed from TinyMCE to Editor
  • Summary changed from TinyMCE wplink modal accessibility to wplink modal accessibility

#10 @iseulde
4 years ago

  • Keywords needs-refresh added; has-patch removed
  • Milestone changed from Awaiting Review to 4.2

Let's look at this together with the other wpLink improvements. Needs a refresh and will need a refresh again once the enhancements have been added.

#11 @DrewAPicture
4 years ago

  • Priority changed from normal to high

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


4 years ago

#13 @afercia
4 years ago

Reading this patch after some months :) I'm not sure the ARIA roles used here are 100% correct, where role=list and role=listitem are used, they should probably be role=listbox and role=option in order to correctly apply the aria-selected property, see http://www.w3.org/TR/wai-aria/states_and_properties#aria-selected Would love some feedback from experts.

Also as @iseulde pointed out, wpLink is under revision for other improvements, it would be nice to integrate in the new version the parts of this patch that are still applicable.

#14 @DrewAPicture
4 years ago

  • Keywords punt added

@afercia: Do you know what the accessibility status is of the new iteration of the wpLink modal?

What do we need to accomplish here to consider this a win for 4.2? Would it be better to wait and try to tackle this in a future release? Getting pretty close to the point where this will need to be punted.

#15 @afercia
4 years ago

@drew didn't have the chance to check the new iteration of the wpLink modal, will try today otherwise agreed, punt :)

#16 @afercia
4 years ago

  • Keywords needs-refresh punt removed

Updated patch for the new iteration of the wpLink modal.

  • re-introduce all the a11y features from previous patch
  • some non strictly related JS refactoring (braces) sorry for that, had to run and quickly merged the previous patch :)
  • CSS adjustments for the spinner position
  • please check the modal height on small screens, might need some tweaking

For accessibility reasons, the first input field should have initial focus. Please consider to never skip a form field thus forcing users to reverse tabbing to "discover" there's something else there. Will work correctly when empty of course and when there's some selected text it will be announced as "selected {text}".

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

@afercia
4 years ago

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


4 years ago

#18 @afercia
4 years ago

Refreshed patch to try to fix some CSS issues on small screens, especially with languages with long strings e.g. Italian. See results in the screenshots below:

https://cldup.com/HjOBYipi9W.png

https://cldup.com/Kuq3erICTt.png

https://cldup.com/tQH3qj-N-E.png

Btw, on small screens maybe labels and fields shouldn't be aligned in one line...

@afercia
4 years ago

#19 @helen
4 years ago

  • Milestone changed from 4.2 to Future Release

Punting this, but going to reverse the URL/link text fields so that URL is always first and always gets focus.

Note on the patch: spinners got tweaked recently, and I am a no on that internal linking toggle looking like a button. Too visually heavy.

#20 @afercia
4 years ago

For reference: we started discussion about "buttons should be buttons" several weeks ago, and the "toggle" here is a classical case: it should be marked up as a button. The missing part is the design/UI decision that should be made for such buttons. At some point it was agreed to start the switch to buttons, and meanwhile some UI decision was supposed to happen, see final discussion on Slack, Feb 18th: https://wordpress.slack.com/archives/core/p1424298161005399 I'm totally fine with punting this, but please can we have some decision for 4.3 early? Please :) Worth noting Press This already started using <button> elements with a "light" style that makes them look like links, see the "Save Draft" and "Preview" buttons.

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


4 years ago

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


3 years ago

#23 @michaelarestad
3 years ago

Btw, on small screens maybe labels and fields shouldn't be aligned in one line...

I don't think they should be on one line for any size screen.

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


3 years ago

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


3 years ago

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


3 years ago

@afercia
3 years ago

#27 @afercia
3 years ago

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

Many things have changed in the link modal dialog, see #33301. It is now greatly simplified and that makes things easier for accessibility too. Now that #33301 is going to be shipped with WordPress 4.5 I'd recommend the refreshed patch for commit consideration. It does a few, simple, things:

  • adds a role="dialog" and an aria-labelledby attribute to the modal container
  • changes the modal title in a h1
  • removes one &nbsp; that gets read out as "blank" by screen readers
  • changes the "Cancel" link in a button
  • removes some no more used CSS

Screenshot before and after the patch:

https://cldup.com/2xBoKzhdMa.png

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


3 years ago

#29 @azaozz
3 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 36991:

wpLink: fix accessibility issues:

  • Add role=dialog and an aria-labelledby attribute to the modal container.
  • Change the modal title to h1.
  • Remove one &nbsp; that gets read out as blank by screen readers.
  • Replace the Cancel link with a button.
  • Clean up unused CSS.

Props afercia.
Fixes #30468.

#30 @azaozz
3 years ago

  • Milestone changed from Future Release to 4.5
Note: See TracTickets for help on using tickets.