fix: added prop to handle moving focus to loader overlay on initial render; updated readme #562
Conversation
|
Preview branch generated at https://fix-move-focus-to-loader.d1gko6en628vir.amplifyapp.com |
| ### build (if first time building dev environment, must be run before yarn dev) | ||
|
|
||
| ```sh | ||
| $ yarn build | ||
| ``` | ||
|
|
There was a problem hiding this comment.
This is a bit unrelated to the overlay issue, but added to the README that upon first run, must run yarn build prior to yarn dev.
| test('handles being passed a ref', () => { | ||
| const loaderRef = React.createRef(null); | ||
|
|
||
| const loaderOverlay = mount( | ||
| <LoaderOverlay className="baz" role="alert" label="loading" ref={loaderRef}> | ||
| Some text | ||
| </LoaderOverlay> | ||
| ); | ||
|
|
||
| setTimeout(() => { | ||
| expect(document.activeElement).toStrictEqual(loaderOverlay.getDOMNode()); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Test cases above this test for implementation of the new focus prop, but also added in a test to ensure backwards compatibility. As it stands, if we were to remove backwards compatibility for use of forwardRef, it would be a breaking change for current implementations where that prop is used.
| ? 'Loader__overlay--small' | ||
| : '' | ||
| )} | ||
| ref={ref ? ref : overlayRef} |
There was a problem hiding this comment.
For backwards compatibility purposes, checks if a ref was passed via prop, and if so, uses that ref instead of the overlayRef.
There was a problem hiding this comment.
I'd make a small tweak here, have the ref set as a variable in the body of the component so that the same ref is being consumed everywhere:
const overlayRef = ref || createRef<HTMLDivElement>()Then you can just utilize overlayRef where needed.
|
|
||
| useEffect(() => { | ||
| if ((!!focus && overlayRef) || ref) { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Trying to move focus prior to the next tick was causing issues where the styling for the focus indicator would not be applied, even thought he element was the active element.
There was a problem hiding this comment.
So I think this setTimeout is identifying a different issue where the ref is initially null on the first render. We may need to instead have the useEffect check that we have a ref value before setting focus:
useEffect(() => {
overlayRef.current?.focus()
}, [overlayRef.current])
|
|
||
| useEffect(() => { | ||
| if ((!!focus && overlayRef) || ref) { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
So I think this setTimeout is identifying a different issue where the ref is initially null on the first render. We may need to instead have the useEffect check that we have a ref value before setting focus:
useEffect(() => {
overlayRef.current?.focus()
}, [overlayRef.current])| ? 'Loader__overlay--small' | ||
| : '' | ||
| )} | ||
| ref={ref ? ref : overlayRef} |
There was a problem hiding this comment.
I'd make a small tweak here, have the ref set as a variable in the body of the component so that the same ref is being consumed everywhere:
const overlayRef = ref || createRef<HTMLDivElement>()Then you can just utilize overlayRef where needed.
|
The github status for security reviews isn't available so 🚨 🚨 REVIEWED FOR SECURITY 🚨 🚨 Small nitpick: You may want to update the PR title to accurately reflect the changes we made |
Will do and thank you again for the help! |
tbusillo
changed the title
tbusillo
changed the title
|
Preview branch generated at https://fix-move-focus-to-loader.d1gko6en628vir.amplifyapp.com |
ref: https://github.com/dequelabs/walnut/issues/3039
Additional comments at above ref as well.
Purpose
To address moving focus to the loader, which was less of an issue with the component itself and more of an issue with its implementation in and need for handling focus outside of the component (e.g., via forwardRef).
This solution addresses one part of the problem, with the second part needing to be addressed where the loader is being used without implementing some kind of focus (this PR will make it easier to fix that, however).
See comments in https://github.com/dequelabs/walnut/issues/3039 and the diffs for additional information.