fix(Portal): take focus on mount and restore on unmount #1154
Conversation
Current coverage is 95.88% (diff: 100%)@@ master #1154 diff @@
==========================================
Files 879 879
Lines 4881 4888 +7
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 4680 4687 +7
Misses 201 201
Partials 0 0
|
| @@ -363,6 +363,14 @@ class Portal extends Component { | |||
| ) | |||
|
|
|||
| this.portal = this.node.firstElementChild | |||
| // don't take focus away from portals that close on blur | |||
| if (!closeOnTriggerBlur) { | |||
| this.triggerNode = document.activeElement | |||
There was a problem hiding this comment.
Nit: Could we use another name here?
If a portal is triggered programmatically, the activeElement will not necessarily be the triggerNode. These are only the same when the user clicked on a trigger, such as a button. How about something like previousActiveElement or similar?
| this.portal.setAttribute('tabindex', '-1') | ||
| this.portal.style.outline = 'none' | ||
| // wait a tick for things like popups which need to calculate where the popup shows up | ||
| setTimeout(() => this.portal && this.portal.focus()) |
There was a problem hiding this comment.
I'd like to pull this side effect out of the render method if at all possible. I haven't the bandwidth at the moment to find a better place myself, but open to suggestions.
There was a problem hiding this comment.
Technically this code is being called by either componentDidMount or componentDidUpdate depending on when 'open' is set to true which is after the render method. Or are you saying you'd like me to make a named helper function that's called here? There's no later life cycle function I can use to trigger this behavior
There was a problem hiding this comment.
Oh, I see, thanks for the clarification. From my initial pass I assumed this was called in a render() method itself. -1 for assumptions :)
levithomason
changed the title
|
Released in |
All portal based elements (modals, popups, etc) are not keyboard accessible because they're usually injected at the end of the dom and not in the flow of the document. This pull request fixes that by putting focus on the popup when open (except when closeOnTriggerBlur is true) and restores the focus to the last known focus point on close.