[FocusTrap] Refactor & cleanup #38878
Conversation
mnajdova
added
component: FocusTrap
Netlify deploy previewhttps://deploy-preview-38878--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
| const rootElement = rootRef.current; | ||
| const doc = ownerDocument(rootRef.current); |
There was a problem hiding this comment.
It's already defined in the upper scope (the useEffect)
| (nativeEvent && reactFocusEventTarget.current !== nativeEvent.target) || | ||
| doc.activeElement !== reactFocusEventTarget.current | ||
| ) { | ||
| if (doc.activeElement !== reactFocusEventTarget.current) { |
There was a problem hiding this comment.
The second part of the condition should always capture the first part too. I can't find scenario where the focusin's event.target will not be the document.activeElement
There was a problem hiding this comment.
I don't see a need for it either. As far as I now, doc.activeElement moves synchronously, so nativeEvent.target === document.activeElement at all time. Can't see a need for it from #21610.
| (nativeEvent && reactFocusEventTarget.current !== nativeEvent.target) || | ||
| doc.activeElement !== reactFocusEventTarget.current | ||
| ) { | ||
| if (doc.activeElement !== reactFocusEventTarget.current) { |
There was a problem hiding this comment.
I don't see a need for it either. As far as I now, doc.activeElement moves synchronously, so nativeEvent.target === document.activeElement at all time. Can't see a need for it from #21610.
|
Nice, thanks for the cleanup. |
Things done in the PR:
containfunction back in theuseEffectnativeEvent- the reason for adding this event handler was to check if the focus was invoked with different target based on [TrapFocus] Fix portal support #21610 (comment), but there can never be a difference between thefocusin'sevent.targetelement and thedocument.activeElement- and this check is already captured in the second part of the condition (I was testing the scenario that was shared in that reproduction and it still works as expected, and all tests pass too), so everything should be fine.