Make WordPress Core

Opened 9 years ago

Closed 3 months ago

Last modified 3 months ago

#32728 closed defect (bug) (fixed)

Customizer Menus: Accessibility: reordering menu items should have some indication about the item level

Reported by: designsimply's profile designsimply Owned by: joedolson's profile joedolson
Milestone: 6.5 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: accessibility, javascript Cc:

Description

/hat tip @afercia for the original report at https://github.com/voldemortensen/menu-customizer/issues/105

Splitting this out from the users test post on Make WordPress Accessible:
https://make.wordpress.org/accessibility/2015/06/12/accessibility-usertest-menu-customizer/

When reordering menu items, there should be an audible indication of their level.

NVDA
When reordering menu items, NVDA announces “menu is now a submenu item” or “menu item moved out of submenu”, which is useful, However, when reviewing the list while reordering, there is no indication at what level any given menu item is currently set to.

Supernova
When using the reorder feature, there is no screen reader feedback to indicate the menus level, perhaps a level indicator can be added after the page title that says something like “top”, “level 1″, “level 2″, etc to show how far down the tree each menu item is. As this is only really to benefit screen reader users, it could be visually hidden using the CSS clip method.

Attachments (4)

32728.diff (4.4 KB) - added by kushang78 4 months ago.
Added patch for this ticket.
32728.2.diff (3.3 KB) - added by joedolson 3 months ago.
Simplified patch
32728.2.fixed.diff (3.3 KB) - added by rcreators 3 months ago.
The patch is missing slash before src directory. so was not able to apply. I updated it and its working now. any one can test it. Patch works fine.
32728-Patch-Result.jpg (58.9 KB) - added by rcreators 3 months ago.
Patch Test Result.

Download all attachments as: .zip

Change History (34)

#1 @afercia
9 years ago

  • Focuses javascript added

In nav-menus.php they have (at least) some text to indicate they're sub-items. This could be further improved adding a depth indicator.

https://cldup.com/DM4D_dQjFF.png

#2 @afercia
9 years ago

  • Type changed from enhancement to defect (bug)

Changing from "enhancement" to "bug" as agreed with @obenland.

#3 follow-up: @celloexpressions
9 years ago

Is the depth useful or would it be better to know what the parent item is? "Sub item under [title]"

#4 in reply to: ↑ 3 @afercia
9 years ago

Replying to celloexpressions:

Is the depth useful or would it be better to know what the parent item is? "Sub item under [title]"

I'd say no parent info but would be useful to know the actual sub items level.

#5 @valendesigns
9 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.3
  • Owner set to valendesigns
  • Status changed from new to assigned
  • Version set to trunk

Anymore suggestions/expectations on the output before I get started on a patch?

#6 @afercia
9 years ago

Just discovered something I was not aware of because, well it's pretty undiscoverable :) In the current nav-menus.php screen, menu items have some infos about the menu item level and parent. It's done with JavaScript as hidden text and also title attribute. I'd remove the title attribute and keep just the hidden text but the question here is: would users expect to find the same kind of information in the Menu Customizer too? See screenshot:

https://cldup.com/NSyxNg33KR.png

#7 @celloexpressions
9 years ago

@afercia could you advise on the ideal wording/information provided and where it should be placed (I'm guessing inside the edit menu item button like on the admin screen?)?

#8 @afercia
9 years ago

@celloexpressions I'd say it should be placed on the focusable elements, i.e. the buttons yeah. Something like what was done in 33151 for the available menu items, hiding redundant text with aria-hidden.
Not sure about the wording. Maybe use the same wording/information used in the admin screen to keep consistency? And then change it in both places if we find something better in future iterations?

#9 @obenland
9 years ago

  • Milestone changed from 4.3 to Future Release
  • Type changed from defect (bug) to enhancement

No patch and we're a ew days away from beta4, let's iterate on this in a future release.

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


7 years ago

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


5 years ago

#14 @afercia
5 years ago

  • Milestone set to Future Release

#15 @celloexpressions
3 years ago

  • Owner valendesigns deleted
  • Type changed from enhancement to defect (bug)

There is clear direction above on the preferred text and location from the accessibility team. Re-typing as a bug because the experience is broken for some users and is worse in the customizer than in the equivalent wp-admin screen.

The next step is to create a patch. The patch will likely need to edit the customize menu item control's HTML template in addition to the JS methods that handle reordering and changing depth to keep the text updated when items are moved around.

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


6 months ago

#17 @joedolson
6 months ago

  • Milestone changed from Future Release to 6.5

@kushang78
4 months ago

Added patch for this ticket.

#18 @kushang78
4 months ago

  • Keywords has-patch added; needs-patch removed
  • As per above all threads & discussions, I've made some changes in below 4 files.
src/js/_enqueues/lib/nav-menu.js
src/js/_enqueues/wp/customize/nav-menus.js
src/wp-admin/includes/class-walker-nav-menu-edit.php
src/wp-includes/customize/class-wp-customize-nav-menu-item-control.php

The changes:

  • Added sub-items text to customizer menu as same as nav-menus.php page.
  • Added new title attribute for quick preview on hover for both.
  • Show & Hide sub-items text with available, Drag & drop functionality and Reorder button functionality.

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


4 months ago

#20 @joedolson
4 months ago

  • Keywords needs-testing added

Adding title attributes is something that we specifically avoid doing in WordPress, because they can cause a wide variety of accessibility problems. But I'll take a look at this soon.

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


3 months ago

#22 @audrasjb
3 months ago

As per today's bugscrub: @joedolson is it still in your radar?
It would be nice for this to be reviewed by next week. Or we can of course also put this ticket in milestone 6.6 to give it more time for review/discussion.

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


3 months ago

#24 @joedolson
3 months ago

This updated patch simplifies the changes. The original patch by @kushang78 included a few changes that I don't think we should make. First, the introduction of duplicate accessible name via the title attribute. Second, it modified the button text in the Customizer to match the format in the admin nav menus, but without also adding the equivalent JS to modify the texts as they are in the admin nav menus.

Overall, I think that the control's accessible name is currently better in the customizer than it is in the admin nav menus, so I don't think this should be changed in the customizer.

The Customizer accessible name format is "Edit menu item: {item title} ({item type})".

The admin accessible name format is "{item title} Menu item n of n". It doesn't include the purpose of the control or the type, so the Customizer is providing better information; the admin menu needs to be updated.

The patch doesn't address the idea of adding the menu item's depth in any way, and I think that's a more major thing to address.

I propose committing the version of the code in the simplified patch, which meets the intention expressed in the original ticket, and open an enhancement ticket to add depth to both menu interfaces and a bug to address the accessible naming in the admin menu interface.

While working on this, I also noticed an unrelated bug where the button text only would return either data.title or wp.customize.Menus.data.l10n.untitled, and didn't return data.original_title, which caused it to be untitled for any item without an explicitly defined title. It seemed reasonable to fix that at the same time.

@joedolson
3 months ago

Simplified patch

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


3 months ago

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


3 months ago

@rcreators
3 months ago

The patch is missing slash before src directory. so was not able to apply. I updated it and its working now. any one can test it. Patch works fine.

@rcreators
3 months ago

Patch Test Result.

#27 @swissspidy
3 months ago

@joedolson Just checking in here as RC 1 is approaching soon, so I'd suggest making a decision on committing the patch if you feel it's ready, or else consider punting.

#28 @joedolson
3 months ago

  • Keywords commit added; needs-testing removed

Will commit tomorrow.

#29 @swissspidy
3 months ago

  • Owner set to joedolson

#31 @joedolson
3 months ago

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

In 57746:

Customize: Accessibility: Label menu subitems when updating menus.

Fix missing subitem indicator when menu items in the Customizer are children of parent menu items & add menu item original title to button text. Adds parity with the admin menu editor and fixes a bug where the button text was 'untitled' if the menu item title had not been edited.

Props designsimply, afercia, celloexpressions, kushang78, joedolson, rcreators.
Fixes #32728.

Note: See TracTickets for help on using tickets.