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

[ejose19]

Fixes #3603

Changes proposed in this pull request:

The poposed solution is to avoid the case where minDate/maxDate is set, and using the change month arrows makes the popover to close on chrome browser, upon debugging, the issue is that document.activeElement is returning "body" instead of the change month button, which makes handlePopoverBlur set up execute incorrectly and subsequently calling handleClosePopover which causes the described issue.

Reviewers should focus on:

While this solution works correctly for that case, it makes "Popover closes when first day of the month is blurred" test fail, and after evaluating both use cases, it seems the min/max date has more weight, and so losing that "feature" is worth it.

Here's the sandbox (open in chrome) and without selecting any date, move 2 months before/after and notice how the popover closes incorrectly.

[adidahiya]

see my comment #3603 (comment)

[ejose19]

@adidahiya I think by the point we reach handlePopoverBlur it's too late to do something already. However at registerPopoverBlurHandler we can pass an argument to check if onMonthChange called this function, and which arrow triggered the event (left or right). Then since lastTabbableElement will be undefined, if the flag is set we assign a better candidate for lastTabbableElement.

However what happens if theoretically a month change leads the popover to a state without any tabbable element? Besides from that let me know what you think about this idea.

[ejose19]

@adidahiya I've refactored this PR, now it should cover all use cases and even improve existing function-ability which had another unreported bug.

To reproduce the other bug, on a DateInput will nul value, place the active element on the first day of the month, and then press left arrow key, notice how the popover closes. This is because we're making handlePopoverBlur fire when DayPicker Day elements lose blur, but this can be a normal operation if the user is just navigating using the arrow buttons.

I also removed the step where it makes lastTabbableElement the document.activeElement, which is incorrect. An example of this mistake is having the dateinput value null, the time element being shown, and navigating using the keyboard arrows, when changing months it will report the active DayPicker Day element as lastTabbableElement, but in practice it's still the milliseconds input.

[adidahiya]

docs preview

this does appear to fix the issue, so the UX changes are ok

[adidahiya]

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

[ejose19]

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.

[adidahiya]

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?)

[ejose19]

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)

[adidahiya]

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

[ejose19]

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"

[adidahiya]

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?)

[adidahiya]

docs preview build

confirmed in Chrome, FF, IE11 👍

thanks @ejose19