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

Accessibility : Reader Improvements #10741

Merged
merged 24 commits into from
Nov 14, 2019

Conversation

jd-alexander
Copy link
Contributor

@jd-alexander jd-alexander commented Nov 6, 2019

Fixes #10734 #10731

Below are the improvements that were made on the reader post list.

Accessibility Scanner Audit

To Test

To verify this works all these improvements were made you can simply run the Accessibility Scanner on the screen and the issues below won't be shown. Another means of verification is simply running the app to see the changes are shown below.

  • Filter org.wordpress.android:id/filter_spinner - Touch target

    This item's height is 47dp. Consider making the height of this touch target 48dp or larger.

  • Follow Button org.wordpress.android:id/follow_button - Touch target

    This item's height is 30dp. Consider making the height of this touch target 48dp or larger.

  • Discover's Domain - org.wordpress.android:id/text_domain - Text contrast

    The item's text contrast ratio is 4.20. This ratio is based on an estimated foreground color of #787C82 and an estimated background color of #FFFFFF. Consider using a contrast ratio greater than 4.50 for small text, or 3.00 for large text.

  • Follow Count org.wordpress.android:id/text_blog_follow_count - Text contrast

    The item's text contrast ratio is 3.91. This ratio is based on an estimated foreground color of #787C82 and an estimated background color of #F6F7F7. Consider using a contrast ratio greater than 4.50 for small text, or 3.00 for large text.

  • Discover Text org.wordpress.android:id/text_discover - Text contrast

    The item's text contrast ratio is 3.16. This ratio is based on an estimated foreground color of #8E9196 and an estimated background color of #FFFFFF. Consider using a contrast ratio greater than 4.50 for small text, or 3.00 for large text.

  • Discover List Item's Visit Layout - org.wordpress.android:id/layout_discover - Touch target

    This item's height is 32dp. Consider making the height of this touch target 48dp or larger. A parent container may be handling touch events for this item. If selecting the larger container performs the same action as selecting this item, consider defining this item as not clickable. If a different action is performed, consider increasing the size of this item.

  • Visit Button org.wordpress.android:id/visit- Touch target

    This item's height is 36dp. Consider making the height of this touch target 48dp or larger. A parent container may be handling touch events for this item. If selecting the larger container performs the same action as selecting this item, consider defining this item as not clickable. If a different action is performed, consider increasing the size of this item.

  • More Button org.wordpress.android:id/image_more - Touch target

    This item's size is 36dp x 36dp. Consider making this touch target 48dp wide and 48dp high or larger. A parent container may be handling touch events for this item. If selecting the larger container performs the same action as selecting this item, consider defining this item as not clickable. If a different action is performed, consider increasing the size of this item.

Talkback Audit

To Test

Enable TalkBack and put the app in the states described below.

Screenshot_20191030-224532

  • The words "site icon" are being read out loud when the user however over the header of a post in the Reader view. The post title and other pertinent information can be made available but it's okay to disregard the author's profile image since their name is being announced.

  • Empty states should be read aloud. If enter has to be pressed that should also be expressed.

  • Saved posts :

Screenshot_20191101-201554

  • Empty state should be announced.

  • Tap to save post to your list. Icon shouldn't be read because the user wont know what it means anyway.

Reader Settings

Below are the improvements that were made on the reader settings.

Accessibility Scanner Audit

To Test

To verify this works all these improvements were made you can simply run the Accessibility Scanner on the screen and the issues below won't be shown. Another means of verification is simply running the app to see the changes are shown below.

settings

  • Remove Button org.wordpress.android:id/btn_remove - Touch target

    This item's size is 32dp x 32dp. Consider making this touch target 48dp wide and 48dp high or larger.

  • [] Add URL or Tag EditText org.wordpress.android:id/edit_add - Touch target

    This item's height is 38dp. Consider making the height of this touch target 48dp or larger.

  • Add Button org.wordpress.android:id/btn_add - Touch target

    This item's size is 32dp x 32dp. Consider making this touch target 48dp wide and 48dp high or larger.

  • Add URL or Tag EditText org.wordpress.android:id/edit_add - Text contrast

    The item's text contrast ratio is 4.20. This ratio is based on an estimated foreground color of #787C82 and an estimated background color of #FFFFFF. Consider using a contrast ratio greater than 4.50 for small text, or 3.00 for large text.

Contains tabs:

Followed Sites Tab

Talkback Audit

To Test

Enable TalkBack and put the app in the states described below.

  • Search

Screenshot_20191102-200643

  • Search input doesn't tell when no results have been returned. Return results after waiting for at least 4 - 5 seconds to ensure query was properly entered.

  • When search returns results the user won't know if results are actually present. The app should announce that two items actually exist. (It actually announces this the first time the user selects an item in the list.)

PR submission checklist:

  • I have considered adding unit tests where possible.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 6, 2019

You can test the changes on this Pull Request by downloading the APK here.

@jd-alexander jd-alexander marked this pull request as ready for review November 7, 2019 17:09
@jd-alexander jd-alexander changed the title Accessbility : Reader Post List Improvements Accessibility : Reader Post List Improvements Nov 7, 2019
@jd-alexander jd-alexander requested a review from shiki November 8, 2019 17:54
@shiki shiki requested a review from osullivanchris November 8, 2019 20:14
Copy link
Member

@shiki shiki left a comment

Choose a reason for hiding this comment

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

This is looking good, @jd-alexander. I have some questions.

I'd also like @osullivanchris's input on the design changes.

