WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#29958 closed defect (bug) (fixed)

collapse menu keyboard accessibility

Reported by: afercia Owned by: jorbin
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.0
Component: Administration Keywords: has-patch
Focuses: ui, accessibility, javascript Cc:

Description

See also related #29955 point 1.

The "Collapse menu" item is not focusable and keyboard users can't collapse/expand the admin menu.

Some screen readers (tested with Firefox + NVDA) may announce it as "clickable" and you can activate it but it's still not focusable: you can't get to it using the Tab key, but only using the Arrow up and Arrow down keys.
Regardless, please consider that this issue is not just for screen reader users, they may found useless to collapse/expand the menu, but it's for all keyboard users: all the people who can't or don't want to use a pointing device. It would be nice to offer them a little improvement.

The best option here is to use a button element which natively supports mouse and keyboard interaction

Quoting @joedolson at WordCamp Chicago 2014:

"If it's supposed to act like a button, it should be a button."

:) Buttons are mentioned also in the accessibility guidelines for themes

  • proposed patch also adds a hidden text "Expand menu" that is available just for screen readers when the menu is folded or auto-folded.
  • about colors on hover and focus, tries to take into account the alternative color schemes which currently are a bit inconsistent, but this part is optional; which color should be used?

In the screenshot below, see the current color on hover/focus in different color schemes: should use the submenu items color or its own color?

https://cldup.com/7zgMnvej9i.png

Needs testing, especially in IE9+.
Any thoughts welcome, especially from the accessibility team and mailing list.

Attachments (7)

29958.patch (7.0 KB) - added by afercia 5 years ago.
29958.2.patch (6.9 KB) - added by afercia 5 years ago.
29958-3.patch (5.5 KB) - added by ipm-frommen 5 years ago.
29958.4.patch (6.9 KB) - added by afercia 4 years ago.
29958.5.diff (8.6 KB) - added by valendesigns 4 years ago.
29958.6.diff (9.9 KB) - added by valendesigns 4 years ago.
29958.diff (12.6 KB) - added by afercia 3 years ago.

Download all attachments as: .zip

Change History (57)

@afercia
5 years ago

#1 @afercia
5 years ago

  • Keywords has-patch needs-testing added

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


5 years ago

#3 @joedolson
5 years ago

I think somebody else needs to sound in on the color choice question; but as is would be fine by me. This looks like a good patch. I haven't applied it, so I can't verify whether it needs a refresh.

#4 @helen
5 years ago

Should probably use the submenu color for both hover and focus - not a fan of it just going white in the color schemes, even I can barely see that.

#5 @afercia
5 years ago

Thanks @helen and @joedolson, will refresh the patch and take care of the SASS part too.

#6 @afercia
5 years ago

Refreshed patch:

  • better styles
  • new ARIA attributes, now with aria-live

Accessibility team please check: the button text is now announced each time the button gets activated. Not sure if the wording can be confusing, let me know if it makes sense.
Perhaps blind persons may not find so useful having a feedback about the menu status or even collapsing/expanding the menu, anyway they're not the only screen reader users.

@afercia
5 years ago

#7 @afercia
5 years ago

Still needs testing, especially in IE9+ :)

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


5 years ago

#9 @joedolson
5 years ago

The text on those sounds good to me; I don't think it's confusing at all. This should definitely go in soon.

#10 follow-up: @SergeyBiryukov
5 years ago

Is there a reason to change 180deg to 180.001deg?

#11 @SergeyBiryukov
5 years ago

  • Component changed from Menus to Administration

#12 in reply to: ↑ 10 @afercia
5 years ago

Replying to SergeyBiryukov:

Is there a reason to change 180deg to 180.001deg?

Trying to fix lack of antialiasing for transformed layers in Firefox during CSS transitions.

The rotate animation is a bit weird, at least on Windows. You can easily notice this in the credits page where the menu items don't show sub items. With Firefox, when you collapse/expand the menu: the icon should be at the same vertical position but it moves 1 pixel up and down instead.
Maybe @michaelarestad could help here, noticed he knows a lot about this issue.

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


5 years ago

#14 @helen
5 years ago

#31272 was marked as a duplicate.

