CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Block Comments: Improve input labels #71843
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
Block Comments: Improve input labels #71843
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. |
I think this can be done separately, and this PR can just focus on improving labeling. |
Sure thing, @Mamaduka. I’ll update both the description and the code so that this PR focuses solely on improving labeling. Thanks for pointing that out. |
@WordPress/gutenberg-design, would appreciate your feedback here. |
I'm not sure if using sequential numbers for labels is the right approach. For example, someone asks someone else to review comment 3. Then someone else deletes comment 2. Now comment 3 has become comment 2. If we add a number, I think it should be a comment ID. Personally, I think the visual label is too obtrusive. My suggestion is to make the comment container "truly" focusable and give it a more specific label. For example, in Google Docs, the comment container is a list item and is focusable. When the container is focused, a screen reader will announce:
cc @joedolson |
@t-hamano - Thank you for the detailed feedback! I've implemented the changes in my local as you suggested and tested them with VoiceOver. Still not yet pushed the code. Here's what I've addressed: Changes Made Removed Visual Labels: Eliminated the obtrusive visual labels as you suggested - the interface is now cleaner and less cluttered. Question About List Items Regarding your mention of list items - could you clarify what you mean by "the contents of the comments container list items"? Here is the video of the tested implementation. Please have a look and let me know your feedback. REC-20250924110258.mp4 |
@t-hamano I think sequential is fine; but the numbers should be persistent. If you delete comment 2, that should not change the labels on any other comment. Having an ID is fine, as long as they are easy to understand - a hash would be overwhelming, which is way sequential IDs makes more sense to me. Practically, though, since we are only supporting comments at the block level, and not at the text level, it may not be that relevant, as long as it's easy to get the relationship between the comment you're writing and the block it relates to. |
@karthick-murugan Sorry, my suggestions should be addressed in a separate PR. I have submitted #71883 to make threads focusable and improve keyboard navigation. In this PR (#71883), we should be able to simply focus on improving the labels.
I'm not sure how to make the numbers persistent other than using the comment ID. Any ideas? The current PR generates sequential numbers based on the length of the thread, so the comment numbers could change. |
Question: This PR improves the labels while also making them visible, but does every label need to be visible? |
First things first, let's keep this PR small and only focus on updating form labels, etc. We can continue the general discussion on tracking issues. Thank you, @jasmussen, for the update. There're a couple of ongoing PRs that should improve the behavior you're describing.
Clicking on comments should scroll to the associated block and highlight it. The spotlight mode is a global setting, so I would avoid enabling it on comment click. P.S. Why did we decide to use "scroll to + highlight" when a comment is selected, instead of selecting the block? The latter seems to be the most common UX in other apps. |
Thanks for the updates.
My mistake, I was using an old branch. Here's how that looks: I still feel like we need a toolbar change. The previously selected block has a toolbar that's sticky to the top of the screen.
I want to elaborate more on this, because just to be clear I'm not suggesting the user preference is changed at all. If you already have it on, you'll see no change. If you already have it off, the check in the dropdown will not be added, it's only the effect that will be applied, and only when the comment itself is selected or has focus inside. The idea is to use the scaffolding already built for spotlight mode, but apply it in more places. The feature itself is very underused right now. To visualize it I created this prototype: block.comments.mp4As noted, I don't think it's a blocker, but let me know if the behavior makes sense! |
@jasmussen, I think I understand it better. We should probably create a separate issue for this and move the discussion there. |
Created this issue, let me know if that works for you: #71891 I omitted mention of the select-block and toolbar change. I suspect there are some technical aspects there that you are perhaps better equipped to capture. |
Updated the conflicts and I have updated the labels such like "Comments dialog: Open comment 196. Author admin 0 replies. Status: active. Posted 22/09/2025" where 196 is the commend id. This was implemented based on our earlier discussions. Please review the video, and let me know if further adjustments are needed. REC-20250925155911.mp4 |
Although various issues are being discussed, let's clarify the purpose of this PR and make minimal changes. I think the purpose of this PR is not to make threads focusable, but to only improve the label on the textarea itself in the comment form to make it clearer. What do you think?
|
selectBlock moves focus to the block, which I don't think is ideal from an a11y perspective, so we went with the scroll+highlight approach. See: #71308 (review) |
100%. Let's focus only on that. Atomic PRs/commits are always easy to review and merge. |
<Thread | ||
key={ thread.id } | ||
thread={ thread } | ||
commentId={ thread.id } |
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 value is already available via the thread
object. There's no need to pass additional props.
|
||
const CommentBoard = ( { | ||
thread, | ||
commentId, |
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.
Same here.
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.
@Mamaduka - Updated both the feedbacks. Thank You.
// Generate contextual label for new comment | ||
// For new comments, use a generic label since we don't have an ID yet | ||
const commentLabel = _x( 'New Comment', 'Add new comment label' ); |
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.
// Generate contextual label for new comment | |
// For new comments, use a generic label since we don't have an ID yet | |
const commentLabel = _x( 'New Comment', 'Add new comment label' ); | |
const commentLabel = __( 'New Comment' ); |
The code comments and context seem redundant to me.
<label | ||
htmlFor={ inputId } | ||
className="editor-collab-sidebar-panel__comment-label" | ||
> | ||
{ labelText || __( 'Comment' ) } | ||
</label> |
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.
Instead of using a label element, I think it would be better to reuse the VisuallyHidden component.
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.
+1.
onKeyDown={ ( event ) => { | ||
if ( event.key === 'Enter' || event.key === ' ' ) { | ||
event.preventDefault(); | ||
handleCommentSelect( thread.id ); | ||
} | ||
} } | ||
role="button" | ||
tabIndex={ 0 } | ||
aria-label={ sprintf( | ||
// translators: %1$s: comment identifier, %2$s: author name, %3$s: reply count, %4$s: status, %5$s: date. | ||
_x( | ||
'Comments dialog. Open comment %1$s. Author %2$s. %3$s replies. Status: %4$s. Posted %5$s. Click to focus.', | ||
'Comment accessibility label with full context' | ||
), | ||
thread.id, | ||
thread?.author_name || 'Unknown', | ||
thread?.replies?.length || 0, | ||
thread?.status === 'approved' ? 'resolved' : 'active', | ||
thread?.date | ||
? new Date( thread.date ).toLocaleDateString() | ||
: 'unknown date' | ||
) } | ||
{ ...( thread?.replies?.length > 0 && { | ||
'aria-expanded': isFocused, | ||
} ) } |
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 don't think these changes are necessary; the aria-label on the thread container itself could be discussed in #71883.
// translators: %1$s: comment identifier, %2$s: author name | ||
_x( | ||
'Reply to Comment %1$s by %2$s', | ||
'Reply to specific comment with author context' | ||
), |
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.
// translators: %1$s: comment identifier, %2$s: author name | |
_x( | |
'Reply to Comment %1$s by %2$s', | |
'Reply to specific comment with author context' | |
), | |
// translators: %1$s: comment identifier, %2$s: author name | |
__( 'Reply to Comment %1$s by %2$s' ), |
// translators: %1$s: comment identifier, %2$s: author name. | ||
_x( | ||
'Edit Comment %1$s by %2$s', | ||
'Edit specific comment with author context' | ||
), |
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.
// translators: %1$s: comment identifier, %2$s: author name. | |
_x( | |
'Edit Comment %1$s by %2$s', | |
'Edit specific comment with author context' | |
), | |
// translators: %1$s: comment identifier, %2$s: author name. | |
__( 'Edit Comment %1$s by %2$s' ), |
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.
@t-hamano - Updated all the changes.
While I generally like visible labels, I think it's OK for these not to be visible. These are single-field forms, triggered using a button that says 'Comment', and with a submit button that visually says 'Comment' or 'Reply'. With good visual indication of which block a comment is associated with, there isn't a lot that a visible label on the field will add, and it doesn't need to be distinguished from other nearby input fields. It's very similar to a search input in that respect. |
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.
LGTM!
Thanks for your effort, @karthick-murugan! Do you mind rebasing your branch on top of the latest trunk? We merged the end-to-end test coverage yesterday and want to ensure that we update locators as needed. |
…prove-comment-input-labels
@Mamaduka - Rebased my branch with trunk. Thank You. |
What?
Closes #71681
Improves comment input labels for better accessibility by implementing sequential numbering and visible labels for comment forms.
Why?
Currently, all comment inputs have the same generic "Comment" label, which provides no context for screen reader users. This makes the collaborative commenting feature largely unusable for users with assistive technologies, as they cannot distinguish between different comment contexts or know which comment they're replying to.
This PR addresses the accessibility issue by implementing unique, contextual labels for each comment input, following the recommendations from @joedolson in the issue discussion.
How
Sequential Comment Numbering: Comments are now numbered sequentially (Comment 1, Comment 2, etc.) based on creation order, not content order.
Contextual Labels:
Visible Labels: Labels are now visible to all users (not just screen readers) to facilitate team communication.
Smart Label Placement: Comment labels appear in the header area above the gravatar image for better visual hierarchy.
Implementation Details:
CommentForm
component withlabelText
propsCommentBoard
component withshowLabel
prop to control label visibilityComments
component to pass sequential numbering to child componentsAddComment
component to calculate next comment numberTesting Instructions
Video
REC-20250923180954.mp4