Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow implementors to change the WAI-ARIA role of the container (#3658) #3669

Merged
merged 5 commits into from
Feb 18, 2021

Conversation

nfreear
Copy link
Contributor

@nfreear nfreear commented Jan 14, 2021

Fixes #3658

Changelog Entry

Added

  • Resolves #3658. Added new containerRole to default style options, by @nfreear

Description

Added a new attribute role to <ReactWebChat> and renderWebChat, which allows the developer to specify the WAI-ARIA landmark role of the container for the Chat UI. It defaults to complementary, which is the currently hardcoded value.

Design

Specific Changes

  1. Modified <ReactWebChat> and <BasicWebChat> to accept role attribute

Tests added

  1. Added accessibility.landmarkRole.valid.html to make sure role is set correctly
  2. Added accessibility.landmarkRole.invalid.html to make sure role will fallback to complementary
  • I have added tests and executed them locally;
  • I have updated CHANGELOG.md;
  • I have updated documentation

Review Checklist

This section is for contributors to review your work.

  • Accessibility reviewed (tab order, content readability, alt text, color contrast)
  • Browser and platform compatibilities reviewed
  • CSS styles reviewed (minimal rules, no z-index)
  • Documents reviewed (docs, samples, live demo)
  • Internationalization reviewed (strings, unit formatting)
  • package.json and package-lock.json reviewed
  • Security reviewed (no data URIs, check for nonce leak)
  • Tests reviewed (coverage, legitimacy)

@ghost
Copy link

ghost commented Jan 14, 2021

CLA assistant check
All CLA requirements met.

@corinagum
Copy link
Contributor

Hey, thanks for the PR! I've added it to our project kanban even though it's still in draft - this is to keep it on our radar, not to pressure you on time. 🙂

@nfreear
Copy link
Contributor Author

nfreear commented Jan 15, 2021

Hi @corinagum,

I see that BotFramework-WebChat-CI-PR has failed below above, but when I try to click on details I get a 401 Unauthorized error.

Are you able to screenshot what the failure is please, or give me access or similar?

Note, I'm just waiting for a response to check my understanding ...;)

Many thanks,

Nick

@corinagum
Copy link
Contributor

corinagum commented Jan 15, 2021

@nfreear There was one failing test due to timeout as opposed to broken code. I've re-run the pipeline with the expectation that we'll get 100% pass. I'll check on it periodically and rerun if we get more timeouts.


Tests now passing

@nfreear
Copy link
Contributor Author

nfreear commented Jan 19, 2021

Thanks @corinagum !

@corinagum
Copy link
Contributor

@nfreear ping! How's it going? We'd definitely like to get your PR in when you're done with it!

@compulim
Copy link
Contributor

compulim commented Feb 18, 2021

I updated the PR as we commented in the original issue.

Instead of using styleOptions, we prefer role attribute, as commented in #3658.

useContainerRole is not needed because:

  • Hooks are added only if it benefit developers when they are in composition mode
    • Composition mode = developer bring their own UI, but using Web Chat business logic layer and maybe part of UIs from Web Chat
  • For this feature, it's very likely that the role prop is being used only once and reading/sharing the value is not a common scenario for composition mode

Copy link
Contributor

@corinagum corinagum left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @nfreear for the submission :)

@corinagum corinagum merged commit 8f435f7 into microsoft:master Feb 18, 2021
@nfreear
Copy link
Contributor Author

nfreear commented Feb 19, 2021

Hi @corinagum and @compulim,

Apologies I left this unfinished (caught up in other work I'm afraid :() ... And, thank you for modifying @compulim -- I can see how the modifications make sense!

Best wishes,

Nick

@corinagum
Copy link
Contributor

Totally understand about getting caught up! Similarly, we love community contributions but it's hard to fit it into our day-to-day schedule, haha. We'd love your participation in other changes if you ever have the time! Thanks again for your help.

@compulim compulim mentioned this pull request Mar 2, 2021
52 tasks
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.

bug: The WebChat container's role should not always be 'complementary' [screen reader accessibility]
3 participants