-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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.
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): make popover focusable via keyboard #4925
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close popover if shift+tab on inputPreviews: documentation | landing | table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there
Since the popover is usually open in a portal which makes it one of the last elements in the DOM, when closing the popover with tab or shift+tab, focus is likely to go to the browser address bar or an element at the bottom of the page, respectively. This change adds invisible divs to the beginning and end of the popover that, when focused, return focus to the input and close the popover. We cannot use the existing Overlay#shouldReturnFocusOnClose prop because it would reopen the popover.
private getKeyboardFocusableElements = (): HTMLElement[] => { | ||
const elements: HTMLElement[] = Array.from( | ||
this.popoverContentElement?.querySelectorAll( | ||
"button:not([disabled]),input,[tabindex]:not([tabindex='-1'])", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a subset of the selector used in Overlay
. I wonder if it's worth sharing that in the future
Focus input when closing DateInput popoverPreviews: documentation | landing | table |
Will fix tests |
// Skipping because simulate just invokes the function passed to React's "on<EventName>" prop | ||
// and doesn't actually simulate anything. Properly testing would require running with an actual | ||
// browser and focusing specific elements via the DOM API. This would require changing the Karma | ||
// config to run with Chrome instead of ChromeHeadless. | ||
it.skip("Popover closes when tabbing on first day of the month", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do have a working local version but could only get it to work by switching the Karma browser from ChromeHeadless
to Chrome
. The problem with simulate is that there's no way to get the focusin
event handler functions to run because React doesn't provide an onFocusIn
prop, only onFocus
. If I use the DOM API instead and call Element#focus()
, the browser will fire a focusin
event on my behalf. I'm not sure if that's an anti-pattern for the unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, this is fine to skip for now. testing focus behavior is tricky in these test suites
Skip failing testPreviews: documentation | landing | table |
// Skipping because simulate just invokes the function passed to React's "on<EventName>" prop | ||
// and doesn't actually simulate anything. Properly testing would require running with an actual | ||
// browser and focusing specific elements via the DOM API. This would require changing the Karma | ||
// config to run with Chrome instead of ChromeHeadless. | ||
it.skip("Popover closes when tabbing on first day of the month", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, this is fine to skip for now. testing focus behavior is tricky in these test suites
packages/datetime/src/dateInput.tsx
Outdated
@@ -301,6 +305,8 @@ export class DateInput extends AbstractPureComponent2<DateInputProps, IDateInput | |||
const { popoverProps = {} } = this.props; | |||
popoverProps.onClose?.(e); | |||
this.setState({ isOpen: false }); | |||
this.startFocusBoundaryElement?.removeEventListener("focusin", this.handleStartFocusBoundaryFocusIn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to use this ref approach and manually bind focusin event handlers? why can’t it be declarative like normal React code <div onFocusIn={this.handleEndFocusBoundaryFocusIn} />
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React doesn't support onFocusIn
, but it looks like their onFocus
prop listens to the focusin
event under the hood (facebook/react#6410 (comment)). I switched to this approach and everything still works as expected.
Use focus prop and stop manually adding event listenerPreviews: documentation | landing | table |
Related to #4812
Changes proposed in this pull request:
Pressing Tab when focused on a
DateInput
's text field will focus the popover content. Previously, pressing tab would focus the next keyboard-focusable element on the page. Since the popover is rendered with a portal by default, this would usually result in the popover closing.Does not autofocus the popover to continue to allow users to manually type in a date string if they prefer not to use the popover.
Screenshot
Before:
After:
Worth noting that the element tab order in the DatePicker seems weird
I would expect this order instead: