QuantumBadger
Add accessibility actions for jumping between level 1 comments
This PR adds actions for jumping among level 1 comments, using the existing previousButton/nextButton logic. While it isn't quite my vision (collapsing the nearest l1), this will significantly ease navigation when it works properly.
While this does jump among l1s, focus is set extremely unreliably. @QuantumBadger might you be able to look into why?
Closes #964.
Thank you @codeofdusk! I'll review this soon and will look into the focus issue.
@codeofdusk After spending a while investigating this, I've put in a hack which explicitly sets the accessibility focus to the jumped-to element, after a delay of 500ms to allow the RecyclerView to re-layout.
Please let me know if this works for you!
@QuantumBadger This does improve reliability, though not perfectly. Also, we're now getting #1023 type behaviour when invoking the actions – I find the other accessibility subtitle style more readable. That said, I imagine the new subtitle might be better for Braille users as it's more compact.
Thanks @codeofdusk, don't worry I haven't forgotten about this :) I'll be looking at normal development tasks again once #1059 is resolved, as it is an existential threat to RedReader and will probably need some significant dev effort to work around unless Reddit change their minds. I'm also moving house next week which is taking up most of my time at the moment.
By #1023-type behaviour, I take it you mean that the accessibility subtitle doesn't get read out, but rather the visible text gets read out instead?
@QuantumBadger Glad to see that RedReader survives (at least for now)!
By #1023-style behaviour, I mean that TalkBack says things like "pts" instead of "up votes" or "up" and reads things in a more abbreviated style. That style works well for Braille (it's compact) but the normal accessibility subtitle works better for speech.
One possibility (suggested to me by @simon818) is that we mark all l1 comments as headings, so TalkBack's previous/next heading commands can move among them. Not sure how feasible this'd be, but I'd be okay with that solution.
Thanks @codeofdusk, yes I'm glad the app's been given a stay of execution, in no small part thanks to the accessibility work! The "L1 comments as headings" idea seems like it might work, it looks like we can use the setAccessibilityHeading() method on the top-level comment views.
The "L1 comments as headings" idea seems like it might work, it looks like we can use the setAccessibilityHeading() method on the top-level comment views.
It's been a while since I've looked at the app. Want to take this one, or should I? If the latter, is there an easy way to determine whether a comment is an l1?
Happy to take a look at this myself in a moment! I should be able to use the indent level inside RedditCommentView for detecting L1 comments.
Hmm, looks like it's more complicated than that unfortunately. Since the RecyclerView lazily generates the View instances, the view for the next "heading" doesn't exist yet, so there's nothing to jump to.
When I get a chance I'll take another look at the original PR and try to fix the remaining reliability issues.
@codeofdusk No, I haven't had time to look into this further unfortunately. I'm still planning to take one more look to see if I can fix the original PR.
@codeofdusk I think I'll merge this in as it's probably useful even if there are focus issues sometimes. As far as I can tell the focus issues are an Android issue and there's not much we can do to fix it. I put a workaround in place previously and it worked for me, and maybe the timeout can be tweaked to make it more reliable.
The issue with the accessibility subtitle also seems like an Android issue -- I'll do some more testing once this is merged in but I don't remember seeing this personally. In theory there's no reason why Android should pick a different item description depending on how you navigated to the item.
In the longer term:
- I'd like to implement a "collapse child comments" feature as this is often requested, and sidesteps the need for the extra skip actions
- The plan is to gradually migrate the app's UI to Jetpack Compose instead of Android Views, and in the upcoming version 1.24 the initial groundwork was done for this (with the album viewer). So I'd like to avoid investing much more time in the legacy Views-based UI. Compose handles accessibility differently and it's possible the issues we've seen here will no longer apply.