Skip to content

Conversation

@karthick-murugan
Copy link
Contributor

@karthick-murugan karthick-murugan commented Aug 22, 2025

What?

Closes #71244

This PR introduces a visual highlight for the related block when a comment is selected in the sidebar. It ensures that users can quickly identify which block a comment is associated with inside the editor.

Why?

Currently, when comments are shown in the sidebar, there is no clear indication in the canvas of which block they belong to. This creates confusion and slows down workflows for authors and reviewers.

By highlighting the related block whenever its associated comment is selected, users will have a clear visual indicator of context, improving the overall commenting experience.

How?

When a comment is selected:

The related block is programmatically selected and a temporary data-comment-highlight="true" attribute is applied to the block’s DOM element inside the editor iframe.

A CSS rule applies a subtle highlight style to the block (e.g., outline or background fade).

When the comment is deselected or another block/comment is selected:

The data-comment-highlight attribute is removed, restoring the block to its normal appearance.

Implementation details

Uses iframe.contentDocument.querySelector('[data-block="..."]') to locate the block.

Adds data-comment-highlight="true" instead of class manipulation, since class selectors weren’t applying reliably inside the editor iframe.

Adds scoped CSS targeting [data-comment-highlight="true"] to visually differentiate highlighted blocks.

Testing Instructions

Open a post or page in the block editor.

Add a few blocks and insert comments linked to those blocks.

From the sidebar, click on a comment.

✅ The related block in the editor should receive a visible highlight.

Screenshot/ Video

REC-20250822175358.mp4

@github-actions
Copy link

github-actions bot commented Aug 22, 2025

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: karthick-murugan <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: joedolson <[email protected]>
Co-authored-by: Sahil1617 <[email protected]>
Co-authored-by: joemcgill <[email protected]>
Co-authored-by: jeffpaul <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

selectBlock( relatedBlocks[ 0 ] );

