-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 : Posting Activity Stats Improvement #10980
Conversation
You can test the changes on this Pull Request by downloading the APK here. |
TalkBack is announcing Dec as "Deck" will make the content description of the months the long version. |
@@ -233,7 +233,8 @@ sealed class BlockListItem(val type: Type) { | |||
data class LoadingItem(val loadMore: () -> Unit, val isLoading: Boolean = false) : BlockListItem(LOADING_ITEM) | |||
|
|||
data class ActivityItem(val blocks: List<Block>) : BlockListItem(ACTIVITY_ITEM) { | |||
data class Block(val label: String, val boxes: List<Box>) | |||
data class Block(val label: String, val boxItems: List<BoxItem>) | |||
data class BoxItem(val box: Box, val contentDescription: String? = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know what you think about the naming here. I am not sure if I am adhering to the naming convention being used so I am rethinking it. So the BoxItem could be renamed to Box
and the enum BoxStyle
or BoxType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the top level items have the Item
name. I think this one could be still Box
since it's basically a box (from the UI point of view). It's not a list item. I think having a Box and BoxType should be good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay thanks @planarvoid 👌
setupViewDelegate() | ||
} | ||
|
||
private fun setupViewDelegate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the logic that does the announcement lives within this function. As reviewers how do you feel about this? I have been thinking and I was wondering if it's worth breaking this up a bit more with some tests as well or since the functionality works and it achieves the goal of announcing each daily stats then it okay. Let me know :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @jd-alexander, I've originally omitted this card for accessibility on purpose because I think its value is very questionable. If you use the app without talkback, the purpose of this card is to generally see how active your site was in the past months. It's impossible to see what the single day boxes mean (find what date they represent is very hard), wha the number of views on these days are, and you can't click them. It's purely a visual component and I still believe it should be omitted. If we really want to implement it, I'd approach it differently. First of all, I think reading all the items on click and saying the number of values is not useful and is a completely different use case. What about reading only the top days? Maybe ordered by views. Something like:
"The days with very high views for January are: the 1st, the 3rd...". On click you'd read "The days with high views for January are: the 5th, the 7th ...". This would reduce the number of clicks the user has to do and make the talk back semi-useful. The actual number of views can be reached through a different use case.
From the code point of view, I think we should build the list of content descriptions for a month in the use case (because building it is business logic). We should pass this list on the Block
element and not have granular content descriptions on the Boxes (since they are really still not clickable). PostingActivityBlockAnnouncer
doesn't have to be specific to posting activity any more, you'd just pass in a list of content descriptions and a view they are tied to and it would iterate through them on click. I also don't think it needs to be injected since it can't be tested anyway (and it would be used purely as view logic). I'd generally inject classes for 2 reasons, one is they can be mocked in unit tests and the second one is if their construction is too complex (and needs other injected classes). This one is neither.
Let me know if all of this makes sense!
class StatsBlockAdapter(val imageManager: ImageManager) : Adapter<BaseStatsViewHolder>() { | ||
class StatsBlockAdapter( | ||
val imageManager: ImageManager, | ||
private val postingActivityBlockAnnouncer: PostingActivityBlockAnnouncer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class definitely shouldn't be added here since it's not related to all the item types. It's a very specific logic relevant to only Box Item so it should be passed in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks 🙏
@@ -32,6 +33,7 @@ import javax.inject.Inject | |||
class StatsListFragment : DaggerFragment() { | |||
@Inject lateinit var viewModelFactory: ViewModelProvider.Factory | |||
@Inject lateinit var imageManager: ImageManager | |||
@Inject lateinit var postingActivityBlockAnnouncer: PostingActivityBlockAnnouncer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also remove this, related to the comment above.
class BlockListAdapter(val imageManager: ImageManager) : Adapter<BlockListItemViewHolder>() { | ||
class BlockListAdapter( | ||
val imageManager: ImageManager, | ||
private var postingActivityBlockAnnouncer: PostingActivityBlockAnnouncer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also remove this, related to the comment above.
@@ -233,7 +233,8 @@ sealed class BlockListItem(val type: Type) { | |||
data class LoadingItem(val loadMore: () -> Unit, val isLoading: Boolean = false) : BlockListItem(LOADING_ITEM) | |||
|
|||
data class ActivityItem(val blocks: List<Block>) : BlockListItem(ACTIVITY_ITEM) { | |||
data class Block(val label: String, val boxes: List<Box>) | |||
data class Block(val label: String, val boxItems: List<BoxItem>) | |||
data class BoxItem(val box: Box, val contentDescription: String? = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the top level items have the Item
name. I think this one could be still Box
since it's basically a box (from the UI point of view). It's not a list item. I think having a Box and BoxType should be good enough.
fun buildContentDescription(month: String, days: Int, posts: Int): String { | ||
return if (posts > 1) { | ||
resourceProvider.getString( | ||
R.string.stats_posting_activity_description_plural, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't belong here. This is specific to PostingActivity
so it doesn't belong to the reusable Helper
class. Either make the method more abstract and pass all the params or move it to the use case.
class ActivityViewHolder(val parent: ViewGroup) : BlockListItemViewHolder( | ||
class ActivityViewHolder( | ||
val parent: ViewGroup, | ||
private var postingActivityBlockAnnouncer: PostingActivityBlockAnnouncer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also remove this, related to the comment above.
That's a good idea @planarvoid I originally decided to go with this implementation since it's approach was merged into the iOS app.
Okay understood!
Good point!
Yes, it does. Overall, what's the decision that's normally taken when there's a variation with an implementation on one platform? In this case, I could do the month content description approach but it would be definitely from that of iOS but it should be fine since it's accessibility right? |
@jd-alexander that's a very good question. I personally don't think it's super important to have everything 100% the same. I generally try to think about the changes (if they make sense) and if I think something doesn't look right, I give feedback to the designer (or to the iOS team). Sometimes they overlook a better solution and it's fine to iteratively improve one platform based on the findings of devs on the other. I think this is especially important in the accessibility improvements because even little changes to the usability can make the lives of the users so much more difficult/easy (controlling the app without seeing the screen is hard). In this case I'd suggest this solution to the iOS team (maybe you can also document it in a post) but I also think it's fine to have the solutions slightly different. |
@planarvoid thanks again for the review and thoughts. They have been very helpful. I implemented the solution you discussed in the comments above. I created another PR since this one was specific to the approach being taken. Let me know what you think 😄 I will be closing this one. |
Fixes #10975
Findings
The Posting Stats was inaccessible when using TalkBack. This improvement makes it more accessible for visually impaired users who are navigating the stats functionality.
Solution
The solution was to loop through each
Box
that represents a day and announce the associated stats. This implementation is similar to what was done on iOS. TheBoxes
didn't have a content description so refactoring was done to generate it based on the month, day and amount of posts.Testing
Reviewing
Only 1 reviewer is needed but anyone can review.
Submitter Checklist
RELEASE-NOTES.txt
if necessary.