Opened 9 years ago
Closed 9 years ago
#38397 closed defect (bug) (fixed)
Twenty Seventeen: Mobile menu parent items require double tap
Reported by: |
|
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)
Change History (21)
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
9 years ago
#3
@
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.
This ticket was mentioned in Slack in #core by helen. View the logs.
9 years ago
#5
@
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
#7
@
9 years ago
- Keywords has-patch needs-testing added; needs-patch removed
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
This ticket was mentioned in Slack in #core by voldemortensen. View the logs.
9 years ago
#10
@
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:
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
#11
@
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.
#13
@
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
@
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
@
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.
#16
@
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
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.