Accessibility : Notification Improvements by jd-alexander · Pull Request #10799 · wordpress-mobile/WordPress-Android

jd-alexander · 2019-11-14T17:41:23Z

Some of the issues described in the above issue were ignored due to further analysis that deemed them not being necessary for resolution. Most notably, some of the accessibility prompts for the Accessibility Scanner were for items that don't need to have to have larger touch targets as they were easy enough to select for announcement and they wouldn't be clickable. eg. some of the header text.

Main

TalkBack Audit

The icon(s) in the list item shouldn't be read since the are profile pictures and the notification content has the name of the person.

Settings

Accessibility Scanner Audit

Main Area - Your Sites- org.wordpress.android:id/header_text - Text contrast

The item's text contrast ratio is 1.98. This ratio is based on an estimated foreground color of #B2B2B2 and an estimated background color of #F6F7F7. Consider increasing this item's text contrast ratio to 3.00 or greater.

Followed Sites - Notification Dialog -

New posts - Description - Text contrast
org.wordpress.android:id/notifications_new_posts_description

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.

Your Sites - Notification Screen

* Notification Setting Pop up Dialog - org.wordpress.android:id/row_containerTouch target

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

Notification Search

Before	After

Search Text - org.wordpress.android:id/search_src_text - Text contrast

The item's text contrast ratio is 2.95. This ratio is based on an estimated foreground color of #80B0C4 and an estimated background color of #6088. Consider increasing this item's text contrast ratio to 3.00 or greater.

TalkBack Audit

Notifications On Switch

The message being read wasn't descriptive. It read "On, On Switch". It should read "Notifications, On Switch"

Before	After

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.

…t important for accessibility.

Also increased touch target row container that's within switch control used for notification settings.

peril-wordpress-mobile · 2019-11-14T17:47:14Z

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

Did this programmatically as styling it was being proven to be more difficult than expected.

The content description for the toolbar's title was being set before the toolbar's TextView was even populated so in essence, so child TextView was present. This logic was moved to the initialization function that sets the title on toolbar which creates the TextView that would further get the content description.

...rc/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.java

jkmassel · 2019-11-18T22:48:45Z

Howdy folks! I'm freezing 13.7 today, so this is being bumped to 13.8. If you'd like it to go out as part of 13.7, please target release/13.7 for this PR, and DM me once it's merged – I'll be happy to cut a new beta release!

shiki

Looking good, @jd-alexander. Sorry for not getting to this earlier. 😞

I really like your commit messages. And also, thank you for the arrows in the screenshots. They're really great!

I have a few comments. Please let me know what you think. 🙂

...rc/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.java

shiki · 2019-11-25T23:38:55Z

...rc/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.java

        mSearchMenuItem = menu.findItem(R.id.menu_notifications_settings_search);
        mSearchView = (SearchView) mSearchMenuItem.getActionView();
        mSearchView.setQueryHint(getString(R.string.search_sites));
+        setSearchViewHintColor();


I noticed that you changed this from your first implementation in 1989207. I think we should do it that way and make it a static too. Possibly shallow reasons:

A private void can indicate that this method can and should be used multiple times but it doesn't really need to be. An instance method also adds a bit of cognitive overhead to analyze how the state changes in this class.

A static void would imply that it's a utility method, which this one is

It's arguably better for readability when you're scanning the contents of onCreateOptionsMenu

Sample untested code:

private static void setSearchViewHintColor(@NonNull SearchView searchView, @ColorRes int colorResId) { final Context context = searchView.getContext(); if (context != null) { ((EditText) searchView.findViewById(androidx.appcompat.R.id.search_src_text)) .setHintTextColor(ContextCompat.getColor(context, colorResId)); } }

What do you think? 🙂

Thanks. Yes, your reasoning is sound and I was thinking of making it a utility method at first hence the first implementation. Since its a utility method wouldn't it make sense to put it somewhere else so if another person wants to utilize it, it's accessible? Let me know what you think about that.

I made these changes so once I get your final opinion on this I will push it.

Since its a utility method wouldn't it make sense to put it somewhere else so if another person wants to utilize it, it's accessible?

I'm okay with keeping it private here and make it public later when we violate the Rule of Three. But up to you!

Okay, thanks for citing that rule! Yes, it's best to make it public at that time 👍

Resolved here fe68ca6

...ss/src/main/java/org/wordpress/android/ui/prefs/notifications/PrefMasterSwitchToolbarView.kt

shiki

Looking great. Just that one thing left!

jd-alexander added 2 commits November 12, 2019 14:35

icon within avatar container no longer announces itself since it's no…

63181e0

…t important for accessibility.

Increased text contrast of new post description on Follow Sites dialog.

5d3b08e

Also increased touch target row container that's within switch control used for notification settings.

jd-alexander added [Type] Enhancement Notifications Accessibility labels Nov 14, 2019

jd-alexander added this to the 13.7 milestone Nov 14, 2019

jd-alexander added 3 commits November 14, 2019 11:19

Added a lighter color to SearchView's hint.

1989207

Did this programmatically as styling it was being proven to be more difficult than expected.

made color value inline and checked if activity is null.

939e025

jd-alexander marked this pull request as ready for review November 15, 2019 19:58

jd-alexander requested a review from shiki November 15, 2019 19:59

jd-alexander commented Nov 15, 2019

View reviewed changes

...rc/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.java Outdated Show resolved Hide resolved

jkmassel modified the milestones: 13.7, 13.8 Nov 18, 2019

shiki reviewed Nov 25, 2019

View reviewed changes

jd-alexander added 2 commits November 26, 2019 11:50

Modified colour value to get rid of contrast warning.

4837104

Updated the semantics of the toolbar content description functionality.

7f26741

jd-alexander requested a review from shiki November 26, 2019 17:30

shiki reviewed Nov 26, 2019

View reviewed changes

SearchView hint color functionality made static.

fe68ca6

jd-alexander requested a review from shiki November 26, 2019 20:33

shiki approved these changes Nov 27, 2019

View reviewed changes

shiki merged commit 429f5a1 into develop Nov 27, 2019

shiki deleted the issue/10735-accessibility-notifications branch November 27, 2019 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accessibility : Notification Improvements #10799

Accessibility : Notification Improvements #10799
shiki merged 8 commits intodevelopfrom
issue/10735-accessibility-notifications

jd-alexander commented Nov 14, 2019 •

edited

Loading

Uh oh!

peril-wordpress-mobile bot commented Nov 14, 2019 •

edited

Loading

Uh oh!

Uh oh!

jkmassel commented Nov 18, 2019

Uh oh!

shiki left a comment

Uh oh!

Uh oh!

Uh oh!

shiki Nov 25, 2019

Uh oh!

jd-alexander Nov 26, 2019 •

edited

Loading

Uh oh!

shiki Nov 26, 2019

Uh oh!

jd-alexander Nov 26, 2019

Uh oh!

jd-alexander Nov 26, 2019

Uh oh!

Uh oh!

Uh oh!

shiki left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jd-alexander commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Main

TalkBack Audit

Settings

Accessibility Scanner Audit

TalkBack Audit

Notifications On Switch

Uh oh!

peril-wordpress-mobile bot commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jkmassel commented Nov 18, 2019

Uh oh!

shiki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shiki Nov 25, 2019

Choose a reason for hiding this comment

Uh oh!

jd-alexander Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shiki Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

jd-alexander Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

jd-alexander Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shiki left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jd-alexander commented Nov 14, 2019 •

edited

Loading

peril-wordpress-mobile bot commented Nov 14, 2019 •

edited

Loading

jd-alexander Nov 26, 2019 •

edited

Loading