-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Colors: fix focus issue #21171
Colors: fix focus issue #21171
Conversation
.contains( document.activeElement ) | ||
) { | ||
return; | ||
} |
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 function is supposed to trigger only when a block gets selected IIRC, which means I'm not sure we need this check. Why is this function being called in that particular case, isn't the block already selected?
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.
Because the block is remounting :( Ideally, this shouldn't run if the block is remounting and the props didn't change. I'm not sure how to make that work. Do you have any ideas?
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 the block is remounting?
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.
The block shouldn't be remounting when a color changes IMO
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.
That's what I think as well: #21163 (comment)
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'm not familiar enough with the useColors hook to figure out why and how it's remounting children.
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 see there's some rendering of cloned elements going on... 🤷♀️
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.
Looks like we can't do this particular fix anyway, it's breaking a lot of stuff. We need to find a way to not remount component nested in the color components.
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.
Another reason why I really do not like this way of magically updating the className and style props.
Size Change: +35 B (0%) Total Size: 857 kB
ℹ️ View Unchanged
|
Closing, #21037 fixes this issue. |
Description
Fixes #21163.
How has this been tested?
Screenshots
Types of changes
Checklist: