Twenty Seventeen: Mobile menu parent items require double tap

[celloexpressions]

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.

[karmatosed]

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.

[mckernanin]

I can't replicate on iOS10, running through some different devices on Browserstack to see if I can replicate it.

[helen]

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

[voldemortensen]

38397.diff should cover this.

Relevant link: ​http://stackoverflow.com/questions/34702991/double-tap-bug-on-ie-touch-in-ie10-ie11-links-must-be-tapped-twice

[voldemortensen]

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

[afercia]

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:

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:

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

[sami.keijonen]

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

[afercia]

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

Instead, when setting the attribute on the link:

[davidakennedy]

Adds aria-haspopup to links.

[davidakennedy]

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.

[sami.keijonen]

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.

[davidakennedy]

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

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

[davidakennedy]

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

[karmatosed]

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

[davidakennedy]

Closing this as fixed. See above commit.