@ipm-frommen
5 years ago

#15 follow-up: @ipm-frommen
5 years ago

  • Focuses administration added
  • Keywords 2nd-opinion added

29958-3.patch, for which I originally created a now closed ticket, takes care of making the Collapse menu menu item accessible/focusable via keyboard.

Thre is no CSS overload as the patch makes use of the generic dashicons and menu styles, and there is no JS tweaking whatsoever.

#16 @ocean90
5 years ago

  • Milestone changed from Awaiting Review to 4.2

#17 in reply to: ↑ 15 @afercia
5 years ago

Replying to ipm-frommen:
Non-link lins href="#" should not be used for accessibility and semantics reasons, that's why the previous patch uses a <button>. There are plans to remove all non-links from all the admin, see #26504 and I would recommend to don't introduce new ones.

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


4 years ago

#19 @DrewAPicture
4 years ago

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

The recommendation stands for the latest patch to be reworked to use a <button> element instead of an empty link. See comment:17.

#20 @afercia
4 years ago

Latest patch doesn't remove the title attribute and doesn't use screen-reader-text for screen reader users. Also, when collapsed. screen readers will still read out 'Collapse menu'. Also, looks like it's a "differential" patch :) as far as I see, it applies "on top" of 29958.2.patch

Thre is no CSS overload as the patch makes use of the generic dashicons and menu styles

As said, we need a button not a link. Using a button, we can't fully take advantage of the existing menu styles but we have to use something more specific for the button.

I would recommend to stick with the second patch approach, refreshed and cleaned up a bit in 29958.4.patch

Side note: couldn't find a way to use aria-expanded on the button (which would be ideal): couldn't figure out a simple way to detect via JavaScript when the CSS media query kicks in.

@afercia
4 years ago

#21 @joedolson
4 years ago

  • Keywords has-patch added; needs-patch removed

@valendesigns
4 years ago

#22 @valendesigns
4 years ago

I tested the previous patch and everything functionally works as expected using my keyboard. The only real issue was that it didn't properly support color schemes and the focus state was persistent. Patch 29958.5.diff solves both those issues. As well, I removed the screen reader text and now rely on JS to toggle the label using some strings I added to the commonL10n1 object.

Version 0, edited 4 years ago by valendesigns (next)

#23 @afercia
4 years ago

@valendesigns: this can't be done with JS. You have also to consider that there's a CSS media query that kicks in and then the menu will be folded. So with your changes, the menu will be collapsed and the label still be <span class="collapse-button-label">Collapse menu</span>.
That's wrong and should be reverted. And that's the reason I've made it with a pure CSS solution. Not perfect, but works.

Thanks for the Sass part, real improvements are always welcome.

I would kindly suggest, for the future, to first discuss with the patch author before any changes. There may be issues that you didn't consider or didn't notice and stepping in to make changes without a clear view on the issue can be just a waste of time for both, me and you.

@valendesigns
4 years ago

#24 @valendesigns
4 years ago

Replying to afercia:

@valendesigns: this can't be done with JS.

Here's a working patch that says otherwise. Please test patch 29958.6.diff.

Thanks for the Sass part, real improvements are always welcome.

No problem.

I would kindly suggest, for the future, to first discuss with the patch author before any changes.

If we all waited to discuss every nuance of the patches we test, then things would never get done. I can appreciate that you created multiple patches for this ticket and your efforts do not go unnoticed, but it's open for anyone to iterate on. Especially as close as we are to 4.2 being released. Time is of the essence and when a patch sits for 3 weeks I'm inclined to refresh it while I'm testing it.

Thank you for your comments, they really helped create a much better patch.

#25 @afercia
4 years ago

I would't add all that JS to detect a viewport size when it's not really needed. Actually I think you're overthinking here and probably complicating things at a point that this wouldn't make it into 4.2.

I'm still convinced things should be discussed before adding relevant changes that miss some important nuances of the issue, I would say it's also a matter of good manners.

This is also my kind request to please don't discuss further here on trac, happy to discuss on Slack if needed.

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

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


4 years ago

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


4 years ago

#28 @DrewAPicture
4 years ago

  • Keywords 4.2-strings added

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


4 years ago

#30 @jorbin
4 years ago

  • Keywords changed from needs-testing, has-patch, 4.2-strings to needs-testing has-patch 4.2-strings
  • Milestone changed from 4.2 to Future Release

This is a fairly large patch to be coming in this late in the release cycle. While it would be great to get this fixed, it's not a regression so let's handle it in a future release. This will give everyone time to work on a better solution where there is more consensus on the correct approach.

#31 @jorbin
4 years ago

  • Keywords 4.2-strings removed

#32 @samuelsidler
4 years ago

  • Keywords 4.3-early added

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


4 years ago

#34 @obenland
4 years ago

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

#35 @obenland
4 years ago

  • Keywords 4.3-early removed
  • Milestone changed from Future Release to 4.3

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


4 years ago

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


4 years ago

#39 @GaryJ
4 years ago

  • Keywords changed from needs-testing, has-patch to needs-testing has-patch

FWIW, .6 patch doesn't apply cleanly, due to a conflict in script-loader.php.

#40 @GaryJ
4 years ago

  • Keywords needs-refresh added

Observations on patches:

.4:

  • Button content is <span class="collapse-button-label">' . __( 'Collapse menu' ) . '</span>' . '<span class="screen-reader-text">' . __( 'Expand menu' ) . '</span>'. With no JS / CSS, this is going to read both labels for certain groups of users.
  • Since the menu can't be manually collapsed without JS, arguably the whole list item markup that contains the button should only be added via JS anyway, even for the default state.

.6:

  • If all the markup is going to be added via JS, then the click event should be delegated from a parent element that will always be present.
  • Overall, I like the JS refactoring.
  • What's the point of return respWidth || ''; ? The variable respWidth should always be populated, and non-zero. If it's not, then returning an empty string doesn't seem like the best option.
  • Instead of the logic being in getMenuState(), I'd rather see a data- attribute set at the time the body classes are amended, so that all logic is handled in one place. This can then simply be read as needed.

#41 @obenland
4 years ago

  • Milestone changed from 4.3 to Future Release

No new patches since it was moved in the milestone, let's try again in a future release.

#42 @jorbin
4 years ago

  • Owner jorbin deleted

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


3 years ago

@afercia
3 years ago

#44 @afercia
3 years ago

  • Focuses javascript added; administration removed
  • Keywords needs-testing needs-refresh removed
  • Milestone changed from Future Release to 4.7
  • Owner set to afercia

29958.diff is a refresh with a first pass for a slightly different approach.

Looking back at this, the JS approach makes more sense if used to simplify a bit other parts of common.js, for example to avoid some code repetition in window.wpResponsive.trigger. There is room for further improvements but that would require a broader refactoring so I've tried to don't introduce too many unrelated changes and keep the diff as clean as possible.

I'd propose to simplify the accessibility part too. No more ARIA live region. When the menu is collapsed, the button text is hidden so instead of updating the button text it's better to use and update an aria-label and an aria-expanded attributes.

  • makes the collapse/expand control a <button>, so it's actionable with a keyboard
  • adds aria-label and aria-expanded attributes on the button, dynamically updated to give feedback to assistive technologies users
  • introduces new functions to get the viewport width, set the menu state, and update the button's ARIA attributes

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


3 years ago

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


3 years ago

#48 @jorbin
3 years ago

  • Owner changed from afercia to jorbin

Assigning to myself for review.

#49 @jorbin
3 years ago

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

In 39141:

Administration: Ensure collapse menu is usable with a keyboard

Currently, the "Collapse menu" item is not focusable and keyboard users can't collapse/expand the admin menu. This aims to fix it so that screen readers no longer announce it as a clickable but it remains unfocusable and thus unusable. So it's now a button.

Quoting joedolson at WordCamp Chicago 2014:
"If it's supposed to act like a button, it should be a button."

Also includes a grunt:precommit run that picked up some postcss changes to src/wp-includes/css/customize-preview.css

Fixes #29958.
Props ajercia, ipm-frommen for an iterative patch, valendesigns for an iterative patch, GaryJ for feedback, joedolson for feedback, helen for feedback

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


3 years ago

Note: See TracTickets for help on using tickets.