Skip to content

update chat and people tab roles#1770

Merged
dmceachernmsft merged 6 commits intomainfrom
dmceachernmsft/ChatAndPeopleRoleFix
Apr 7, 2022
Merged

update chat and people tab roles#1770
dmceachernmsft merged 6 commits intomainfrom
dmceachernmsft/ChatAndPeopleRoleFix

Conversation

@dmceachernmsft
Copy link
Copy Markdown
Member

What

Update chat and people roles to be tab over button.

Why

Helps with narration so that the screen readers and talk backs on devices announce 'tab' instead of 'button'

https://skype.visualstudio.com/SPOOL/_workitems/edit/2790210

How Tested

Ran windows narrator on windows and 'talkback' on android to verify the change.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 6, 2022

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 6, 2022

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 6, 2022

onClick={props.onChatButtonClicked}
styles={mobilePaneButtonStylesThemed}
checked={props.activeTab === 'chat'}
role={'tab'}
Copy link
Copy Markdown
Member

@JamesBurnside JamesBurnside Apr 6, 2022

Choose a reason for hiding this comment

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

Not sure if really necessary, but the documentation says tabs should be inside a parent that has tablist role, then have an id such that the tab contents have labelledby="<that tab id>"

so would be something like

const chatTabId = useId() // from fluent
const peopleTabId = useId()

<Stack.Item grow role="tablist" aria-label="<something>">
  <DefaultButton role="tab" id={chatTabId}>Chat</>
  <DefaultButton role="tab" id={peopleTabId}>People</>
</Stack>
...
<ChatPane role="tabpanel" labelledby={chatTabId} />
<PeoplePane role="tabpanel" labelledby={peopleTabId} />

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Tab_Role

But I'm happy to forgo for now and let a11y team file an issue if its needed if everything works as expected with the current implementation

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it behaves as expected but I agree I should look at this. will make a update shortly

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We chatted offline about this and the tablist is making the button labels be read out twice so this will not be changed in this PR. We can look into re-adding this again if the a11y team flags it as necessary

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 6, 2022

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 6, 2022

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 7, 2022

@dmceachernmsft dmceachernmsft merged commit 9ae1c9d into main Apr 7, 2022
@dmceachernmsft dmceachernmsft deleted the dmceachernmsft/ChatAndPeopleRoleFix branch April 7, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants