Skip to content

[Mobile]: Improve screen reader support on BottomSheet's cells.#15213

Merged
etoledom merged 11 commits intomasterfrom
rnmobile/bottom-sheet-screen-reader
May 16, 2019
Merged

[Mobile]: Improve screen reader support on BottomSheet's cells.#15213
etoledom merged 11 commits intomasterfrom
rnmobile/bottom-sheet-screen-reader

Conversation

@etoledom
Copy link
Copy Markdown
Contributor

@etoledom etoledom commented Apr 26, 2019

Ref: wordpress-mobile/gutenberg-mobile#935

This PR focuses on the cells of the bottom sheets, providing better labeling, hints and action handling.

  • The Cells with no value has their label as default accessibility label.
  • For Cells with value, is needed to provide a properly localized accessibility label.
  • For SwitchCells, the proper localized accessibility label is needed too, indicating the switch state.

For both the Cells with value and SwtchCells, the manual handling of the labels is not optimal, but required if we want to properly localize the whole sentence.

I believe that for these cases we can make an exception, and generically concatenate label and value as the accessibility label for cells with value.

Same for switches, we can generically concatenate the localized switch state with _x(), indicating the context of the On, Off strings.

This will save us from possible mistakes in the future, and will help with an easier accessibility maintenance.

I left this PR implementing the first case to show the point. But I'd happily edit it to automatically handle accessibility labels in BottomSheet cells. See the changes on image/edit.native.js and link/modal.native.js.

EDIT: Made changes to handle accessibility labels internally.

To test:

  • With VoiceOver ON, select a paragraph block.
  • Activate the link button in the toolbar.
  • Check that the labels are read correctly, with Empty when correspond.
  • Check that the cell with switch is toggled with double tap.
  • Select an Image block with an image.
    • Recommended to deactivate VoiceOver to exit the BottomSheet :(
  • Activate the Image Settings button from the inline tool bar.
  • Check that the labels are read correctly, with Empty when correspond.

@etoledom etoledom added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Apr 26, 2019
@etoledom etoledom requested review from Tug and pinarol April 26, 2019 14:31
@etoledom etoledom self-assigned this Apr 26, 2019
@pinarol
Copy link
Copy Markdown
Contributor

pinarol commented May 3, 2019

For both the Cells with value and SwtchCells, the manual handling of the labels is not optimal, but required if we want to properly localize the whole sentence.

I think in this case we can really benefit from _x( "%1$s. %2$s", "<context>" ) so if we give the proper context and use the same context for all accessibility related things we won't be generating much trouble for translators. I know it looks weird but I remember this as the recommended way compared to __('.').

I believe that for these cases we can make an exception, and generically concatenate label and value as the accessibility label for cells with value.

Same for switches, we can generically concatenate the localized switch state with _x(), indicating the context of the On, Off strings.

This will save us from possible mistakes in the future, and will help with an easier accessibility maintenance.

I agree, I think this case deserves a more generic handling. I'd say let's just concatenate label and value at base level otherwise it will generate code duplication and will be more bug prone.

@etoledom
Copy link
Copy Markdown
Contributor Author

@pinarol - Thanks for the review!
I made the changes necessary to handle the accessibility labels internally by the cells.

Please take a look anytime you get the chance :)

Copy link
Copy Markdown
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

Looks and works great! 🎉
Tested with WPAndroid and WPiOS apps on real devices

@etoledom etoledom merged commit baef1c2 into master May 16, 2019
@etoledom
Copy link
Copy Markdown
Contributor Author

Thank you!

@etoledom etoledom deleted the rnmobile/bottom-sheet-screen-reader branch May 16, 2019 14:13
@gziolo gziolo added this to the 5.8 (Gutenberg) milestone May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). 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