Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Menu): Accessibility - move MenuItem onClick handler from li to a #61

Merged
merged 7 commits into from Aug 9, 2018

Conversation

miroslavstastny
Copy link
Member

@miroslavstastny miroslavstastny commented Aug 8, 2018

MenuItem

Move onClick handler from <li> to <a> for the menu to be accessible.

Closes #44

TODO

  • Conformance test
  • Minimal doc site example
  • Stardust base theme
  • Teams Light theme
  • Teams Dark theme
  • Teams Contrast theme
  • Confirm RTL usage
  • W3 accessibility check
  • Stardust accessibility check
  • Update glossary props table
  • Update the CHANGELOG.md

@miroslavstastny miroslavstastny changed the title feat(Menu): Accessibility - move MenuItem onClick handler from li to a [HELP NEEDED] feat(Menu): Accessibility - move MenuItem onClick handler from li to a Aug 8, 2018
@miroslavstastny miroslavstastny changed the title [HELP NEEDED] feat(Menu): Accessibility - move MenuItem onClick handler from li to a feat(Menu): Accessibility - move MenuItem onClick handler from li to a Aug 9, 2018
@codecov
Copy link

codecov bot commented Aug 9, 2018

Codecov Report

Merging #61 into master will decrease coverage by 0.02%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
- Coverage   85.92%   85.89%   -0.03%     
==========================================
  Files          74       74              
  Lines        1101     1106       +5     
  Branches      216      219       +3     
==========================================
+ Hits          946      950       +4     
  Misses        149      149              
- Partials        6        7       +1
Impacted Files Coverage Δ
src/components/Menu/MenuItem.tsx 100% <ø> (ø) ⬆️
src/themes/teams/components/Menu/menuItemStyles.ts 83.33% <75%> (-0.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 167bf51...bd0dee9. Read the comment docs.

...(shape === 'underlined' && {
paddingBottom: '4px',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it be in relative units (rem) as well?

color: variables.defaultColor,
paddingBottom: '4px',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems that relative units should be used here

color: 'inherit',
display: 'block',
...(shape === 'underlined'
? { padding: '0 0 8px 0' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pxToRem for 8px?

@miroslavstastny miroslavstastny merged commit a38523e into master Aug 9, 2018
@miroslavstastny miroslavstastny deleted the feat/menu-accessibility branch August 9, 2018 13:24
{...accessibility.attributes.root}
{...rest}
>
<ElementType className={classes.root} {...accessibility.attributes.root} {...rest}>
{childrenExist(children) ? (
children
) : (
<a
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retested more keyboard navigation and readers.

I focus on space/enter functionality which should works on the menu item by this spec:
https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-1/menubar-1.html#

  1. Enter/Space functionality without the readers:
  • link without the "href" attribute doesn't invoke onclick event on enter/space key
  • link with href='javascript:void(0)' invoke onclick event on enter key. Spece key doesn't invoke onclick.
  1. Enter/Space functionality with the readers:

2.1 Google/JAWS

if user navigate in virtual cursor mode then space/enter key invoke onclick event
if user navigate to menu item by tab key then works the same as without screen reader(see section above)

2.2 Edge/Narrator

space/enter key invoke onclick event in scan mode or in normal navigation

2.3 OSX/Ghrome/VoiceOver

space/enter key invoke onclick event when user press Control+Option+Space key. Just enter/Space works the same as without screen reader(see section above)

2.4 Fifefox/NVDA

if user navigate in browse mode then space/enter key invoke onclick event
if user navigate to menu item by tab key then works the same as without screen reader(see section above)

  1. Then another option is replace link with button. Then tested space/enter key works in each combination. Without reader or with readers.

Note:
in aria example supporting space was done javascript:
// Create simulated mouse event to mimic the behavior of ATs
// and let the event handler handleClick do the housekeeping.
try {
clickEvent = new MouseEvent('click', {
'view': window,
'bubbles': true,
'cancelable': true
});
}
catch (err) {
if (document.createEvent) {
// DOM Level 3 for IE 9+
clickEvent = document.createEvent('MouseEvents');
clickEvent.initEvent('click', true, true);
}
}
tgt.dispatchEvent(clickEvent);
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants