Tests/Docs for aria hidden polyfill#725
Merged
devongovett merged 26 commits intomainfrom Jul 14, 2020
Merged
Conversation
…t container to watch
fix bug where index of 0 wasn't undoing and removing
- Discovered an issue in our DialogTrigger tests where if all tests before `should restore focus to the trigger when the dialog is closed` were commented out, then this test would fail. Discovered because it was the starting point for my tests in aria-modal-polyfill. Issue is that we were getting lucky with some timings. By explicitly running everything we can avoid stuff happening after an `it` clause concludes.
…vate/react-spectrum-v3 into tests-for-aria-hidden-polyfill
|
Build successful! 🎉 |
snowystinger
changed the title
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
devongovett
requested changes
Jul 13, 2020
|
|
||
| ## Introduction | ||
|
|
||
| Certain browser + screen reader combinations do not implement aria-modal correctly, allowing users to navigate outside of the modal. |
Member
There was a problem hiding this comment.
Suggested change
| Certain browser + screen reader combinations do not implement aria-modal correctly, allowing users to navigate outside of the modal. | |
| Certain browser + screen reader combinations do not implement aria-modal correctly, which may unintentionally allow users to navigate outside of the modal. |
| ## Introduction | ||
|
|
||
| Certain browser + screen reader combinations do not implement aria-modal correctly, allowing users to navigate outside of the modal. | ||
| This watches a container for aria-modal nodes and hides the rest of the dom from screen readers when one is open use the [aria-hidden](https://www.npmjs.com/package/aria-hidden) package. |
Member
There was a problem hiding this comment.
Suggested change
| This watches a container for aria-modal nodes and hides the rest of the dom from screen readers when one is open use the [aria-hidden](https://www.npmjs.com/package/aria-hidden) package. | |
| `react-aria-modal` watches a container element for `aria-modal` nodes and hides the rest of the dom from screen readers with `aria-hidden` when one is open. See the [aria-hidden](https://www.npmjs.com/package/aria-hidden) package for more details. |
maybe also add something about when you'd need to use this? i.e. most of the time you don't need it, only if your app is not mounted at the root of the page.
| watchModals(); | ||
| ``` | ||
|
|
||
| You can also pass it a selector string, by default it will watch body, which is where most applications should have their provider. |
Member
There was a problem hiding this comment.
Suggested change
| You can also pass it a selector string, by default it will watch body, which is where most applications should have their provider. | |
| By default, `@react-aria/aria-modal-polyfill` watches the document body, but you can also pass it a selector string if your modals are mounted in a different container. |
| <FunctionAPI function={docs.exports.watchModals} links={docs.links} /> | ||
|
|
||
| ## How to use | ||
| Include this once in your application at the top level before modals are rendered. |
Member
There was a problem hiding this comment.
Suggested change
| Include this once in your application at the top level before modals are rendered. | |
| Include `@react-aria/aria-modal-polyfill` once in your application and call `watchModals` before any modals are rendered. |
| keywords: [dialog, modal, aria] | ||
| --- | ||
|
|
||
| # aria-modal-polyfill |
Member
There was a problem hiding this comment.
Maybe make the title watchModals? All the other docs pages have the title be the function/component name but this one has the package name. If you change it, make sure to change the filename of this mdx file too.
| * | ||
| * If a modal container is removed, then undo the hiding based on the last hide others. Check if there are any other modals still around, and | ||
| * hide based on the last one added. | ||
| * Acts as a polyfill for 'aria-modal' by watching for added modals and hiding the rest of the tree. |
Member
There was a problem hiding this comment.
Suggested change
| * Acts as a polyfill for 'aria-modal' by watching for added modals and hiding the rest of the tree. | |
| * Acts as a polyfill for `aria-modal` by watching for added modals and hiding any surrounding DOM elements with `aria-hidden`. |
| it('should hide everything except the modal', async () => { | ||
| watchModals(); | ||
| let {getByLabelText, getByRole} = render( | ||
| <Provider theme={theme}> |
Member
There was a problem hiding this comment.
Would these tests pass without the watchModals call? Do we need to render something outside the provider? I believe the provider itself will get aria-hidden applied already, without watchModals.
| If a dialog does not have a visible title element, an `aria-label` or `aria-labelledby` | ||
| prop must be passed instead to identify the element to assistive technology. | ||
|
|
||
| `useDialog` applies 'aria-modal' to the modal. This doesn't cover every browser/screen reader combination and should be combined with `@react-aria/aria-modal-polyfill`, see the [Aria Modal Polyfill docs](../react-aria/aria-modal-polyfill.html). |
Member
There was a problem hiding this comment.
I'd move this after the last paragraph before the example below which talks about this already.
Suggested change
| `useDialog` applies 'aria-modal' to the modal. This doesn't cover every browser/screen reader combination and should be combined with `@react-aria/aria-modal-polyfill`, see the [Aria Modal Polyfill docs](../react-aria/aria-modal-polyfill.html). | |
| **Note:** `useModal` only hides content within parent `OverlayProvider` components. However, if you have additional content in your application outside any `OverlayProvider`, then you should use the `@react-aria/aria-modal-polyfill` package to ensure that this content is hidden while modals are open as well. See the [aria-modal-polyfill docs](../react-aria/aria-modal-polyfill.html) for more information. |
Comment on lines
201
to
203
| When a Dialog is open, then it should be the only thing that can be interacted with. For the most part this is | ||
| handled by aria-modal, however, there are some cases where this is not the case. In order to make aria-modal work | ||
| in all cases, we have a package `@react-aria/aria-modal-polyfill`, see the [Aria Modal Polyfill docs](../react-aria/aria-modal-polyfill.html). |
Member
There was a problem hiding this comment.
Suggested change
| When a Dialog is open, then it should be the only thing that can be interacted with. For the most part this is | |
| handled by aria-modal, however, there are some cases where this is not the case. In order to make aria-modal work | |
| in all cases, we have a package `@react-aria/aria-modal-polyfill`, see the [Aria Modal Polyfill docs](../react-aria/aria-modal-polyfill.html). | |
| When a Dialog is open, then it should be the only thing that can be interacted with. If your application's `Provider` is at the root of the page, this is handled automatically. However, if your application has content outside the `Provider`, then you should use the `@react-aria/aria-modal-polyfill` package to hide this content from assistive technology while a modal is open. See the [aria-modal-polyfll docs](../react-aria/aria-modal-polyfill.html) for more details. |
ktabors
reviewed
Jul 13, 2020
| import {triggerPress} from '@react-spectrum/test-utils'; | ||
| import {watchModals} from '../'; | ||
|
|
||
| describe('watchModals', () => { |
Member
There was a problem hiding this comment.
Should we have an example where watchModals is passed a selector?
|
|
||
| **Note: `useDialog` only handles the dialog itself. It should be combined with other hooks and | ||
| components as described below to create a fully accessible modal dialog.** | ||
| **Note:** `useModal` only hides content within parent `OverlayProvider` components. However, if you have additional content in your application outside any `OverlayProvider`, then you should use the `@react-aria/aria-modal-polyfill` package to ensure that this content is hidden while modals are open as well. See the [aria-modal-polyfill docs](../react-aria/aria-modal-polyfill.html) for more information. |
Member
There was a problem hiding this comment.
parent 'OverlayProvider' components reads weird to me. I'm wondering if you should remove parent or add child.
Member
There was a problem hiding this comment.
This is hard to describe but I think both Devon and I had a go at explaining this and what Rob has feels like the best of the bunch.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
devongovett
previously approved these changes
Jul 14, 2020
dannify
previously approved these changes
Jul 14, 2020
|
Build successful! 🎉 |
|
Build successful! 🎉 |
devongovett
approved these changes
Jul 14, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: