collapse menu keyboard accessibility

[afercia]

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?

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

[joedolson]

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.

[helen]

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.

[afercia]

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

[afercia]

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]

Still needs testing, especially in IE9+ :)

[joedolson]

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

[SergeyBiryukov]

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

[afercia]

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.

[helen]

#31272 was marked as a duplicate.

[ipm-frommen]

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.

[afercia]

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.

[DrewAPicture]

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

[afercia]

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.

[valendesigns]

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 commonL10n object.

[afercia]

@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]

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.

[afercia]

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 track, happy to discuss on Slack if needed.

[jorbin]

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.

[GaryJ]

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

[GaryJ]

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.

[obenland]

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

[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

[jorbin]

Assigning to myself for review.

[jorbin]

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