@osullivanchris
Copy link

@shiki thanks for the ping! Just a quick reply on the fly here to acknowledge - happy to go into details on Monday. But overall if we are making touch areas bigger invisibly thats fine. But if we need to physically make the pixels on screen bigger, I'm happy to jump in and review.

Not 100% sure on what all the visual changes proposed are. But for example - I can see that you've found the 'Visit Juls Kitchen' touch area is too small, and so is 'Visit site' below it. However, if I understand correctly, these two links actually bring the user to the same location. So we could make those two items into 1 touch target, solving the issue without needing to change the spacing/alignment. We could also make the 'more' icon to the right tapable in the entire height of the two items left of it. Crude sketch below.

Also please be aware that @develric and I are working on changing the reader navigation, bringing in a tab bar to switch between sections. So lets try not to conflict with each others changes.

Hope I have understood correctly!

IMG_4731

@jd-alexander
Copy link
Contributor Author

Thanks for this comment @osullivanchris

But overall if we are making touch areas bigger invisibly that's fine. But if we need to physically make the pixels on the screen bigger, I'm happy to jump in and review.

Okay, what I could do is simply approach it all from the programmatic standpoint. because modifying the spacing changes the layouts.

Not 100% sure on what all the visual changes proposed are. But for example - I can see that you've found the 'Visit Juls Kitchen' touch area is too small, and so is 'Visit site' below it. However, if I understand correctly, these two links actually bring the user to the same location. So we could make those two items into 1 touch target, solving the issue without needing to change the spacing/alignment. We could also make the 'more' icon to the right tappable in the entire height of the two items left of it. Crude sketch below.

Yes, the sketch helps and your explanation of grouping items helps. I will spend some more time on this using another approach that doesn't impact the UI and discuss in any more details on Monday when I do another review.

… one layout gets focus. Also the empty state is automatically announced when the view is shown.
The empty view subtitle for the saved posts / bookmarks is built from a span that includes an icon. When TalkBack reads this out it doesn't state what the icon is, so a content description is set and that is utilized instead of the actual text.
…nsistency

Changed the content description of the empty saved post message to
"Add to Save Posts button" so it's similar to what's being announced on
the post items.
@jd-alexander jd-alexander force-pushed the issue/10734-accessibility-reader-filters branch from 6b080de to f703dab Compare November 9, 2019 04:41
@jd-alexander
Copy link
Contributor Author

jd-alexander commented Nov 9, 2019

@osullivanchris I reverted the UI facing changes and now the touch targets are larger but the UI is still intact 🙌

I want your feedback on this particular change that needs to happen. So for the Followed Tags screen, in the Reader Settings section of the issue, the Touch Target of the Enter a URL or tag to follow EditText can’t be increased programmatically because there’s no space in its parent to make that happen. Directly above it is the List component and the touch area can only be expanded if the parent of the view has the space to accommodate this. So my question is, can the height of this view get the required 10dp extra that’s needed to meet the requirement? And you mentioned previously that there should be a 8dp spacing between the add button and the EditText so those were the two changes I was thinking of making. Let me know. Below is a how the change reflects, Let me know :)

edit_text

@jd-alexander jd-alexander changed the title Accessibility : Reader Post List Improvements Accessibility : Reader Improvements Nov 11, 2019
@jd-alexander jd-alexander added this to the 13.7 milestone Nov 12, 2019
@osullivanchris
Copy link

@jd-alexander thanks for the ping! Yeah that seems like a positive change to me! I think it will also look better and more tappable with the size increase so it seems like a win-win to me. The only thing I would ask is could you make sure that the text and icon are vertically aligned in the area? At the moment it looks like maybe you've just added the padding above - perhaps you could add 50% above and 50% below, if that makes sense?

just gonna ping @develric on this one too as we're making some other reader changes and so nothing breaks that he's working on. You might want to check in with him generally if you're implementing stuff on reader.

Thanks!

@jd-alexander jd-alexander requested a review from shiki November 12, 2019 19:33
shiki
shiki previously approved these changes Nov 12, 2019
Copy link
Member

@shiki shiki left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Just waiting for the resolution on @osullivanchris' comment in #10741 (comment).

@develric
Copy link
Contributor

develric commented Nov 12, 2019

Hi there 😊!

just gonna ping @develric on this one too as we're making some other reader changes and so nothing breaks that he's working on.

Thanks for the ping @osullivanchris. These modifications (and the general effort actually) seem a good improvement and if I understand correctly the rationale it should basically not "interfere" each other with the reader modifications we are doing (or we can work in that direction anyway 😊). Could be interesting to have an extra step on the work we are doing from the accessibility perspective. Don't think we can do it in this round sprint, but since we plan to release it behind a feature flag for now, we can agree on having this later before to final release (taking a note about this).

@shiki shiki self-requested a review November 12, 2019 21:16
@shiki shiki dismissed their stale review November 12, 2019 21:16

Made a mistake. Didn't see the new commits after the force-push.

@jd-alexander
Copy link
Contributor Author

Thanks for checking in @develric and yes, it would be good for some accessibility checks to be made on the improvements :)

@osullivanchris I implemented the changes and pushed them and this is a screenshot from the emulator with the new behavior and look. The UI is now centered correctly.

Screenshot_1573595364

Copy link
Member

@shiki shiki 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, everyone!

@shiki shiki merged commit 3f99349 into develop Nov 14, 2019
@shiki shiki deleted the issue/10734-accessibility-reader-filters branch November 14, 2019 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility: Reader Filters
5 participants