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

Stats accessibility improvements 2 #10277

Merged
merged 11 commits into from Jul 27, 2019

Conversation

@planarvoid
Copy link
Contributor

commented Jul 24, 2019

This PR looks rather big but it needed to be this way because I had to add the content description to all the ListItems and I didn't want to create a temp solution where the content description would be nullable. If you'd rather have that, I can try to split this PR into 2.

This covers all the list items in the Stats. For example the following screenshot:

Screenshot 2019-07-24 at 15 39 02

would now have the header (Title ... Views) marked as not important for accessibility and selecting any item in the list would read as: Title: Home page / Archives, Views: 24. This should help the users with keeping the context.

There was quite a few of these list items in the Stats but all the changes should be almost the same~

To test:

  • Go to Stats with Talkback turned on
  • Click on all the list items to check the content description
  • Check that the headers are not selectable

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@planarvoid planarvoid added this to the 13.0 milestone Jul 24, 2019

@planarvoid planarvoid requested a review from malinajirka Jul 24, 2019

@planarvoid planarvoid self-assigned this Jul 24, 2019

@peril-wordpress-mobile

This comment has been minimized.

Copy link

commented Jul 24, 2019

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@planarvoid planarvoid requested review from khaykov and removed request for malinajirka Jul 25, 2019

@khaykov
Copy link
Member

left a comment

I haven't noticed anything alarming in the code, but there is a lot of it, and it all looks really similar, so I hope I haven't missed anything :)

Everything works awesome! I have two comments:

A lot of non-clickable list items (Authors, Countries, Month, etc) are announced as clickable with “double-tap to activate”.

When you expand a list item, it's description says "something something, Expand" instead of "Collapse".

<string name="stats_item_expanded">Item expanded</string>
<string name="stats_item_collapsed">Item collapsed</string>
<string name="stats_posting_activity_graph">Graph of posting activity.</string>
<string name="stats_list_item_description">%1$s: %2$s, %3$s: %4$s</string>

This comment has been minimized.

Copy link
@khaykov

khaykov Jul 25, 2019

Member

This will totally raise some questions from translators :)

This comment has been minimized.

Copy link
@planarvoid

planarvoid Jul 26, 2019

Author Contributor

I know :-), any ideas about how to make it nicer? should I add a comment?

This comment has been minimized.

Copy link
@khaykov

khaykov Jul 27, 2019

Member

Hmm. This is a tricky one. I think since they use translated labels in it, it should be ok? We can think of something better if we get asked about it.

@planarvoid planarvoid changed the base branch from feature/stats_accessibility_improvements_1 to develop Jul 26, 2019

@planarvoid

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

thanks for the review @khaykov!

A lot of non-clickable list items (Authors, Countries, Month, etc) are announced as clickable with “double-tap to activate”.

It took me some time to figure this one out so I have to share why it's happening. There is this code in the View.java:

public void setOnClickListener(@Nullable OnClickListener l) {
        if (!isClickable()) {
            setClickable(true);
        }
        getListenerInfo().mOnClickListener = l;
    }

Now, as you see, it's valid to pass in null as a listener but it still sets the view as clickable when the listener is null 😢. I've fixed it by setting it to not clickable after I set the listener to null.

When you expand a list item, it's description says "something something, Expand" instead of "Collapse".

This should be fixed now.

planarvoid and others added some commits Jul 26, 2019

@khaykov
Copy link
Member

left a comment

Thanks for the fixes! Looks good 👍

@khaykov khaykov merged commit c8cd744 into develop Jul 27, 2019

4 checks passed

Peril Found some issues. Don't worry, everything is fixable.
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

@khaykov khaykov deleted the feature/stats_accessibility_improvements_2 branch Jul 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.