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

[RNMobile] Improve block inserter accessibility #25549

Conversation

jd-alexander
Copy link
Contributor

@jd-alexander jd-alexander commented Sep 22, 2020

Fixes wordpress-mobile/gutenberg-mobile#915

Solution

The MenuItem that gets rendered for each block within the Inserter was given the role of button and it's accessibility label was appended with block to make it clearer that the options in the menu are to add blocks.

The open event of the Inserter's bottom sheet now announces it's state so the user knows that there's a view that is overlaying the rest of the content. The copy is as follows.

  • When the sheet opens, the app announces Scrollable block menu opened. Select a block.
  • When the sheet closes, the app announces Scrollable block menu closed.

Concerns

The only concern I have with this functionality is how I had to implement it on iOS. Firstly, I had to put a delay so that the announcement wasn't triggered during the screen changes else the announcement wouldn't be made at all. Also since an announcement is being made it's interrupting VoiceOver's behavior when it's attempting to refocus on the first element on the screen.

I was thinking that I could add an event listener to the custom announcements created and then tell VoiceOver to focus on those elements once the announcements are done. I wanted to try this but I wasn't sure how to get a reference to the first View on the screen.

Testing

  1. Enable either VoiceOver on iOS or TalkBack on Android.
  2. Open and close the bottom sheet to hear the announcement.
  3. Open the bottom sheet and hover over different blocks to hear the "block, Button" addition to each option. Before, only the name of the block would have been announced.

Screenshots

Android

Click to see screenshots

iOS

Click to see screenshots



Reviewing

Only 1 reviewer is needed but anyone can review.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@jd-alexander
Copy link
Contributor Author

@etoledom could you please add yourself as a reviewer to this PR. Thank you 🙇

@etoledom etoledom self-requested a review September 23, 2020 19:35
@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 Sep 23, 2020
@jd-alexander
Copy link
Contributor Author

I'm not sure why these tests are failing. How can these be re-run?

@jd-alexander jd-alexander marked this pull request as ready for review September 23, 2020 19:37
@jd-alexander
Copy link
Contributor Author

After doing some more research within the codebase and elsewhere I realized there's an example of using a ref to set the accessibility focus. This was done in the Add Mobile Spacer component PR. So my idea is to track which block is currently focused using a custom hook.
Once the custom announcement is done, then the previously focused area will be refocused. This solution seems like a lot of work to me just to get this behavior working. Sharing nonetheless, in case this can lead to other ideas.

@guarani
Copy link
Contributor

guarani commented Sep 24, 2020

I'm not sure why these tests are failing. How can these be re-run?

It might be a permissions thing, do you see this re-run button after clicking on details of the failing test?

Screen Shot 2020-09-24 at 11 05 27

@jd-alexander
Copy link
Contributor Author

jd-alexander commented Sep 24, 2020

Thanks for checking @guarani I am not able to see the re-run button so I think because I haven't merged in three PRs as yet I am not a contributor so I probably don't have that permission. When the three PRs I have opened are merged I should be good. Could you trigger another re-run for me? It might be that these tests are flaky.

@guarani
Copy link
Contributor

guarani commented Sep 24, 2020

@jd-alexander I re-triggered them now.

I am not able to see the re-run button so I think because I haven't merged in three PRs as yet I am not a contributor so I probably don't have that permission.

Gotcha 👌

@jd-alexander
Copy link
Contributor Author

@jd-alexander I re-triggered them now.

I am not able to see the re-run button so I think because I haven't merged in three PRs as yet I am not a contributor so I probably don't have that permission.

Gotcha 👌

My changes probably broke some tests so I'm gonna do an investigation.

@jd-alexander jd-alexander marked this pull request as draft September 29, 2020 07:47
@jd-alexander
Copy link
Contributor Author

I will be fixing these conflicts soon to get this in the review queue 🙏

@jd-alexander jd-alexander marked this pull request as ready for review October 6, 2020 12:55
@jd-alexander
Copy link
Contributor Author

@etoledom the CI builds have been giving a lot of issues today hence the failure in the builds. You can still give this a try and let me know your thoughts on it. Thank you.

@jd-alexander
Copy link
Contributor Author

So after doing some debugging, I am getting a weird failure in my environment. When I run the gutenberg-editor-gallery.test locally on master or this branch it fails to find the block when this line is run in the test

const galleryBlock = await editorPage.getBlockAtPosition(

When I dug into the getBlockAtPosition() function I realized that the failure was occurring here

const elements = await this.driver.elementsByXPath( blockLocator );

Looking at the function parameters there is a default position of 1 and when I looked at my emulator I saw that the Gallery block was definitely not in the first position as the default editor was being shown like so

So I am wondering if the way how my test is being run locally is incorrect in some way. I just looked at another PR and saw the video of the test result and realized an empty editor is being used when the test was run.

@jd-alexander
Copy link
Contributor Author

jd-alexander commented Oct 9, 2020

Yay! so the test was indeed fixed with my change 🥳 I am going to fix the associated PR now that I am figuring out the issue with my local environment which was caused due to an incorrect command.

@jd-alexander
Copy link
Contributor Author

All is well here @etoledom for another review. This and the gb-mobile PR are passing as all test related issues have been resolved. Thanks much for all the help and guidance thus far 👍

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.

Tested on iOS and Android and it's working great!
Thank you for this improvement @jd-alexander 🙏

@jd-alexander
Copy link
Contributor Author

Note: I am merging this PR even though there are failed tests as these tests aren't the result of changes made in this PR.

@jd-alexander jd-alexander merged commit c7a0b7c into WordPress:master Oct 14, 2020
@jd-alexander jd-alexander deleted the rnmobile/improve_block_inserter_accessibility branch October 14, 2020 16:27
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Oct 14, 2020
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.

Improve Screen Reader support for Block Picker.
4 participants