Skip to content

Conversation

@T-Hugs
Copy link
Contributor

@T-Hugs T-Hugs commented Feb 12, 2018

Pull request checklist

Description of changes

2 Bug fixes:

  1. When a ContextualMenu creates a submenu, the aria-owns property on the parent menu item will be wrong if a submenu props object contained an ID.
  2. The attributes aria-setsize and aria-posinset are missing from CommandBar. Microsoft Narrator requires these to announce progress through menu items.

Focus areas to test

Unit test included for (1) above. Otherwise this is a low-risk change.

{ renderedItems!.map((item, index) => (
this._renderItemInCommandBar(item, index, expandedMenuItemKey!)
{ renderedItems!.map(item => (
this._renderItemInCommandBar(item, posInSet++, setSize, expandedMenuItemKey!)
Copy link
Collaborator

Choose a reason for hiding this comment

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

posInSet [](start = 49, length = 8)

Can you please add another value to the snapshot tests such that the count increment is working correctly. Or a unit test would be equally good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you meant by "add another value to the snapshot tests such that the count increment is working correctly" so I added a unit test.

Copy link
Member

Choose a reason for hiding this comment

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

That works! But he just meant add another item in the items of the command bar in the snapshot unit test which is easier to at least validate in the snapshot that the setSize works. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks David!

@manishgarg1
Copy link
Collaborator

Please merge master to make the screener tests pass

Copy link
Collaborator

@manishgarg1 manishgarg1 left a comment

Choose a reason for hiding this comment

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

Test fixes required

@manishgarg1 manishgarg1 self-assigned this Feb 12, 2018
@T-Hugs
Copy link
Contributor Author

T-Hugs commented Feb 12, 2018

@manishgarg1 I am not sure the screener test failures are caused by my changes. I see the same failures in other runs.

Edit: Never mind, I see I need to merge master.

joschect
joschect previously approved these changes Feb 12, 2018
@joschect joschect dismissed their stale review February 12, 2018 22:14

Should wait for micah

@dzearing
Copy link
Member

Spinning off screener tests again. If it fails unexpectedly, please remerge master.

I think there are 2 issues;

master was broken - this is now fixed.

screener has a bug which will link "officedev/master" instead of "trevorsg/master" which is hiding what broke. I talked with screener devs, and they're fixing this!

@manishgarg1 manishgarg1 merged commit f57acaa into microsoft:master Feb 13, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
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.

Aria-owns property for ContextualMenu item for a sub menu is wrong

4 participants