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

Improve the placeholder instructions accessibility. #45801

Merged
merged 17 commits into from
Sep 21, 2023
Merged

Improve the placeholder instructions accessibility. #45801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0aa9782
Improve the placeholder instructions accessibility.
afercia Nov 15, 2022
9bba5f0
Use a speak message for the placeholder instructions.
afercia Nov 17, 2022
ed69991
Update test.
afercia Nov 17, 2022
f818056
Try to only announce message after initial focus.
alexstine Jan 7, 2023
d3faeb9
Fix unit tests.
alexstine Jan 7, 2023
3c8d17b
Announce message on render and add test.
afercia Feb 22, 2023
ccb24c3
Refresh.
alexstine Sep 19, 2023
887b619
Changelog entry.
alexstine Sep 19, 2023
00fdbbf
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/place…
alexstine Sep 19, 2023
22c9153
Reviewer feedback.
alexstine Sep 19, 2023
d4c5265
Fix unit tests.
alexstine Sep 19, 2023
eea9100
Merge branch 'fix/placeholder-instructions-accessibility' of github.c…
alexstine Sep 19, 2023
4fdf14e
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/place…
alexstine Sep 20, 2023
e4a0f6a
Fix changelog.
alexstine Sep 20, 2023
37bda2d
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/place…
alexstine Sep 20, 2023
9bf6cf5
Merge branch 'trunk' into fix/placeholder-instructions-accessibility
ciampo Sep 21, 2023
87923da
Remove duplicate CHANGELOG entry
ciampo Sep 21, 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
6 changes: 3 additions & 3 deletions packages/block-library/src/cover/test/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ describe( 'Cover block', () => {
await setup();

expect(
screen.getByRole( 'group', {
name: 'To edit this block, you need permission to upload media.',
} )
within( screen.getByLabelText( 'Block: Cover' ) ).getByText(
'To edit this block, you need permission to upload media.'
)
).toBeInTheDocument();
} );

Expand Down
3 changes: 2 additions & 1 deletion packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@

## Unreleased

### Bug fix
### Bug Fix

