-
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
[Popover] Improved focus event handling #2382
Conversation
Ignore focus gained from outside pagePreview: 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.
🎉 this is sweeeeeet!
this.handleMouseEnter(e); | ||
} | ||
}; | ||
|
||
private handleTargetBlur = (e?: React.FormEvent<HTMLElement>) => { | ||
private handleTargetBlur = (e?: React.FocusEvent<HTMLElement>) => { |
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.
can we make this arg required? to ensure that it exists before you reference it
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.
Sure thing. No idea why it was optional to begin with. API change in React?
@@ -499,23 +503,26 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState> | |||
} | |||
}; | |||
|
|||
private handleTargetFocus = (e?: React.FormEvent<HTMLElement>) => { | |||
private handleTargetFocus = (e?: React.FocusEvent<HTMLElement>) => { |
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.
required?
Make event arg non-optionalPreview: documentation | landing | table |
@reiv what's the best way to verify this feature in the docs preview? perhaps you could add a switch for |
@giladgray I made the change to fix this tooltip behavior: #2373. You can try reproducing that (see the GIF I posted); the tooltip should no longer reappear when the page reacquires focus. The tooltip attached to the button on the Tooltip docs page can be used as well. The popover example which I think you're referring to still suffers from another buggy focus-related interaction which this PR does not address (#2307), so it would probably not work the way we'd expect. |
@reiv ah yes of course, thanks. in this PR, the tooltip no longer re-opens when switching back to the page, but i think ideally the tooltip wouldn't close when you leave the page ( |
@giladgray That sounds reasonable. I'll check it out when I get the time. |
thanks @reiv, you are a champion! 🏆 |
@giladgray Ok so the behavior in #2373 is actually a bit more nuanced than that, and your suggestion alone wouldn't change it. To break it down step by step:
In order to reconcile this with your suggestion, the popover would need to stay open as long as the popover target has focus, even if the hover interaction dictates that it should close. I'm not sure what this means for |
@reiv thank you for investigating! what you describe makes good sense about opened vs focused. perhaps we could add some "opened & focused & lost focus to other page" logic but that's starting to feel crufty, so let's call it here. the behavior in this PR is certainly an improvement so let's ship it! |
Fixes #2373
Changes proposed in this pull request:
This PR makes two changes to the way
Popover
handles focus events.FocusEvent.relatedTarget
¹ instead of wrappingdocument.activeElement
in a timeout to accomplish the same.[1] MDN says this is "experimental technology", but it seems to be available in all browsers save for IE < 9.