Revisions: Specify block level diff status via aria-label#77779
Revisions: Specify block level diff status via aria-label#77779himanshupathak95 wants to merge 12 commits into
Conversation
8352414 to
50879fd
Compare
|
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. |
| const blockRef = useRef(); | ||
| useBlockElementRef( block.clientId, blockRef ); | ||
|
|
||
| useEffect( () => { | ||
| const el = blockRef.current; | ||
| if ( ! el || ! diffStatus ) { | ||
| el?.removeAttribute( 'aria-label' ); | ||
| return; | ||
| } | ||
|
|
||
| const blockTitle = getBlockType( block.name )?.title; | ||
| if ( ! blockTitle ) { | ||
| return; | ||
| } | ||
|
|
||
| const diffLabel = getDiffStatusLabel( diffStatus, blockTitle ); | ||
| if ( ! diffLabel ) { | ||
| return; | ||
| } | ||
|
|
||
| el.setAttribute( 'aria-label', diffLabel ); | ||
| }, [ block.clientId, diffStatus, block.name ] ); |
There was a problem hiding this comment.
I feel this approach is risky because it modifies the DOM from the outside, which should be managed by other components themselves, leading to unpredictable behavior.
I haven't tested it, but is it possible to inject the diff label via wrapperProps?
diff --git a/packages/editor/src/components/post-revisions-preview/revisions-canvas.js b/packages/editor/src/components/post-revisions-preview/revisions-canvas.js
index 0e9552dff01..852a27803db 100644
--- a/packages/editor/src/components/post-revisions-preview/revisions-canvas.js
+++ b/packages/editor/src/components/post-revisions-preview/revisions-canvas.js
@@ -9,7 +9,7 @@ import clsx from 'clsx';
import { Spinner } from '@wordpress/components';
import { privateApis as blockEditorPrivateApis } from '@wordpress/block-editor';
import { useSelect } from '@wordpress/data';
-import { useEffect, useRef } from '@wordpress/element';
+import { useEffect } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';
import { getBlockType } from '@wordpress/blocks';
import { sprintf, __ } from '@wordpress/i18n';
@@ -26,9 +26,7 @@ import {
} from './diff-format-types';
import { useDiffMarkers } from './diff-markers';
-const { usePrivateStyleOverride, useBlockElementRef } = unlock(
- blockEditorPrivateApis
-);
+const { usePrivateStyleOverride } = unlock( blockEditorPrivateApis );
// SVG filter for removed blocks: grayscale + red tint
const REVISION_REMOVED_FILTER_SVG = `
@@ -120,31 +118,14 @@ function getDiffStatusLabel( status, blockTitle ) {
*/
function withRevisionDiffClasses( BlockListBlock ) {
return ( props ) => {
- const { block, className } = props;
+ const { block, className, wrapperProps } = props;
const diffStatus = block?.__revisionDiffStatus?.status;
- const blockRef = useRef();
- useBlockElementRef( block.clientId, blockRef );
-
- useEffect( () => {
- const el = blockRef.current;
- if ( ! el || ! diffStatus ) {
- el?.removeAttribute( 'aria-label' );
- return;
- }
-
- const blockTitle = getBlockType( block.name )?.title;
- if ( ! blockTitle ) {
- return;
- }
-
- const diffLabel = getDiffStatusLabel( diffStatus, blockTitle );
- if ( ! diffLabel ) {
- return;
- }
-
- el.setAttribute( 'aria-label', diffLabel );
- }, [ block.clientId, diffStatus, block.name ] );
+ const blockTitle = getBlockType( block.name )?.title;
+ const diffLabel =
+ diffStatus && blockTitle
+ ? getDiffStatusLabel( diffStatus, blockTitle )
+ : undefined;
const enhancedClassName = clsx( className, {
'is-revision-added': diffStatus === 'added',
@@ -152,7 +133,16 @@ function withRevisionDiffClasses( BlockListBlock ) {
'is-revision-modified': diffStatus === 'modified',
} );
- return <BlockListBlock { ...props } className={ enhancedClassName } />;
+ return (
+ <BlockListBlock
+ { ...props }
+ className={ enhancedClassName }
+ wrapperProps={ {
+ ...wrapperProps,
+ ...( diffLabel && { 'aria-label': diffLabel } ),
+ } }
+ />
+ );
};
}|
So, I was working on this to use the wrapperProps as suggested. I replaced the useBlockElementRef + useEffect + setAttribute with a wrapperProps containing 'aria-label'. Here's what I found - useBlockProps assembles the final props with 'aria-label': blockLabel as an explicit key after ...wrapperProps, so the spread is overridden. I think we need to change this to-
- 'aria-label': blockLabel,
+ 'aria-label': wrapperProps[ 'aria-label' ] || blockLabel,Also, I found out that paragraph block ignores blockProps['aria-label'] The Paragraph block sets aria-label explicitly, after spreading { ...blockProps }: gutenberg/packages/block-library/src/paragraph/edit.js Lines 143 to 160 in 5d981ce Simplest fix I can think of is to destructure aria-label out of blockProps in paragraph block's edit, and prefer the injected value when it differs from the default. Somethig like this - const { 'aria-label': injectedAriaLabel, ...restBlockProps } = blockProps;
const defaultLabel = __( 'Block: Paragraph' );
<RichText
{ ...restBlockProps }
aria-label={
injectedAriaLabel !== defaultLabel
? injectedAriaLabel
: RichText.isEmpty( content )
? __( 'Empty block; start writing or type forward slash to choose a block' )
: defaultLabel
}
/>Does this approach sound good? |
This seems like the right approach to me. cc @ellatrix |
50879fd to
3547840
Compare
|
There was an e2e test failing earlier likely introduced by the paragraph/edit.js change. I think the reason was that the test inserts a "Success Message" block variation. Active variations change blockTitle, so blockLabel becomes "Block: Success Message". The previous code compared injectedAriaLabel against the hardcoded "Block: Paragraph" and since "Block: Success Message" !== "Block: Paragraph", the condition mistakenly treated the variation label as a diff-injected label, changing the paragraph's aria-label away from "Block: Paragraph". The test, which selects by { name: 'Block: Paragraph' }, then couldn't find the element. I reconstructed blockLabel from blockProps['data-title'] and compared against that instead. |
|
I believe the approach of this PR is correct, but I would also appreciate any feedback from @ellatrix. |
| const blockEditingMode = useBlockEditingMode(); | ||
| const { 'aria-label': injectedAriaLabel, ...restBlockProps } = blockProps; | ||
| // translators: %s: block type title e.g. "Paragraph" | ||
| const blockLabel = sprintf( __( 'Block: %s' ), blockProps[ 'data-title' ] ); |
There was a problem hiding this comment.
I don't think it's a good idea to force blocks to change. Why are moving this here?
There was a problem hiding this comment.
Can't we apply the label after spreading rich text props in the block props?
There was a problem hiding this comment.
Applying it after the spread was overriding the injected diff labels.
| id: `block-${ clientId }${ htmlSuffix }`, | ||
| role: 'document', | ||
| 'aria-label': blockLabel, | ||
| 'aria-label': wrapperProps[ 'aria-label' ] || blockLabel, |
There was a problem hiding this comment.
We should be careful not to introduce a new API here. If it wasn't possible before for extenders to set an aria label, then I'm not sure if we should add that. Perhaps we can check the store if revisions mode is on, or add some React context?
There was a problem hiding this comment.
Hi @ellatrix, thanks for the feedback!
What do you think about using a private React context (e.g., BlockAriaLabelOverrideContext exported via lock-unlock)? We could provide the diff label from revisions-canvas.js and consume it directly in useBlockProps.
This approach lets us completely revert the changes to the paragraph block and avoids passing anything through wrapperProps.
What?
Part of #77530
Adds an
aria-labelto blocks in the revisions canvas that reflects their diff status (added, removed, or modified).Why?
Screen reader users had no programmatic indication of a block's diff status when browsing the revisions canvas. Navigating to a changed block would only announce the block type (e.g. "Paragraph block") with no mention of what changed.
How?
Adds a
useEffectinsidewithRevisionDiffClassesthat writesaria-labeldirectly to the block's DOM element after render, usinguseBlockElementRefto get a ref to the element.A helper
getDiffStatusLabel()builds the translatable label string based on the diff status and block type title:Added Block: ParagraphRemoved Block: ImageModified Block: HeadingTesting Instructions
Screenshots or screencast
Before
Screen.Recording.2026-04-28.at.19.26.39.mov
After
Screen.Recording.2026-04-28.at.16.54.14.mov