Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#38397 closed defect (bug) (fixed)

Twenty Seventeen: Mobile menu parent items require double tap

Reported by: celloexpressions's profile celloexpressions Owned by:
Milestone: 4.7 Priority: low
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch needs-testing commit
Focuses: accessibility Cc:

Description

Because of the code in place to make sub-menus accessible on larger screens, the first tap on a parent menu item on mobile does nothing. It should either open the submenu (duplicating the action of tapping the dropdown arrow) or navigate to the top-level item on the first tap. However, it's important that the current behavior be maintained on larger screens. I'd suggest adding a conditional for the screen size in the JS that handles this.

Testing in Edge 38 on Windows Phone 10.

Attachments (3)

38397.diff (1.0 KB) - added by voldemortensen 9 years ago.
38397.2.patch (786 bytes) - added by davidakennedy 9 years ago.
Adds aria-haspopup to links.
38397.3.patch (804 bytes) - added by davidakennedy 9 years ago.
Removes aria-haspopup attribute for menu items with sub-menus.

Download all attachments as: .zip

Change History (21)

#1 @karmatosed
9 years ago

Brought over to trac from this GitHub issue as part of merge: https://github.com/WordPress/twentyseventeen/issues/393.

If we do a fix here, we also need to fix in Twenty Sixteen.

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


9 years ago

#3 @mckernanin
9 years ago

This issue appears to be specific to mobile IE.
Tested on my iPhone 7 Plus running iOS 10, a few Android devices via browserstack, and Windows Phone 8.1 via an emulator on browserstack, and the menus work properly on iOS and Android.

Last edited 9 years ago by mckernanin (previous) (diff)

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


9 years ago

#5 @helen
9 years ago

  • Priority changed from normal to low

I hate these issues but it seems puntable to me if we get down to it, lowering priority accordingly.

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


9 years ago

@voldemortensen
9 years ago

#7 @voldemortensen
9 years ago

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

#8 @voldemortensen
9 years ago

If this works, I'll submit a pull request to Twenty Sixteen as well.

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


9 years ago

#10 @afercia
9 years ago

  • Focuses accessibility added

Worth noting the aria-haspopup attribute set on the list item doesn't have any benefit for accessibility, since it's on a non-focusable, non-actionable element (the <li>). Screen readers won't announce it, I've tested just with VoiceOver but it will be the same with other screen readers:

https://cldup.com/MHnPPN9cTD.png

Only when I edit the JS and set the attribute on the <a> element, then it is announced. VoiceOver announces aria-haspopup saying "Menu pop-up", other screen readers use a different wording:

https://cldup.com/wYof2zhqkF.png

That's also how the aria-haspopup is implemented in the core admin menu and toolbar, it is set on the links, not on the list items.

The example mentioned on the Stack Overflow post, see https://www.w3.org/WAI/tutorials/menus/examples/appmenu/, is completely different because:

  • it's an application menu, not a navigation menu
  • thus, it uses the ARIA roles menubar, menu, menuitem that make the list items actionable when using a screen reader
  • the click event is attached on the list items

So that example shouldn't be taken into consideration. As it is now, on the Twenty Seventeen menu the aria-haspopup is pointless and as far as I see there are two options:

  • remove it: the button already gives some indication there's a sub menu that can be expanded
  • set the aria-haspopup attribute on the links, and then it should be tested again to check the bug reported on this ticket

#11 @sami.keijonen
9 years ago

@afercia: Really good points. What I see happening over and over again is that issues inherits from _s theme. It's a great starting point but needs better development so that things like these would not be issues in so many future themes.

#12 @afercia
9 years ago

Confirmed also with Firefox + NVDA: no indication whatsoever given by the aria-haspopup attribute set on the list item

https://cldup.com/uYo76NmV3w.png

Instead, when setting the attribute on the link:

https://cldup.com/9vyb4VgXya.png

@davidakennedy
9 years ago

Adds aria-haspopup to links.

#13 @davidakennedy
9 years ago

Thanks all for the work and debugging here!

In 38397.2.patch, I added the attribute to the <a> and not the <li>. I can confirm it helps accessibility, as @afercia mentioned. But in some simulated testing in Browserstack, it didn't solve the original issue in this ticket – even though the <a> is a sibling element to the <ul>. It's not a parent any longer.

I'd love it if someone with a real Windows device could test this though. I'd like to avoid browser sniffing if at all possible.

@sami.keijonen Sometimes code isn't perfect – pretty sure I helped write the initial accessible dropdown code for _s. :) But it's great that the bug is known now, and can be fixed. I have a running list of improvements to roll back to Underscores after Twenty Seventeen launches.

#14 @sami.keijonen
9 years ago

Confirm that 38397.2.patch works for aria-haspopup but can't test the original issue because don't have windows phone anymore.

@davidakennedy: I apologize, I sounded rude. What I ment to say is that it would be great if underscores would get the same love and attention as default themes. Let's discuss that in Slack or in repo.

#15 @davidakennedy
9 years ago

@sami.keijonen Thanks for testing! No need to apologize. :) I didn't take it as rude. I agree! Ping me if you want to talk Underscores.

@davidakennedy
9 years ago

Removes aria-haspopup attribute for menu items with sub-menus.

#16 @davidakennedy
9 years ago

  • Keywords commit added

After further review from the accessibility team, it was determined that the menu items with sub-menus don't need the aria-haspopup attribute. This is because on larger screens, clicking a control to reveal the sub-menu isn't required. It's activated via :hover or :focus. On small screens, the button, enabled with aria-expanded, handles this case so aria-haspopup isn't needed there. Removing them entirely fixes this bug, done in 38397.3.patch.

I tested in a few Windows devices via Browserstack and don't see the problem any longer.

See: https://wordpress.slack.com/archives/accessibility/p1479147076000571

#17 @karmatosed
9 years ago

In 39232:

Twenty Seventeen: Fix for mobile menu parent items requiring double tap

After further review from the accessibility team, it was determined that the menu items with sub-menus don't need the aria-haspopup attribute.

Props davidakennedy, celloexpressions, voldemortensen, afercia
See #38397

#18 @davidakennedy
9 years ago

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

Closing this as fixed. See above commit.

Note: See TracTickets for help on using tickets.