const iframe = document.querySelector(
'iframe[name="editor-canvas"]'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selecting and manipulating elements in the DOM this way feels fragile. Did you explore using a block attribute / datastore to tie the comment to the block/highlighting?

Perhaps we could leverage the existing mechanism that ties between blocks and comments - clicking on a block highlights the related comment thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamsilverstein - Updated the feedback.

REC-20250826195838.mp4

@t-hamano
Copy link
Contributor

I’ve adjusted the flash timing so the effect is more noticeable.

Since flashBlock is a public API, I would recommend not changing the API in this PR, and I think it should be considered in a follow-up.

@karthick-murugan
Copy link
Contributor Author

I’ve adjusted the flash timing so the effect is more noticeable.

Since flashBlock is a public API, I would recommend not changing the API in this PR, and I think it should be considered in a follow-up.

Got it @t-hamano. I will create a new issue and a PR for the flashBlock API changes. Also will remove the changes from this PR. Thank you.

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple more notes. I think we're close to finishing this.

Comment on lines 106 to 116
const [ focusThread, setFocusThread ] = useState(
showCommentBoard && blockCommentId ? blockCommentId : null
);

// Sync comment ↔ block highlighting bidirectionally.
useEffect( () => {
// Highlight comment when block is selected.
if ( blockCommentId && ! focusThread ) {
setFocusThread( blockCommentId );
}
}, [ blockCommentId, focusThread, blocks, setFocusThread ] );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar sync effects can be tricky. I know there are "checks" in place, but this effect will run a lot of times. Each block update will trigger the effect.

IMO, we have a clear use, even "click on a comment element", there should be no need for any side-effects. Technically, we should be able to house all required logic in handleCommentSelect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mamaduka – Removing the useEffect will stop highlighting comments when any block is selected, but the block flashing when a comment is selected will continue to work as expected. Do you think it’s fine if I proceed with this approach?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a big blocker; we can start here and refactor later.

I just noticed that the blocks dependency isn't required for this effect; what was the reason for adding it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its just for syncing the highlight.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an odd hack.

Comment on lines 118 to 139
// Handle comment selection.
const handleCommentSelect = useCallback(
( threadId ) => {
// Clear the add comment form when selecting a comment.
setShowCommentBoard( false );

// Set the focused thread.
setFocusThread( threadId );

// Flash the related block when a comment is clicked.
if ( blocks ) {
const relatedBlockClientId = findBlockByCommentId(
threadId,
blocks
);
if ( relatedBlockClientId ) {
flashBlock( relatedBlockClientId );
}
}
},
[ blocks, flashBlock, setFocusThread, setShowCommentBoard ]
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need two handleCommentSelect functions? The logic should be co-located in a single callback below.

P.S. Memoization is sort of useless here. The blocks dependency will often change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mamaduka - Updated both the changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should've been clearer. What I was suggesting was moving all the logic into handleCommentSelect defined in the Thread component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mamaduka - Thanks for the clarification! I've moved all the logic into the handleCommentSelect function defined in the Thread component as you suggested.

Changes made:
✅ Removed the handleCommentSelect function from the main Comments component
✅ Consolidated all logic (clearing comment board, setting focus, flashing blocks, scrolling) into the single handleCommentSelect in the Thread component
✅ Removed the useEffect for comment ↔ block highlighting sync
✅ Removed unnecessary useCallback memoization

The code is now much cleaner with a single source of truth for comment selection logic, and no performance issues from the frequently changing blocks dependency in memoization.

Comment on lines 177 to 187
// Flash the related block when a comment is clicked.
if ( blocks ) {
const relatedBlockClientId = findBlockByCommentId(
threadId,
blocks
);
if ( relatedBlockClientId ) {
// Use longer timeout for comment-to-block highlighting (800ms)
flashBlock( relatedBlockClientId, 800 );
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just use the relatedBlock value here. There's no need to find the same value twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mamaduka - Yes done.

Comment on lines 110 to 116
// Sync comment ↔ block highlighting bidirectionally.
useEffect( () => {
// Highlight comment when block is selected.
if ( blockCommentId && ! focusThread ) {
setFocusThread( blockCommentId );
}
}, [ blockCommentId, focusThread, blocks, setFocusThread ] );
Copy link
Member

@Mamaduka Mamaduka Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were keeping this effect for now? Was the removal intentional?

Sorry, just saw this mentioned in #71308 (comment). But it's still not clear if the removal was intentional.

Related: #71308 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mamaduka - Not intentional. Added back.

Comment on lines 184 to 195
// Flash the related block when a comment is clicked.
if ( relatedBlock ) {
flashBlock( relatedBlock );
}

// Scroll to the related block element if available.
if ( relatedBlock && relatedBlockElement ) {
relatedBlockElement.scrollIntoView( {
behavior: 'instant',
block: 'center',
} );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should combine these so that they can "flash" the block highlight after the scroll.

P.S. Most of the inline comments inside the handleCommentSelect function aren't needed, IMO. What each part of the code is doing is clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mamaduka - Good point! I've combined the flash and scroll so the block highlights after scrolling, and remove the unnecessary inline comments since the code is self-explanatory. Thank You.

@Mamaduka Mamaduka added the [Type] Enhancement A suggestion for improvement. label Sep 19, 2025
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you so much for sticking with it until the end. I believe this is the best we can do for now. If necessary, let's follow up by removing unnecessary effects, improving performance, and refactoring.

@Mamaduka Mamaduka changed the title Highlight Related Block Block Comments: Highlight the related block Sep 19, 2025
@t-hamano t-hamano merged commit d6210ac into WordPress:trunk Sep 19, 2025
73 of 75 checks passed
@github-project-automation github-project-automation bot moved this from 🔎 Needs Review to ✅ Done in WordPress 6.9 Editor Tasks Sep 19, 2025
@github-actions github-actions bot added this to the Gutenberg 21.8 milestone Sep 19, 2025
@karthick-murugan karthick-murugan deleted the highlight-related-block branch September 19, 2025 12:20
@adamsilverstein
Copy link
Member

Nice work here @karthick-murugan - thanks for sticking with this thru all the feedback (and thanks @t-hamano and @Mamaduka for all the helpful guidance), this was a tricky issue to get working well.

adamsilverstein added a commit to adamsilverstein/gutenberg that referenced this pull request Sep 22, 2025
* Image size fix in lightbox

* Revert "Image size fix in lightbox"

This reverts commit 63f81c1.

* Highlight and scroll to related blocks

* Feedback changes updated

* Latest Feedback updated

* Feedback changes updated

* Feedback changes added

* Feedback changes

* Feedback changes

* Remove unecessarunnecessary effect and new api

* Fixed feedback issues

* Added flashblock and updated highlight issues

* Update packages/editor/src/components/collab-sidebar/comments.js

Co-authored-by: Adam Silverstein <[email protected]>

* Feedback changes

* Feedback changes updated

* Update packages/editor/src/components/collab-sidebar/comments.js

Co-authored-by: Adam Silverstein <[email protected]>

* Flashblock and scroll updates added

* Removing FlashBlock API changes

* Remove duplicate handleCommentSelect

* handleCommentSelect to Thread Component

* Updating feedback

* FlashBlock after scroll

---------

Co-authored-by: karthick-murugan <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: joedolson <[email protected]>
Co-authored-by: Sahil1617 <[email protected]>
Co-authored-by: joemcgill <[email protected]>
Co-authored-by: jeffpaul <[email protected]>
@t-hamano t-hamano added [Feature] Notes Phase 3 of the Gutenberg roadmap around block commenting and removed Collaborative Workflows Phase 3 of the Gutenberg roadmap around all-things related to collaborative workflows labels Oct 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Notes Phase 3 of the Gutenberg roadmap around block commenting [Type] Enhancement A suggestion for improvement.

Projects

5 participants