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

Add role=application to list view to prevent browse mode triggering in NVDA #44291

Merged
merged 4 commits into from
Sep 21, 2022
Merged

Add role=application to list view to prevent browse mode triggering in NVDA #44291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5ded249
Add role=application to list view to prevent browse mode triggering i…
talldan Sep 20, 2022
200953e
Move the aria-label to application role.
alexstine Sep 21, 2022
e64ffb9
Add aria-label back to table for now. This broke too many E2E and thi…
alexstine Sep 21, 2022
5932be5
Update snapshots
talldan Sep 21, 2022
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
1 change: 1 addition & 0 deletions packages/block-editor/src/components/list-view/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ function ListView(
onCollapseRow={ collapseRow }
onExpandRow={ expandRow }
onFocusRow={ focusRow }
applicationAriaLabel={ __( 'Block navigation structure' ) }
>
<ListViewContext.Provider value={ contextValue }>
<ListViewBranch
Expand Down
37 changes: 23 additions & 14 deletions packages/components/src/tree-grid/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,21 @@ function getRowFocusables( rowElement ) {
* Renders both a table and tbody element, used to create a tree hierarchy.
*
* @see https://github.com/WordPress/gutenberg/blob/HEAD/packages/components/src/tree-grid/README.md
* @param {Object} props Component props.
* @param {WPElement} props.children Children to be rendered.
* @param {Function} props.onExpandRow Callback to fire when row is expanded.
* @param {Function} props.onCollapseRow Callback to fire when row is collapsed.
* @param {Function} props.onFocusRow Callback to fire when moving focus to a different row.
* @param {Object} ref A ref to the underlying DOM table element.
* @param {Object} props Component props.
* @param {WPElement} props.children Children to be rendered.
* @param {Function} props.onExpandRow Callback to fire when row is expanded.
* @param {Function} props.onCollapseRow Callback to fire when row is collapsed.
* @param {Function} props.onFocusRow Callback to fire when moving focus to a different row.
* @param {string} props.applicationAriaLabel Label to use for the application role.
* @param {Object} ref A ref to the underlying DOM table element.
*/
function TreeGrid(
{
children,
onExpandRow = () => {},
onCollapseRow = () => {},
onFocusRow = () => {},
applicationAriaLabel,
...props
},
ref
Expand Down Expand Up @@ -286,14 +288,21 @@ function TreeGrid(
/* eslint-disable jsx-a11y/no-noninteractive-element-to-interactive-role */
return (
<RovingTabIndexContainer>
<table
{ ...props }
role="treegrid"
onKeyDown={ onKeyDown }
ref={ ref }
>
<tbody>{ children }</tbody>
</table>
{
// Prevent browser mode from triggering in NVDA by wrapping List View
// in a role=application wrapper.
// see: https://github.com/WordPress/gutenberg/issues/43729
}
<div role="application" aria-label={ applicationAriaLabel }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Having the label passed in to the application role is much nicer because it gives the application a label that is also discoverable via browse mode navigation.

@MarcoZehe Am I going to get in trouble with the ARIA spec by removing the aria-label from the treegrid role? Seems like in this situation since we've got a dedicated application here, it should be fine.

<table
{ ...props }
role="treegrid"
onKeyDown={ onKeyDown }
ref={ ref }
>
<tbody>{ children }</tbody>
</table>
</div>
</RovingTabIndexContainer>
);
/* eslint-enable jsx-a11y/no-noninteractive-element-to-interactive-role */
Expand Down
38 changes: 21 additions & 17 deletions packages/components/src/tree-grid/test/__snapshots__/cell.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,27 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`TreeGridCell uses a child render function to render children 1`] = `
<table
onKeyDown={[Function]}
role="treegrid"
<div
role="application"
>
<tbody>
<tr>
<td
role="gridcell"
>
<button
className="my-button"
onFocus={[Function]}
<table
onKeyDown={[Function]}
role="treegrid"
>
<tbody>
<tr>
<td
role="gridcell"
>
Click Me!
</button>
</td>
</tr>
</tbody>
</table>
<button
className="my-button"
onFocus={[Function]}
>
Click Me!
</button>
</td>
</tr>
</tbody>
</table>
</div>
`;
2 changes: 1 addition & 1 deletion packages/components/src/tree-grid/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`TreeGrid simple rendering renders a table, tbody and any child elements 1`] = `"<table role=\\"treegrid\\"><tbody><tr role=\\"row\\" aria-level=\\"1\\" aria-posinset=\\"1\\" aria-setsize=\\"1\\"><td role=\\"gridcell\\">Test</td></tr></tbody></table>"`;
exports[`TreeGrid simple rendering renders a table, tbody and any child elements 1`] = `"<div role=\\"application\\"><table role=\\"treegrid\\"><tbody><tr role=\\"row\\" aria-level=\\"1\\" aria-posinset=\\"1\\" aria-setsize=\\"1\\"><td role=\\"gridcell\\">Test</td></tr></tbody></table></div>"`;