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

Improve accessibility of video block select poster image. #14752

Merged
merged 1 commit into from
Apr 11, 2019
Merged

Improve accessibility of video block select poster image. #14752

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 32 additions & 4 deletions packages/block-library/src/video/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,14 @@ import {
} from '@wordpress/block-editor';
import { mediaUpload } from '@wordpress/editor';
import { Component, Fragment, createRef } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import {
__,
sprintf,
} from '@wordpress/i18n';
import {
compose,
withInstanceId,
} from '@wordpress/compose';

/**
* Internal dependencies
Expand Down Expand Up @@ -126,12 +133,19 @@ class VideoEdit extends Component {
controls,
loop,
muted,
playsInline,
poster,
preload,
src,
playsInline,
} = this.props.attributes;
const { setAttributes, isSelected, className, noticeOperations, noticeUI } = this.props;
const {
className,
instanceId,
isSelected,
noticeOperations,
noticeUI,
setAttributes,
} = this.props;
const { editing } = this.state;
const switchToEditing = () => {
this.setState( { editing: true } );
Expand Down Expand Up @@ -165,6 +179,7 @@ class VideoEdit extends Component {
/>
);
}
const videoPosterDescription = `video-block__poster-image-description-${ instanceId }`;

/* eslint-disable jsx-a11y/no-static-element-interactions, jsx-a11y/onclick-has-role, jsx-a11y/click-events-have-key-events */
return (
Expand Down Expand Up @@ -232,11 +247,21 @@ class VideoEdit extends Component {
isDefault
onClick={ open }
ref={ this.posterImageButton }
aria-describedby={ videoPosterDescription }
>
{ ! this.props.attributes.poster ? __( 'Select Poster Image' ) : __( 'Replace image' ) }
</Button>
) }
/>
<p
id={ videoPosterDescription }
hidden
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of using hidden prop here? In other places, we use screen-reader-text class name if the message exists only for screen readers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @gziolo, nice question!
Like you said we use screen-reader-text class name if the message exists only for screen readers. When we use screen-reader-text the screen readers still announce the text of the element. In this case, we also want to hide this element from screen readers. This element should not appear on the a11y tree by itself. It is only used when the other element that references it is focused and in that case the description is announced as part of the other element because it references it with aria-describedby.

Copy link
Member

Choose a reason for hiding this comment

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

I didn’t articulate it good enough. This special class hides an element completely unless it receives focus. In this case this class would be applied to p element which never gets focused through tabbing.

Copy link
Member

Choose a reason for hiding this comment

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

@joedolson, could you advise what is the best approach? I saw your name in related docs 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

Just trying to figure out exactly what this change is doing. It seems like it's adding a description of the image that's currently selected as the poster image - is that correct?

If so, this should be fine. If a hidden element is the target of an aria-describedby or aria-labelledby attribute, that text should be read out by screen readers when they focus on the element that carries the labelling attribute. (In short: being hidden doesn't matter to aria-describedby).

Do I correctly understand what's being done here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it’s correct. Thanks for a quick answer 👍

>
{ this.props.attributes.poster ?
sprintf( __( 'The current poster image url is %s' ), this.props.attributes.poster ) :
__( 'There is no poster image currently selected' )
}
</p>
{ !! this.props.attributes.poster &&
<Button onClick={ this.onRemovePoster } isLink isDestructive>
{ __( 'Remove Poster Image' ) }
Expand Down Expand Up @@ -275,4 +300,7 @@ class VideoEdit extends Component {
}
}

export default withNotices( VideoEdit );
export default compose( [
withNotices,
withInstanceId,
] )( VideoEdit );