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

UIAccessibility usability improvements for VoiceOver #535

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mathewa6
Copy link

@mathewa6 mathewa6 commented May 15, 2018

Thanks for the library! I came across it on Twitter after it was listed in the top downloaded pods.

Since most of this project is based on UIKit elements, it is accessible. However, there are some usability issues that get in the way of a VoiceOver (VO) user being aware of what is happening on screen.

  1. When the HUD is shown, there is no indication given to a VO user unless they swipe.
  2. When they do swipe, the list of elements from the HUD spill onto elements from underneath the HUG.
  3. Occasionally, the dismissal of the HUD refocuses VO to the top of the superview, which isn't ideal.

This PR attempts to address these issues in a non intrusive manner. All tests passing on my end.
These changes do the following:

  • Focus on the HUD when presented and return focus to the last active element after hiding.
  • Prevent VO from scrolling onto elements behind the HUD while it is in progress/animating.
  • Provides a percentage readout to VO for the determinate HUDs.

Happy to make changes as needed.

TLDR: Heres a 20s video(no audio): Just pay attention to the VO focus box.

@matej
Copy link
Collaborator

matej commented May 16, 2018

👏 Thank you for taking interest in this. I'm putting this on my TODO list for testing. Thanks!

@ay8s
Copy link

ay8s commented Sep 24, 2018

This is definitely an improvement. Awesome work @mathewa6! 👍

I do see the focus switch between the HUD and the previously select element midway through progress. https://cl.ly/32ba0000d2e0 Would also be great to read the label if the indicator has one.

@mathewa6
Copy link
Author

Thanks. I had to find my notes from when I worked on this to figure out why I hadn't read the label automatically. It seems like I couldn't simply set the accessibilityLabel on the UIActivityIndicatorView to use the self.label text for some reason. (Maybe I'm missing something?).

The other option would be to set the entire MBProgressHUD to be its own accessibilityElement whose label and traits are changed depending on the internal classes used, which I thought is best saved for a bigger PR.

@matej
Copy link
Collaborator

matej commented Nov 4, 2018

Took a closer look at this. While I believe that making the whole indicator a single accessibility element would be a nicer solution, I still think this is a step in the right direction and improves the behavior over the current one. One problem is that "Progress" isn't really localized. Taking the label value, when set, would make that better, as I would assume the host app would already make sure the label text is localized.

MBProgressHUD.m Outdated Show resolved Hide resolved
MBProgressHUD.m Outdated Show resolved Hide resolved
MBProgressHUD.m Outdated Show resolved Hide resolved
@mathewa6
Copy link
Author

mathewa6 commented Nov 5, 2018

Thanks @matej !

I've made 2 additional commits, the first of which directly addresses the feedback you've posted.

The second uses KVO on the label's text property and then uses that as accessibilityLabel on the indicator. I've hence removed label as an accessibilityLabel except for text only and custom modes to prevent redundant VoiceOver speech. Do note that this does result in an additional call to -updateIndicators: one at init and one if self.label.text is set. Not sure if this is the best way to go about it, but happy to make changes as it's the closest to just making the entire HUD a single element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants