-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Remove focus state for notes as expected #73130
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
Remove focus state for notes as expected #73130
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
t-hamano
left a 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.
Thanks for the PR!
I believe the failing tests are related to this pull request. Could you investigate the cause?
|
@karthick-murugan FYI there's a merge conflict if you would please review/resolve as well |
I’m seeing consistent e2e test failures in block-comments.spec.js — specifically expect(locator).toBeFocused() after clicking the “x more replies” button. The issue seems to be that the new inline onBlur handler in comments.js immediately collapses/unselects a thread when focus leaves the thread element. During the “x more replies” click, a focus/blur sequence fires, triggering the collapse logic before focus can be restored — causing the test to fail. Therefore clicking on x more replies the focus of thread is lost. Please provide some ideas to fix this issue. |
|
If relevant, let's add the backport label as soon as possible so this gets more eyes |
|
I see, this is a bit complicated. The note loses focus in various situations, but as I understand it, we only need to collapse the note when we perform the following two actions:
The note should not be collapsed during any operations other than those mentioned. I am currently considering the best approach... |
|
@karthick-murugan, I was testing it in a separate pull request, and it seems to be working correctly: #73158 |
|
@karthick-murugan, I'd like to close this PR in favor of #73158, but thank you for your work! |
What?
Closes #72858
This PR fixes the focus state issue where individual notes and their corresponding blocks were not properly collapsing when focus moved away or when clicking outside the note. The fix ensures that expanded notes automatically collapse when:
Why?
Previously, when a note was expanded and the user clicked outside the note (either within the panel or outside it), or when focus moved away from the note, the note's focus state (drop shadow, white background, etc.) and the corresponding block highlighting would persist incorrectly. This created a confusing user experience where notes appeared to remain selected even after the user had moved on.
This issue was reported in #72858 and affects the Notes feature in the Gutenberg editor, particularly when using the collaborative sidebar.
How?
The implementation adds two key behaviors:
Focus-based collapse: Enhanced the
onBlurhandler in theThreadcomponent to detect when focus moves outside the thread element and automatically collapse the note by callingunselectThread().Selection clearing mechanism: Added a
clearSelectioncallback function in theCommentscomponent that:Testing Instructions
Video
REC-20251110143032.mp4