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.

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

[Mobile]Paragraph - Add accessibility label for unselected state #15126

Merged
merged 2 commits into from
Apr 24, 2019

Conversation

pinarol
Copy link
Contributor

@pinarol pinarol commented Apr 23, 2019

Description

This PR improves accessibility on image block selected state.
Fixes part of wordpress-mobile/gutenberg-mobile#907

To test:
Please refer to the gutenberg-mobile side PR.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@pinarol pinarol added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Apr 23, 2019
@pinarol pinarol requested a review from etoledom April 23, 2019 15:55
@pinarol pinarol self-assigned this Apr 23, 2019
@pinarol pinarol changed the title Add accessibility label for unselected state [Mobile]Add accessibility label for unselected state Apr 23, 2019
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this @pinarol !

I was able to reproduce the unexpected behavior of screen reader speaking the unselected label after being selected via double tap.

I added a couple of comments with some ideas on how to address that issue.
Hopefully it won't be necessary to declare the internal onFocus and onBlur methods.

<View>
<View
accessible={ ! this.state.isSelected }
accessibilityLabel={ __( 'Paragraph block' ) + __( '.' ) + ' ' + ( isEmpty( content ) ? __( 'Empty' ) : content ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we could use onAccessibilityTap={ this.props.onFocus } to avoid defining the internal onFocus method.
The parent component (BlockHolder) should be in charge of setting the selection state of the block in the store. And we can use that state from the store internally via props.

Copy link
Contributor

Choose a reason for hiding this comment

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

One more detail: content has all the html code, so the ScreenReader will read the html tags too.
We can clean it up using: import { create } from '@wordpress/rich-text';

@@ -95,13 +116,16 @@ class ParagraphEdit extends Component {
} = attributes;

return (
<View>
<View
accessible={ ! this.state.isSelected }
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we should probably use isSelected from props. In that way it will be properly set to false when the block is selected by any method (not just from focusing RichText directly), and ScreenReader won't read this label after double tap to select.

@pinarol
Copy link
Contributor Author

pinarol commented Apr 24, 2019

Thank you! Ready to give another look @etoledom 🎉

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Thank you for the updates. It's working beautifully 🎉 !

Sadly on Android we can't test it properly until wordpress-mobile/gutenberg-mobile#918 is fixed. So let's :shipit: for now and continue.

Nice job!

@pinarol pinarol merged commit 1b7c347 into master Apr 24, 2019
@pinarol pinarol deleted the rnmobile/paragraph-block-accessibility branch April 24, 2019 10:38
@pinarol pinarol changed the title [Mobile]Add accessibility label for unselected state [Mobile]Paragraph - Add accessibility label for unselected state Apr 24, 2019
hypest pushed a commit that referenced this pull request May 2, 2019
* Add accessibility label for unselected state

* Improve accessibility handling
@youknowriad youknowriad added this to the 5.6 (Gutenberg) milestone May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants