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]: Improving accessibility on Image Block (deselected) #14713

Merged
merged 14 commits into from
Apr 23, 2019

Conversation

etoledom
Copy link
Contributor

This PR improves accessibility on the Image Block.

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

@etoledom etoledom 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 Mar 29, 2019
@etoledom etoledom self-assigned this Mar 29, 2019
@etoledom etoledom requested a review from Tug March 29, 2019 18:31
@etoledom etoledom changed the title Improving accessibility on Image Block (deselected) [Mobile]: Improving accessibility on Image Block (deselected) Mar 29, 2019
accessible={ ! isSelected }
accessibilityLabel={ __( 'Image block' ) + '.' + alt }
accessibilityRole={ 'button' }
accessibilityHint={ __( 'Doulbe tap to edit the image' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Doulbe => Double

@@ -16,7 +16,12 @@ import styles from './styles.scss';

function MediaPlaceholder( props ) {
return (
<TouchableWithoutFeedback onPress={ props.onMediaOptionsPressed }>
<TouchableWithoutFeedback
accessibilityLabel={ __( 'Image block. Empty ' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can reuse the 'Image block' string here. Image block. Empty as a whole does not make much sense for translators:

accessibilityLabel={ __( 'Image block' ) + '. ' + __( 'Empty' ) }

<TouchableWithoutFeedback onPress={ this.onImagePressed } disabled={ ! isSelected }>
<TouchableWithoutFeedback
accessible={ ! isSelected }
accessibilityLabel={ __( 'Image block' ) + '.' + alt }
Copy link
Contributor

Choose a reason for hiding this comment

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

It could use a space after the period:

__( 'Image block' ) + '. ' + alt

@etoledom
Copy link
Contributor Author

etoledom commented Apr 3, 2019

Thank you for the review @Tug !
Added the requested changes. Nice catches :)

@etoledom etoledom requested a review from Tug April 3, 2019 07:38
@etoledom etoledom requested review from SergioEstevao and hypest and removed request for SergioEstevao April 8, 2019 10:04
For some reason, VoiceOver wouldn't scroll down the block list.
The breaking change was located at the removal of this css property `font-family: $default-regular-font;` on the nextpage block.

Addinb back that property to be able to scroll the blocks list again using VoiceOver.
// that React Native wasn't able to handle (adding a fallback generic font family).
const textStyle = {
...styles[ 'block-library-nextpage__text' ],
fontFamily: 'System',
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 was creating an issue that disabled scrolling with VoiceOver active.
I'm not sure the specific reason, but rolling back this change fixed the issue.

@etoledom etoledom requested review from pinarol and removed request for hypest April 22, 2019 07:51
@etoledom
Copy link
Contributor Author

I cherry-picked a commit to fix an issue scrolling with VoiceOver active: 14652d6.

That commit also brought ScreenReader improvements for the NextPage block.

@Tug this is ready for another check. Adding @pinarol as reviewer too.

<TouchableWithoutFeedback onPress={ this.onImagePressed } disabled={ ! isSelected }>
<TouchableWithoutFeedback
accessible={ ! isSelected }
accessibilityLabel={ __( 'Image block' ) + '. ' + alt }
Copy link
Contributor

Choose a reason for hiding this comment

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

the dot here can be localized also

hypest pushed a commit that referenced this pull request May 2, 2019
* Adding accessibility properties to MediaPlaceholder and ImageEdit

* Fix lint issues

* Fix typo

* Better accessibility labels

* Disable lists. (#14844)

* Fix scroll with VoiceOver ON.

For some reason, VoiceOver wouldn't scroll down the block list.
The breaking change was located at the removal of this css property `font-family: $default-regular-font;` on the nextpage block.

Addinb back that property to be able to scroll the blocks list again using VoiceOver.

* Setting accessibilityStates properly to avoid Android crash

* Remove vscode settings file (added by mistake)

* Fix lint issues

* Update accessibility labels to use sprintf

* Fix lint issues
@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.

5 participants