- `Placeholder`: Improved DOM structure and screen reader announcements ([#45801](https://github.com/WordPress/gutenberg/pull/45801)).
- `DateTimePicker`: fix onChange callback check so that it also works inside iframes ([#54669](https://github.com/WordPress/gutenberg/pull/54669)).

## 25.8.0 (2023-09-20)
Expand Down
23 changes: 15 additions & 8 deletions packages/components/src/placeholder/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import classnames from 'classnames';
*/
import { useResizeObserver } from '@wordpress/compose';
import { SVG, Path } from '@wordpress/primitives';
import { useEffect } from '@wordpress/element';
import { speak } from '@wordpress/a11y';

/**
* Internal dependencies
Expand Down Expand Up @@ -72,10 +74,17 @@ export function Placeholder(
modifierClassNames,
withIllustration ? 'has-illustration' : null
);

const fieldsetClasses = classnames( 'components-placeholder__fieldset', {
'is-column-layout': isColumnLayout,
} );

useEffect( () => {
if ( instructions ) {
speak( instructions );
}
}, [ instructions ] );
Copy link
Member

Choose a reason for hiding this comment

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

On page load, all block with placeholders with instructions are now all "speaking" at once. Is this by intention?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ellatrix No, it needs to be fixed.


return (
<div { ...additionalProps } className={ classes }>
{ withIllustration ? PlaceholderIllustration : null }
Expand All @@ -90,14 +99,12 @@ export function Placeholder(
<Icon icon={ icon } />
{ label }
</div>
<fieldset className={ fieldsetClasses }>
{ !! instructions && (
<legend className="components-placeholder__instructions">
{ instructions }
</legend>
) }
{ children }
</fieldset>
{ !! instructions && (
<div className="components-placeholder__instructions">
{ instructions }
</div>
) }
<div className={ fieldsetClasses }>{ children }</div>
</div>
);
}
Expand Down
12 changes: 0 additions & 12 deletions packages/components/src/placeholder/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,18 +72,6 @@
}
}

// Overrides for browser and editor fieldset styles.
.components-placeholder__fieldset.components-placeholder__fieldset {
border: none;
padding: 0;

.components-placeholder__instructions {
padding: 0;
font-weight: normal;
font-size: 1em;
}
}

.components-placeholder__fieldset.is-column-layout,
.components-placeholder__fieldset.is-column-layout form {
flex-direction: column;
Expand Down
49 changes: 34 additions & 15 deletions packages/components/src/placeholder/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { render, screen, within } from '@testing-library/react';
*/
import { useResizeObserver } from '@wordpress/compose';
import { SVG, Path } from '@wordpress/primitives';
import { speak } from '@wordpress/a11y';

/**
* Internal dependencies
Expand Down Expand Up @@ -41,17 +42,21 @@ const Placeholder = (

const getPlaceholder = () => screen.getByTestId( 'placeholder' );

jest.mock( '@wordpress/a11y', () => ( { speak: jest.fn() } ) );
const mockedSpeak = jest.mocked( speak );

describe( 'Placeholder', () => {
beforeEach( () => {
// @ts-ignore
useResizeObserver.mockReturnValue( [
<div key="1" />,
{ width: 320 },
] );
mockedSpeak.mockReset();
} );

describe( 'basic rendering', () => {
it( 'should by default render label section and fieldset.', () => {
it( 'should by default render label section and content section.', () => {
render( <Placeholder /> );
const placeholder = getPlaceholder();

Expand All @@ -74,9 +79,12 @@ describe( 'Placeholder', () => {
);
expect( placeholderInstructions ).not.toBeInTheDocument();

// Test for empty fieldset.
const placeholderFieldset =
within( placeholder ).getByRole( 'group' );
// Test for empty content. When the content is empty,
// the only way to query the div is with `querySelector`
// eslint-disable-next-line testing-library/no-node-access
Copy link
Contributor

@alexstine alexstine Jan 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a better way or is it okay to ignore?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mhh, there isn't an obvious better choice. Maybe @tyxla can think of something here?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the ping @ciampo!

Nope, if we remove the fieldset in favor of a div (which I agree it makes sense), there's no other way to query for the empty div. There are other DOM traversing alternatives, but this one is the most precise.

Therefore, since I agree with removing the fieldset in favor of a div in this use case, I'm fine with the ignore. Making an element inaccessible intentionally is the ideal case for this type of ESLint ignore.

const placeholderFieldset = placeholder.querySelector(
'.components-placeholder__fieldset'
);
expect( placeholderFieldset ).toBeInTheDocument();
expect( placeholderFieldset ).toBeEmptyDOMElement();
} );
Expand Down Expand Up @@ -104,27 +112,38 @@ describe( 'Placeholder', () => {
expect( placeholderLabel ).toBeInTheDocument();
} );

it( 'should display a fieldset from the children property', () => {
const content = 'Fieldset';
it( 'should display content from the children property', () => {
const content = 'Placeholder content';
render( <Placeholder>{ content }</Placeholder> );
const placeholderFieldset = screen.getByRole( 'group' );
const placeholder = screen.getByText( content );

expect( placeholderFieldset ).toBeInTheDocument();
expect( placeholderFieldset ).toHaveTextContent( content );
expect( placeholder ).toBeInTheDocument();
expect( placeholder ).toHaveTextContent( content );
} );

it( 'should display a legend if instructions are passed', () => {
it( 'should display instructions when provided', () => {
const instructions = 'Choose an option.';
render(
<Placeholder instructions={ instructions }>
<div>Fieldset</div>
<div>Placeholder content</div>
</Placeholder>
);
const placeholder = getPlaceholder();
const instructionsContainer =
within( placeholder ).getByText( instructions );

expect( instructionsContainer ).toBeInTheDocument();
} );

it( 'should announce instructions to screen readers', () => {
const instructions = 'Awesome block placeholder instructions.';
render(
<Placeholder instructions={ instructions }>
<div>Placeholder content</div>
</Placeholder>
);
const captionedFieldset = screen.getByRole( 'group', {
name: instructions,
} );

expect( captionedFieldset ).toBeInTheDocument();
expect( speak ).toHaveBeenCalledWith( instructions );
} );

it( 'should add an additional className to the top container', () => {
Expand Down
Loading