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

[datetime] fix(DateInput): Improve blur handling to avoid unexpected popover closures #4316

Merged
merged 5 commits into from
Sep 22, 2020
Merged

[datetime] fix(DateInput): Improve blur handling to avoid unexpected popover closures #4316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6a0e307
fix: dateInput popover closure when navigating months
ejose19 Sep 10, 2020
7e1954d
refactor: improve blur management on dateInput
ejose19 Sep 11, 2020
9f75624
refactor: apply PR recommendations
ejose19 Sep 21, 2020
408a8a7
Apply suggestions from code review
ejose19 Sep 21, 2020
ecb2941
fix: handlePopoverBlur when relatedTarget != null
ejose19 Sep 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/datetime/src/common/classes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export const DATEPICKER_FOOTER = `${DATEPICKER}-footer`;
export const DATEPICKER_MONTH_SELECT = `${DATEPICKER}-month-select`;
export const DATEPICKER_YEAR_SELECT = `${DATEPICKER}-year-select`;
export const DATEPICKER_NAVBAR = `${DATEPICKER}-navbar`;
export const DATEPICKER_NAVBUTTON = `DayPicker-NavButton`;
export const DATEPICKER_TIMEPICKER_WRAPPER = `${DATEPICKER}-timepicker-wrapper`;

export const DATERANGEPICKER = `${NS}-daterangepicker`;
Expand Down
76 changes: 47 additions & 29 deletions packages/datetime/src/dateInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,9 @@ export class DateInput extends AbstractPureComponent2<IDateInputProps, IDateInpu

private inputEl: HTMLInputElement | IRefObject<HTMLInputElement> | null = null;
private popoverContentEl: HTMLElement | null = null;
private lastElementInPopover: HTMLElement | null = null;
// Last element in popover that is tabbable, and the one that triggers popover closure
// when the user press TAB on it
private lastTabbableElement: HTMLElement | null = null;

