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
[TreeView] Simplify focus logic #22098
Conversation
focusin is not cancelable: https://developer.mozilla.org/en-US/docs/Web/API/Element/focusin_event
It's not a ref but the actual element
React.useEffect(() => { | ||
if (nodeRef && treeId) { | ||
const handleFocusIn = (event) => { | ||
event.preventDefault(); |
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.
Nice, I feel like I’ve removed this so many times 😂 But I keep somehow adding it back.
function handleFocus(event) { | ||
// DOM focus stays on the tree which manages focus with aria-activedescendant | ||
if (event.target === event.currentTarget) { | ||
ownerDocument(event.target).getElementById(treeId).focus(); |
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.
Just realized that I no longer guard against falsy treeId
. What was the intention behind that @joshwooding ?
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.
And if (ownerDocument(nodeRef).activeElement !== tree)
guarded against tree
being null
. I'm not so sure anymore if we should crash or issue a console error (which might be missed).
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.
@eps1lon If the context isn't there it would error but probably not worth guarding against. I think it's fine if it crashed, I didn't intend to guard against a null tree.
react@experimental
changed how focus events are handled (they're listening to focusin/focusout now internally) which broke some TreeItem tests. It seems like we can simplify this by relying on React's event system which should be more robust long term.focusin
is not cancelablenodeRef
->treeitemElement
because it is not a ref (a ref has a{ current: T | null }
shape)Verified that this PR indeed fixes tests on
react@experimental
with eps1lon/material-ui@test/fix-react-next...eps1lon:test/fix-react-experimental