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

[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 1 commit
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 = `${DATEPICKER}-NavButton`;
ejose19 marked this conversation as resolved.
Show resolved Hide resolved
export const DATEPICKER_TIMEPICKER_WRAPPER = `${DATEPICKER}-timepicker-wrapper`;

export const DATERANGEPICKER = `${NS}-daterangepicker`;
Expand Down
10 changes: 7 additions & 3 deletions packages/datetime/src/dateInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,11 @@ export class DateInput extends AbstractPureComponent2<IDateInputProps, IDateInpu
// If the user tabs on a DayPicker Day element and the lastElementInPopover is also a DayPicker Day
ejose19 marked this conversation as resolved.
Show resolved Hide resolved
// 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.lastElementInPopover.classList.contains("DayPicker-Day")) {
if (
e.key === "Tab" &&
!e.shiftKey &&
this.lastElementInPopover.classList.contains(Classes.DATEPICKER_DAY)
) {
this.setState({ isOpen: false });
}
this.props.dayPickerProps.onDayKeyDown?.(day, modifiers, e);
Expand Down Expand Up @@ -432,9 +436,9 @@ export class DateInput extends AbstractPureComponent2<IDateInputProps, IDateInpu
// - On disabled change months buttons
// - DayPicker day elements, their "blur" will be managed at its own onKeyDown
// event listener for improving UX
ejose19 marked this conversation as resolved.
Show resolved Hide resolved
const isChangeMonthEvt = eventTarget.classList.contains("DayPicker-NavButton");
const isChangeMonthEvt = eventTarget.classList.contains(Classes.DATEPICKER_NAVBUTTON);
const isChangeMonthButtonDisabled = isChangeMonthEvt && (eventTarget as HTMLButtonElement).disabled;
const isDayPickerDayEvt = eventTarget.classList.contains("DayPicker-Day");
const isDayPickerDayEvt = eventTarget.classList.contains(Classes.DATEPICKER_DAY);
if (!isChangeMonthButtonDisabled && !isDayPickerDayEvt) {
this.handleClosePopover();
}
Expand Down