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

fix: Example Body Content Overflow with support for keyboard scroll #2225

Merged
merged 3 commits into from
May 22, 2023

Conversation

williamjstanton
Copy link
Collaborator

@williamjstanton williamjstanton commented May 18, 2023

Added tabindex to the scrollable area of this example.

Summary

Fixes: #2222

Keyboard accessibility: the scrollable area in the Body Content Overflow example of a modal could not be scrolled up/down using the keyboard only.

Release Category

Component Examples

Release Note

Adding tabIndex 0 to Modal.Body will allow for content to be accessible and scrollable if the content overflows


Checklist

  • MDX documentation adheres to Canvas Kit's standard MDX template
  • Label ready for review has been added to PR

For the Reviewer

  • PR title is short and descriptive
  • PR summary describes the change (Fixes/Resolves linked correctly)
  • PR Release Notes describes additional information useful to call out in a release message or removed if not applicable
  • Breaking Changes provides useful information to upgrade to this code or removed if not applicable

Where Should the Reviewer Start?

  1. I would open the modal example, and inspect the scrollable. overflow container for a tabindex attribute.
  2. When the modal is opened, verify keyboard focus is set to the top of the dialog,
  3. Use the Tab key to focus the scrollable area,
  4. Verify the Up / Down arrow keys can be used to scroll all content into view.

Areas for Feedback? (optional)

  • Code
  • Documentation
  • Testing
  • Codemods

Testing Manually

Screenshots or GIFs (if applicable)

This should not have much affect on the UI. There will be a visible focus border displayed around the scrollable area.

Thank You Gif (optional)

Added tabindex to the scrollable area of this example.
@cypress
Copy link

cypress bot commented May 18, 2023

1 flaky tests on run #5762 ↗︎

0 886 2 0 Flakiness 1

Details:

Merge 70b5f16 into 41766d8...
Project: canvas-kit Commit: 4a32daa6f0 ℹ️
Status: Passed Duration: 06:43 💡
Started: May 19, 2023 6:34 PM Ended: May 19, 2023 6:41 PM
Flakiness  cypress/integration/Autocomplete.spec.ts • 1 flaky test

View Output Video

Test Artifacts
... > when a value is entered > when down arrow key is pressed > when the user presses the enter key > when the use hits the "2" key > should change the filtered results Output Screenshots Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@myvuuu myvuuu requested a review from mannycarrera4 May 19, 2023 17:43
@mannycarrera4
Copy link
Contributor

I'm wondering if this be the default and always add tabIndex 0 to the body instead of in the example?

@williamjstanton
Copy link
Collaborator Author

I'm wondering if this be the default and always add tabIndex 0 to the body instead of in the example?

I thought a little bit about that too, but I only want the Modal.Body to be keyboard focusable when the content overflows like in this example. The Basic example is great as-is, and I don't want to focus body content there.

Maybe the move is to support a new boolean prop for Modal.Body for consumers?

@mannycarrera4
Copy link
Contributor

I'm wondering if this be the default and always add tabIndex 0 to the body instead of in the example?

I thought a little bit about that too, but I only want the Modal.Body to be keyboard focusable when the content overflows like in this example. The Basic example is great as-is, and I don't want to focus body content there.

Maybe the move is to support a new boolean prop for Modal.Body for consumers?

I think this is where a great call out in the documentation would exists saying if you need focus and have scrollable content, use this tabIndex. I agree that the example as is is good for now

@alanbsmith alanbsmith merged commit 7094e14 into Workday:prerelease/minor May 22, 2023
@williamjstanton williamjstanton deleted the patch-2 branch July 7, 2023 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants