fix(3635): a11y - Sets focus to input on select #4285
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network.
@justinseiter you can click here to see the review status or cancel the code review job.
* Adds sendFocusBackToInput method for handling returning focus. * Adds tests around possible consumer implementations.
justinseiter
force-pushed
the
fix/issue-3635
branch
from
bd1cd27
to
83afadf
Compare
Codecov Report
@@ Coverage Diff @@
## main #4285 +/- ##
==========================================
+ Coverage 96.34% 96.52% +0.17%
==========================================
Files 25 25
Lines 2352 2358 +6
Branches 958 962 +4
==========================================
+ Hits 2266 2276 +10
+ Misses 86 82 -4
|
There was a problem hiding this comment.
It looks like this feature should successfully improve accessibility as described. I like that the tests were expanded so much for this. I just flagged a few things that our automation caught for future work since some things have been deprecated that affect the tests.
Reviewed with ❤️ by PullRequest
| @@ -97,7 +97,7 @@ describe("DatePicker", () => { | |||
| expect(datePicker.state.open).toBe(false); | |||
| }); | |||
|
|
|||
| it("should close the popper and return focus to the date input.", (done) => { | |||
| it("should close the popper and return focus to the date input on Escape.", (done) => { | |||
| // Date Picker Dialog | Date Grid | Enter | Closes the dialog and returns focus to the Choose Date button. | ||
| var div = document.createElement("div"); | ||
| document.body.appendChild(div); | ||
| var datePicker = ReactDOM.render(<DatePicker />, div); |
There was a problem hiding this comment.
ISSUE: react/no-render-return-value (Severity: Medium)
Do not depend on the return value from ReactDOM.render
Remediation:
Automation Result: react/no-render-return-value Do not depend on the return value from ReactDOM.render
It looks like these tests will need to be updated at some point. This link gives information about how this is legacy and what to do instead.
The existing test has this same code. It doesn't necessarily need to be fixed in this PR.
🤖 powered by PullRequest Automation 👋 verified by Dallas
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it("should hide the calendar when the pressing Shift + Tab in the date input", () => { | ||
| var datePicker = TestUtils.renderIntoDocument( | ||
| <DatePicker onBlur={onBlurSpy} />, |
There was a problem hiding this comment.
ISSUE: no-use-before-define (Severity: Medium)
'onBlurSpy' was used before it was defined.
Remediation:
Automation Result: no-use-before-define 'onBlurSpy' was used before it was defined.
This isn't actually in the changed code, but it looks like things are out of order here and that onBlurSpy is defined 2 lines below this. Does this test work as expected?
🤖 powered by PullRequest Automation 👋 verified by Dallas
| it("should return focus to input once time is selected", (done) => { | ||
| document.body.appendChild(div); // So we can check the dom later for activeElement | ||
| renderDatePicker("February 28, 2018 4:43 PM"); | ||
| const dateInput = ReactDOM.findDOMNode(datePicker.input); |
There was a problem hiding this comment.
ISSUE: react/no-find-dom-node (Severity: Medium)
Do not use findDOMNode. It doesn’t work with function components and is deprecated in StrictMode. See https://reactjs.org/docs/react-dom.html#finddomnode
Remediation:
Automation Result: react/no-find-dom-node Do not use findDOMNode. It doesn’t work with function components and is deprecated in StrictMode. See https://reactjs.org/docs/react-dom.html#finddomnode
It looks like this has been deprecated and should be updated soon. It is being used many times in this file, so it should probably be its own PR later.
🤖 powered by PullRequest Automation 👋 verified by Dallas
There was a problem hiding this comment.
This PR looks quite good for addressing the a11y problems outlined in the original issue. It's great to have the behavior for returning focus consolidated into one place and invoked where necessary, compared to the previous approach.
Thanks for such an excellent write-up as well.
I see the another reviewer already left some comments, and I don't see any further issues.
Reviewed with ❤️ by PullRequest
| @@ -123,6 +123,53 @@ describe("DatePicker", () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it("should close the popper and return focus to the date input on Enter.", (done) => { | |||
| // https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/datepicker-dialog.html | |||
| var dateInput = div.querySelector("input"); | ||
| TestUtils.Simulate.focus(dateInput); | ||
|
|
||
| // user may tab or arrow down to the current day (or some other element in the popper) |
Fixes: #3635 : Accessibility issue - when date picker closes focus does not return on input
Issue
When using the
EscapeorEnterkeys as well as after selection with pointer, focus did not return to input.Accessibility guidelines suggest that in these instances focus should return to the input.
What were the specific changes made?
sendFocusBackToInputmethod for handling returning focus. There were already two separate (nearly identical) implementations to handle this withinhandleSelectandonPopperKeydown. ThesendFocusBackToInputis simply an abstraction to facilitate reuse in those instances as well as several others.Additional notes/context
This work makes considerations not only for default implementations but as well as:
isClearable- Clicking clear now returns focus to inputcustomInput- This one Just Works ™️ as long as implemented as outlined in docs (ie withforwardRef)shouldCloseOnSelect- Don't hide calendar on date selection means we'll keep focus within pickerdisabledKeyboardNavigation- Pointer onlyshowTimeSelect- When this prop is enabled the picker currently closes once a time is selected. This work just makes sure that focus only returns on that event and not on date selection.inline- Focus remains on pickerwithPortal- Again, Just works ™️ thanks to previous contributors. :)Escapewhile in the input field - If picker was open and focus was on input, pressingEscapewould send focus tobody. Now returns to input.CleanShot.2023-09-29.at.12.07.25.mp4