Skip to content
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.

Sign up for GitHub

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

Jump to bottom

List view: Refactor ARIA attributes #48461

Merged
merged 26 commits into from
Apr 14, 2023
Merged

List view: Refactor ARIA attributes #48461

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
0034510
Fix ARIA attributes.
alexstine Feb 26, 2023
e28899c
Add assertive for selection read.
alexstine Feb 26, 2023
ebd1773
Merge branch 'trunk' of github.com:wordpress/gutenberg into update/li…
alexstine Feb 27, 2023
ba860a1
Merge branch 'trunk' of github.com:wordpress/gutenberg into update/li…
alexstine Mar 19, 2023
9749780
Merge branch 'trunk' of github.com:wordpress/gutenberg into update/li…
alexstine Mar 31, 2023
21fc852
Merge branch 'trunk' of github.com:wordpress/gutenberg into update/li…
alexstine Apr 3, 2023
55f43ac
Remove useless useEffect. Refactor expanded to use isExpanded attribute.
alexstine Apr 4, 2023
88788ba
Add prop to disable aria-expanded on tree grid tr so I can override o…
alexstine Apr 4, 2023
4c30383
Try to fix failing e2e test
andrewserong Apr 4, 2023
4c2b450
Update list view tests
andrewserong Apr 4, 2023
08967ae
Restore removed useEffect.
alexstine Apr 4, 2023
54797d5
Revert TreeGrid changes.
alexstine Apr 4, 2023
0833362
Finish revert.
alexstine Apr 4, 2023
115cfdc
Eliminate aria-expanded on main tr.
alexstine Apr 4, 2023
0630cf1
Add data-expanded back to TreeGrid. Need to detect when to expand via…
alexstine Apr 4, 2023
b56270b
Fix expander.
alexstine Apr 5, 2023
90f9425
Move isExpanded undefined to tr directly.
alexstine Apr 5, 2023
eed5466
Fix E2E.
alexstine Apr 5, 2023
17cdc5b
Retain dark color for button when expanded
andrewserong Apr 5, 2023
3c9068c
Add title fallback.
alexstine Apr 5, 2023
714f331
Save some verbosity on block options button.
alexstine Apr 5, 2023
87b8c8c
Fix E2E.
alexstine Apr 5, 2023
e7a577b
Add components changelog entry.
alexstine Apr 5, 2023
cffe7d1
Fix conflicts.
alexstine Apr 5, 2023
f2abb6a
Refresh.
alexstine Apr 14, 2023
7e5a6ba
Refresh, again.
alexstine Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/block-editor/src/components/list-view/block-select-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ function ListViewBlockSelectButton(
onDragStart,
onDragEnd,
draggable,
isExpanded,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this component was no longer receiving this prop.

ariaLabel,
ariaDescribedBy,
},
ref
) {
Expand Down Expand Up @@ -76,7 +79,9 @@ function ListViewBlockSelectButton(
onDragEnd={ onDragEnd }
draggable={ draggable }
href={ `#block-${ clientId }` }
aria-hidden={ true }
aria-label={ ariaLabel }
aria-describedby={ ariaDescribedBy }
aria-expanded={ isExpanded }
>
<ListViewExpander onClick={ onToggleExpanded } />
<BlockIcon icon={ blockInformation?.icon } showColors />
Expand Down
51 changes: 12 additions & 39 deletions packages/block-editor/src/components/list-view/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,7 @@ import {
} from '@wordpress/components';
import { useInstanceId } from '@wordpress/compose';
import { moreVertical } from '@wordpress/icons';
import {
useState,
useRef,
useEffect,
useCallback,
memo,
} from '@wordpress/element';
import { useState, useRef, useCallback, memo } from '@wordpress/element';
import { useDispatch, useSelect } from '@wordpress/data';
import { sprintf, __ } from '@wordpress/i18n';

Expand Down Expand Up @@ -53,7 +47,6 @@ function ListViewBlock( {
path,
isExpanded,
selectedClientIds,
preventAnnouncement,
isSyncedBranch,
} ) {
const cellRef = useRef( null );
Expand Down Expand Up @@ -111,20 +104,13 @@ function ListViewBlock( {
level
);

let blockAriaLabel = __( 'Link' );
if ( blockInformation ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice that the condition to check if blockInformation is truthy has been removed. From a little digging, I think the check was added back in #38775 to deal with an issue with useSelect where it's possible that blockInformation will be null in some circumstances. So, I'm not too sure, but it might be necessary to keep the condition around. Just pinging @talldan who originally implemented the fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that is the case, please check the variable directly below this line. It was not included in this conditional and still used blockInformation to get the title for building the options aria-label.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the case of the below settingsAriaLabel it's doing a check for blockInformation within its ternary before attempting to access blockInformation.title, so it didn't need to be included in the if block. For blockAriaLabel both outcomes of the ternary are accessing properties of blockInformation, so if blockInformation were to be null then an error would be thrown.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too sure if that check is still needed, though, but because #38775 introduced the check, it'd probaly be good to verify before removing the check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah, I totally missed that sneaky conditional on that line. NVM.

blockAriaLabel = isLocked
? sprintf(
// translators: %s: The title of the block. This string indicates a link to select the locked block.
__( '%s link (locked)' ),
blockInformation.title
)
: sprintf(
// translators: %s: The title of the block. This string indicates a link to select the block.
__( '%s link' ),
blockInformation.title
);
}
const blockAriaLabel = isLocked
? sprintf(
// translators: %s: The title of the block. This string indicates a link to select the locked block.
__( '%s (locked)' ),
blockInformation.title
)
: blockInformation.title;

const settingsAriaLabel = blockInformation
? sprintf(
Expand All @@ -134,8 +120,7 @@ function ListViewBlock( {
)
: __( 'Options' );

const { isTreeGridMounted, expand, collapse, BlockSettingsMenu } =
useListViewContext();
const { expand, collapse, BlockSettingsMenu } = useListViewContext();

const hasSiblings = siblingBlockCount > 0;
const hasRenderedMovers = showBlockMovers && hasSiblings;
Expand All @@ -149,15 +134,6 @@ function ListViewBlock( {
{ 'is-visible': isHovered || isFirstSelectedBlock }
);

// If ListView has experimental features related to the Persistent List View,
alexstine marked this conversation as resolved.
Show resolved Hide resolved
// only focus the selected list item on mount; otherwise the list would always
// try to steal the focus from the editor canvas.
useEffect( () => {
if ( ! isTreeGridMounted && isSelected ) {
cellRef.current.focus();
}
}, [] );

const onMouseEnter = useCallback( () => {
setIsHovered( true );
toggleBlockHighlight( clientId, true );
Expand Down Expand Up @@ -250,17 +226,13 @@ function ListViewBlock( {
data-block={ clientId }
data-expanded={ canExpand ? isExpanded : undefined }
isExpanded={ canExpand ? isExpanded : undefined }
aria-selected={ !! isSelected || forceSelectionContentLock }
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
ref={ rowRef }
>
<TreeGridCell
className="block-editor-list-view-block__contents-cell"
colSpan={ colSpan }
ref={ cellRef }
aria-label={ blockAriaLabel }
aria-selected={ !! isSelected || forceSelectionContentLock }
aria-expanded={ canExpand ? isExpanded : undefined }
aria-describedby={ descriptionId }
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
>
{ ( { ref, tabIndex, onFocus } ) => (
<div className="block-editor-list-view-block__contents-container">
Expand All @@ -277,9 +249,10 @@ function ListViewBlock( {
currentlyEditingBlockInCanvas ? 0 : tabIndex
}
onFocus={ onFocus }
isExpanded={ isExpanded }
isExpanded={ canExpand ? isExpanded : undefined }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to match the others.

selectedClientIds={ selectedClientIds }
preventAnnouncement={ preventAnnouncement }
ariaLabel={ blockAriaLabel }
ariaDescribedBy={ descriptionId }
/>
<div
className="block-editor-list-view-block-select-button__description"
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/components/list-view/leaf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const ListViewLeaf = forwardRef(
level={ level }
positionInSet={ position }
setSize={ rowCount }
disableAriaExpanded={ true }
Copy link
Contributor

Choose a reason for hiding this comment

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

This causes the clickable expander to no longer render, which removes the ability to expand / collapse container blocks via the mouse.

{ ...props }
>
{ children }
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/list-view/use-block-selection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ export default function useBlockSelection() {
}

if ( label ) {
speak( label );
speak( label, 'assertive' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated to this PR but this required a fix.

}
},
[
Expand Down
9 changes: 7 additions & 2 deletions packages/components/src/tree-grid/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ function UnforwardedTreeGrid(
const canExpandCollapse = 0 === currentColumnIndex;
const cannotFocusNextColumn =
canExpandCollapse &&
activeRow.getAttribute( 'aria-expanded' ) === 'false' &&
( activeRow.getAttribute( 'data-expanded' ) === 'false' ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to add this attribute check here or else we'll never know if it is okay to expand or collapse with the keyboard. Should probably document an example in README. Likely need to update unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @alexstine , would you be able to work on a follow-up where you add info to the README, or (even better) add a Storybook example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ciampo Storybook is tricky for me to use so doubt I would be much help making an example that might not be testable by me. I only use Storybook when stuff is not testable any other way because accessibility is not great and components used in-practice are much easier to test.

For the README example, let me open a follow-up. That should be easy enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable, thank you! I'll see if I can help and add the Storybook example myself

activeRow.getAttribute( 'aria-expanded' ) === 'false' ) &&
keyCode === RIGHT;

if ( ( [ LEFT, RIGHT ] as number[] ).includes( keyCode ) ) {
Expand All @@ -112,6 +113,8 @@ function UnforwardedTreeGrid(
// Left:
// If a row is focused, and it is expanded, collapses the current row.
if (
activeRow.getAttribute( 'data-expanded' ) ===
'true' ||
activeRow.getAttribute( 'aria-expanded' ) === 'true'
) {
onCollapseRow( activeRow );
Expand Down Expand Up @@ -151,8 +154,10 @@ function UnforwardedTreeGrid(
// Right:
// If a row is focused, and it is collapsed, expands the current row.
if (
activeRow.getAttribute( 'data-expanded' ) ===
'false' ||
activeRow.getAttribute( 'aria-expanded' ) ===
'false'
'false'
) {
onExpandRow( activeRow );
event.preventDefault();
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/tree-grid/row.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ function UnforwardedTreeGridRow(
positionInSet,
setSize,
isExpanded,
disableAriaExpanded,
...props
}: WordPressComponentProps< TreeGridRowProps, 'tr', false >,
ref: React.ForwardedRef< HTMLTableRowElement >
Expand All @@ -28,7 +29,7 @@ function UnforwardedTreeGridRow(
aria-level={ level }
aria-posinset={ positionInSet }
aria-setsize={ setSize }
aria-expanded={ isExpanded }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the best area to explain this. The reason why this needs to exist is as follows.

  1. If aria-expanded exists on the tr and the link, it is announced twice when it is toggled.
  2. If it only exists on the row, it is announced when toggled but not after that.
  3. If it exists on the link only, it works fine when toggled and after.
  4. Had to figure out how to eliminate it off the tr.
  5. The better way about this may be to add data-expanded directly here and pass the value from the ListView. Something like this.
aria-expanded={ disableAriaExpanded ? undefined : isExpanded }
data-expanded={ disableAriaExpanded ? isExpanded : undefined }

One of these attributes is required for the keyboard functionality. Not pretty, but it works.

aria-expanded={ disableAriaExpanded ? undefined : isExpanded }
>
{ children }
</tr>
Expand Down
4 changes: 4 additions & 0 deletions packages/components/src/tree-grid/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ export type TreeGridRowProps = {
* it has no other built-in behavior.
*/
isExpanded?: boolean;
/**
* An optional value that designates whether a row should have aria-expanded attribute or if this should be managed in the parent component.
*/
disableAriaExpanded?: boolean;
};

type RovingTabIndexItemPassThruProps = {
Expand Down