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.

Sign up for GitHub

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

Jump to bottom

fix(3635): a11y - Sets focus to input on select #4285

Merged
merged 1 commit into from
Oct 3, 2023
Merged

fix(3635): a11y - Sets focus to input on select #4285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 29 additions & 26 deletions src/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ export default class DatePicker extends React.Component {
constructor(props) {
super(props);
this.state = this.calcInitialState();
this.preventFocusTimeout = null;
}

componentDidMount() {
Expand Down Expand Up @@ -463,6 +464,23 @@ export default class DatePicker extends React.Component {
this.setState({ focused: true });
};

sendFocusBackToInput = () => {
// Clear previous timeout if it exists
if (this.preventFocusTimeout) {
this.clearPreventFocusTimeout();
}

// close the popper and refocus the input
// stop the input from auto opening onFocus
// setFocus to the input
this.setState({ preventFocus: true }, () => {
this.preventFocusTimeout = setTimeout(() => {
this.setFocus();
this.setState({ preventFocus: false });
});
});
};

cancelFocusInput = () => {
clearTimeout(this.inputFocusTimeout);
this.inputFocusTimeout = null;
Expand Down Expand Up @@ -543,15 +561,11 @@ export default class DatePicker extends React.Component {
};

handleSelect = (date, event, monthSelectedIn) => {
// Preventing onFocus event to fix issue
// https://github.com/Hacker0x01/react-datepicker/issues/628
this.setState({ preventFocus: true }, () => {
this.preventFocusTimeout = setTimeout(
() => this.setState({ preventFocus: false }),
50,
);
return this.preventFocusTimeout;
});
if (this.props.shouldCloseOnSelect && !this.props.showTimeSelect) {
// Preventing onFocus event to fix issue
// https://github.com/Hacker0x01/react-datepicker/issues/628
this.sendFocusBackToInput();
}
if (this.props.onChangeRaw) {
this.props.onChangeRaw(event);
}
Expand Down Expand Up @@ -699,6 +713,7 @@ export default class DatePicker extends React.Component {

this.props.onChange(changedDate);
if (this.props.shouldCloseOnSelect) {
this.sendFocusBackToInput();
this.setOpen(false);
}
if (this.props.showTimeInput) {
Expand Down Expand Up @@ -765,6 +780,7 @@ export default class DatePicker extends React.Component {
}
} else if (eventKey === "Escape") {
event.preventDefault();
this.sendFocusBackToInput();
this.setOpen(false);
} else if (eventKey === "Tab") {
this.setOpen(false);
Expand Down Expand Up @@ -875,24 +891,8 @@ export default class DatePicker extends React.Component {
onPopperKeyDown = (event) => {
const eventKey = event.key;
if (eventKey === "Escape") {
// close the popper and refocus the input
// stop the input from auto opening onFocus
// close the popper
// setFocus to the input
// allow input auto opening onFocus
event.preventDefault();
this.setState(
{
preventFocus: true,
},
() => {
this.setOpen(false);
setTimeout(() => {
this.setFocus();
this.setState({ preventFocus: false });
});
},
);
this.sendFocusBackToInput();
}
};

Expand All @@ -902,6 +902,9 @@ export default class DatePicker extends React.Component {
event.preventDefault();
}
}

this.sendFocusBackToInput();

if (this.props.selectsRange) {
this.props.onChange([null, null], event);
} else {
Expand Down
104 changes: 103 additions & 1 deletion test/datepicker_test.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Copy link

Choose a reason for hiding this comment

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

Nice clarification here on an existing test

◽ Compliment

Image of Dallas Dallas

// https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/datepicker-dialog.html
// Date Picker Dialog | Escape | Closes the dialog and returns focus to the Choose Date button.
var div = document.createElement("div");
Expand All @@ -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
Copy link

Choose a reason for hiding this comment

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

Adding in the relevant documentation here is really useful—thanks!

◽ Compliment

Image of Ryan Ryan

// 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);
Copy link

Choose a reason for hiding this comment

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

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


// user focuses the input field, the calendar opens
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)
Copy link

Choose a reason for hiding this comment

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

Wow, this is also really great that you're walking us through each step of the test in detail. This helps a lot with readability and future maintainability.

◽ Compliment

Image of Ryan Ryan

var today = div.querySelector(".react-datepicker__day--today");
today.focus();

// user hits Enter
TestUtils.Simulate.keyDown(today, getKey("Enter"));
defer(() => {
expect(datePicker.calendar).toBeFalsy();
expect(datePicker.state.preventFocus).toBe(false);
expect(document.activeElement).toBe(div.querySelector("input"));
done();
});
});

it("should not close the popper and keep focus on selected date if showTimeSelect is enabled.", (done) => {
var div = document.createElement("div");
document.body.appendChild(div);
var datePicker = ReactDOM.render(<DatePicker showTimeSelect />, div);

// user focuses the input field, the calendar opens
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)
var today = div.querySelector(".react-datepicker__day--today");
today.focus();

// user hits Enter
TestUtils.Simulate.keyDown(today, getKey("Enter"));
defer(() => {
expect(datePicker.calendar).toBeTruthy();
expect(document.activeElement).toBe(today);
done();
});
});

it("should not re-focus the date input when focusing the year dropdown", () => {
const onBlurSpy = jest.fn();
const datePicker = mount(
Expand Down Expand Up @@ -214,6 +261,24 @@ describe("DatePicker", () => {
expect(datePicker.state.open).toBe(true);
});

it("should keep focus within calendar when clicking a day on the calendar and shouldCloseOnSelect prop is false", () => {
var div = document.createElement("div");
document.body.appendChild(div);
ReactDOM.render(<DatePicker shouldCloseOnSelect={false} />, div);

// user focuses the input field, the calendar opens
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)
var today = div.querySelector(".react-datepicker__day--today");
today.focus();

// user hits Enter
TestUtils.Simulate.keyDown(today, getKey("Enter"));
expect(document.activeElement).toBe(today);
});

it("should set open to true if showTimeInput is true", () => {
var datePicker = TestUtils.renderIntoDocument(
<DatePicker shouldCloseOnSelect={false} showTimeInput />,
Expand Down Expand Up @@ -325,6 +390,23 @@ describe("DatePicker", () => {
expect(datePicker.calendar).toBeFalsy();
});

it("should hide the calendar and keep focus on input when pressing escape in the date input", (done) => {
var div = document.createElement("div");
document.body.appendChild(div);
var datePicker = ReactDOM.render(<DatePicker />, div);

// user focuses the input field, the calendar opens
var dateInput = div.querySelector("input");
TestUtils.Simulate.focus(dateInput);

TestUtils.Simulate.keyDown(dateInput, getKey("Escape"));
defer(() => {
expect(datePicker.calendar).toBeFalsy();
expect(document.activeElement).toBe(dateInput);
done();
});
});

it("should hide the calendar when the pressing Shift + Tab in the date input", () => {
var datePicker = TestUtils.renderIntoDocument(
<DatePicker onBlur={onBlurSpy} />,
Copy link

Choose a reason for hiding this comment

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

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

Expand Down Expand Up @@ -403,6 +485,26 @@ describe("DatePicker", () => {
expect(datePicker.state.inputValue).toBeNull();
});

it("should return focus to input when clear button is used", (done) => {
var div = document.createElement("div");
document.body.appendChild(div);
var datePicker = ReactDOM.render(
<DatePicker selected={utils.newDate("2015-12-15")} isClearable />,
div,
);

var clearButton = TestUtils.findRenderedDOMComponentWithClass(
datePicker,
"react-datepicker__close-icon",
);
TestUtils.Simulate.click(clearButton);

defer(() => {
expect(document.activeElement).toBe(div.querySelector("input"));
done();
});
});

it("should set the title attribute on the clear button if clearButtonTitle is supplied", () => {
const datePicker = TestUtils.renderIntoDocument(
<DatePicker
Expand Down
16 changes: 16 additions & 0 deletions test/timepicker_test.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from "react";
import defer from "lodash/defer";
import DatePicker from "../src/index.jsx";
import TestUtils from "react-dom/test-utils";
import ReactDOM from "react-dom";
Expand Down Expand Up @@ -165,6 +166,21 @@ describe("TimePicker", () => {
expect(getInputString()).toBe("February 28, 2018 12:30 AM");
});

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);
Copy link

Choose a reason for hiding this comment

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

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

TestUtils.Simulate.focus(dateInput);
const time = TestUtils.findRenderedComponentWithType(datePicker, Time);
const lis = TestUtils.scryRenderedDOMComponentsWithTag(time, "li");
TestUtils.Simulate.keyDown(lis[1], getKey("Enter"));

defer(() => {
expect(document.activeElement).toBe(dateInput);
done();
});
});

it("should not select time when Escape is pressed", () => {
renderDatePicker("February 28, 2018 4:43 PM");
TestUtils.Simulate.focus(ReactDOM.findDOMNode(datePicker.input));
Expand Down