private refHandlers = {
input: isRefObject<HTMLInputElement>(this.props.inputProps?.inputRef)
Expand All @@ -205,8 +207,20 @@ export class DateInput extends AbstractPureComponent2<IDateInputProps, IDateInpu
const { value, valueString } = this.state;
const dateString = this.state.isInputFocused ? valueString : getFormattedDateString(value, this.props);
const dateValue = isDateValid(value) ? value : null;
const dayPickerProps = {
const dayPickerProps: DayPickerProps = {
...this.props.dayPickerProps,
// If the user presses the TAB key on a DayPicker Day element and the lastTabbableElement is also a DayPicker Day
// element, the popover should be closed
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite follow this comment, why is this the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From your #3603 (comment)

but I would still like to retain the functionality added in #2093 which closes the DateInput popover after a user TABs out of the last tabbable element in the popover.

Which is basically a remark with that comment, if you feel it's redundant I can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

but what about "...and the lastElementInPopover is also a DayPicker Day element"? why is this guard necessary? the comment leaves me wondering what the other possibilities are (I assume it refers to cases where a time picker is rendered, making that the last tabbable element?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried all combinations and that extra guards need to be in place for when there's any tabbable element below the calendar (like time picker, but it would cover any new element added in the future)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, then that begs the question... which code is responsible for closing the popover when there is a tabbable element (like TimePicker) below the calendar? your implementation seems to work fine, but I want to make sure the code comments are clear and comprehensible for the next person who reads this code. I'm pulling your branch now to figure this out for myself

Copy link
Contributor Author

@ejose19 ejose19 Sep 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

registerPopoverBlurHandler along handlePopoverBlur is handling that (like originally implemented), just excluding the 2 cases mentioned in the comment. It's actually "agnostic" regarding if the last tabbable element is below the calendar or not, that state variable holds the last tabbable element inside the popover and listens for blur events.

Actually, now that you mention it, I found that this code

blueprint/packages/datetime/src/dateInput.tsx

Lines 446 to 447 in 408a8a7

this.lastElementInPopover = relatedTarget;
this.lastElementInPopover.addEventListener("blur", this.handlePopoverBlur);
is also incorrect. They are storing the "last tabbed element", not "last tabbable element" which will cause an issue if the user shift+tab and then tab. I will fix that part to use the same formula as registerPopoverBlurHandler (also maybe a documentation to lastElementInPopover meaning "last element in popover that is tabbable, and the one that triggers closure if the user press tabs on it"

onDayKeyDown: (day, modifiers, e) => {
if (
e.key === "Tab" &&
!e.shiftKey &&
this.lastTabbableElement.classList.contains(Classes.DATEPICKER_DAY)
) {
this.setState({ isOpen: false });
}
this.props.dayPickerProps.onDayKeyDown?.(day, modifiers, e);
},
// dom elements for the updated month is not available when
// onMonthChange is called. setTimeout is necessary to wait
// for the updated month to be rendered
Expand Down Expand Up @@ -406,57 +420,61 @@ export class DateInput extends AbstractPureComponent2<IDateInputProps, IDateInpu
this.safeInvokeInputProp("onKeyDown", e);
};

private getLastTabbableElement = () => {
// Popover contents are well structured, but the selector will need
// to be updated if more focusable components are added in the future
const tabbableElements = this.popoverContentEl.querySelectorAll("input, [tabindex]:not([tabindex='-1'])");
const numOfElements = tabbableElements.length;
// Keep track of the last focusable element in popover and add
// a blur handler, so that when:
// * user tabs to the next element, popover closes
// * focus moves to element within popover, popover stays open
const lastTabbableElement = numOfElements > 0 ? tabbableElements[numOfElements - 1] : null;

return lastTabbableElement as HTMLElement;
};

// focus DOM event listener (not React event)
private handlePopoverBlur = (e: FocusEvent) => {
let relatedTarget = e.relatedTarget as HTMLElement;
if (relatedTarget == null) {
// Support IE11 (#2924)
relatedTarget = document.activeElement as HTMLElement;
}
const eventTarget = e.target as HTMLElement;
// Beware: this.popoverContentEl is sometimes null under Chrome
if (
relatedTarget == null ||
(this.popoverContentEl != null && !this.popoverContentEl.contains(relatedTarget))
) {
this.handleClosePopover();
// Exclude the following blur operations that makes "body" the activeElement
// and would close the Popover unexpectedly
// - On disabled change months buttons
// - DayPicker day elements, their "blur" will be managed at its own onKeyDown
const isChangeMonthEvt = eventTarget.classList.contains(Classes.DATEPICKER_NAVBUTTON);
const isChangeMonthButtonDisabled = isChangeMonthEvt && (eventTarget as HTMLButtonElement).disabled;
const isDayPickerDayEvt = eventTarget.classList.contains(Classes.DATEPICKER_DAY);
if (!isChangeMonthButtonDisabled && !isDayPickerDayEvt) {
this.handleClosePopover();
}
} else if (relatedTarget != null) {
this.unregisterPopoverBlurHandler();
this.lastElementInPopover = relatedTarget;
this.lastElementInPopover.addEventListener("blur", this.handlePopoverBlur);
this.lastTabbableElement = this.getLastTabbableElement();
this.lastTabbableElement.addEventListener("blur", this.handlePopoverBlur);
}
};

private registerPopoverBlurHandler = () => {
if (this.popoverContentEl != null) {
// If current activeElement exists inside popover content, a month
// change has triggered and this element should be lastTabbableElement
let lastTabbableElement = this.popoverContentEl.contains(document.activeElement)
? document.activeElement
: undefined;
// Popover contents are well structured, but the selector will need
// to be updated if more focusable components are added in the future
if (lastTabbableElement == null) {
const tabbableElements = this.popoverContentEl.querySelectorAll(
"input, [tabindex]:not([tabindex='-1'])",
);
const numOfElements = tabbableElements.length;
if (numOfElements > 0) {
// Keep track of the last focusable element in popover and add
// a blur handler, so that when:
// * user tabs to the next element, popover closes
// * focus moves to element within popover, popover stays open
lastTabbableElement = tabbableElements[numOfElements - 1];
}
}
this.unregisterPopoverBlurHandler();
this.lastElementInPopover = lastTabbableElement as HTMLElement;
this.lastElementInPopover.addEventListener("blur", this.handlePopoverBlur);
this.lastTabbableElement = this.getLastTabbableElement();
this.lastTabbableElement.addEventListener("blur", this.handlePopoverBlur);
}
};

private unregisterPopoverBlurHandler = () => {
if (this.lastElementInPopover != null) {
this.lastElementInPopover.removeEventListener("blur", this.handlePopoverBlur);
if (this.lastTabbableElement != null) {
this.lastTabbableElement.removeEventListener("blur", this.handlePopoverBlur);
}
};

Expand Down
5 changes: 2 additions & 3 deletions packages/datetime/test/dateInputTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,13 @@ describe("<DateInput>", () => {
assert.isFalse(wrapper.find(Popover).prop("isOpen"));
});

it("Popover closes when first day of the month is blurred", () => {
it("Popover closes when tabbing on first day of the month", () => {
const defaultValue = new Date(2018, Months.FEBRUARY, 6, 15, 0, 0, 0);
const wrapper = mount(<DateInput {...DATE_FORMAT} defaultValue={defaultValue} />);
wrapper.find("input").simulate("focus").simulate("blur");
// First day of month is the only .DayPicker-Day with tabIndex == 0
const tabbables = wrapper.find(Popover).find(".DayPicker-Day").filter({ tabIndex: 0 });
const firstDay = tabbables.getDOMNode() as HTMLElement;
firstDay.dispatchEvent(createFocusEvent("blur"));
tabbables.simulate("keydown", { key: "Tab" });
// manually updating wrapper is required with enzyme 3
// ref: https://github.com/airbnb/enzyme/blob/master/docs/guides/migration-from-2-to-3.md#for-mount-updates-are-sometimes-required-when-they-werent-before
wrapper.update();
Expand Down