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

fix(Menu): Add aria posinset and setsize, hide indicator from narration #1066

Merged
merged 6 commits into from
Mar 19, 2019

Conversation

jurokapsiar
Copy link
Contributor

This fixes menu items narration especially for older versions of Chromium.

  • explicitly adds aria-posinset and aria-setsize attributes to the menu item root
  • changes default behavior of Indicator component to IconBehavior

Might also fix #982 (but verification is needed)

@jurokapsiar jurokapsiar added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. labels Mar 17, 2019
@jurokapsiar jurokapsiar self-assigned this Mar 17, 2019
@jurokapsiar jurokapsiar changed the title Add aria posinset and setsize, hide indicator from narration fix(Menu): Add aria posinset and setsize, hide indicator from narration Mar 17, 2019
@codecov
Copy link

codecov bot commented Mar 18, 2019

Codecov Report

Merging #1066 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1066      +/-   ##
==========================================
+ Coverage   81.89%   81.91%   +0.01%     
==========================================
  Files         693      693              
  Lines        8447     8450       +3     
  Branches     1233     1233              
==========================================
+ Hits         6918     6922       +4     
+ Misses       1514     1513       -1     
  Partials       15       15
Impacted Files Coverage Δ
...b/accessibility/Behaviors/Menu/menuItemBehavior.ts 100% <ø> (ø) ⬆️
packages/react/src/components/Menu/MenuItem.tsx 63.11% <ø> (ø) ⬆️
...kages/react/src/components/Indicator/Indicator.tsx 77.77% <100%> (ø) ⬆️
packages/react/src/components/Menu/Menu.tsx 90.56% <100%> (+2.56%) ⬆️

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 abe2d6c...c70063d. Read the comment docs.

Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

👍 for code changes
@kolaps33 could you please review/verify accessibility?

itemPosition?: number

/** MenuItem count inside Menu. */
itemsCount?: number
Copy link
Member

Choose a reason for hiding this comment

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

These props will be added to public API actually, I'm fine with this for now, but I'm not sure that components should be so coupled with accessibility concerns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I am aware. but as far as I know, we do not have any other option to pass the information from parent menu to the menu item behaviors.

@kolaps33
Copy link
Collaborator

Tested with following:
ElectronFiddle + Jaws a NVDA
Fx + NVDA 
Ch + NVDA
Edge + Narrator

narrating position correctly :)

OSX/Chrome/VoiceOver still exists the issue:
#982

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Acc - Reader - "VoiceOver keys" - VoiceOver narrate wrongly position in menu
4 participants