-
Notifications
You must be signed in to change notification settings - Fork 506
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
Fix #3907: [A11Y] Output Congratulations for screenreader #3980
Conversation
Screen recording: 3907.mp4 |
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.
@BenHenning Can you have a look at the output for this PR. I have couple of concerns about this:
- Should we actually read out
Congratulations
/Correct
because most of the time once correct answer is submitted we do have some feedback text which says that the answer was correct. - If we do need to announce it, can you suggest some strategy based on which we can test this on Espresso or Robolectic?
Thanks
@rt4914 replies below:
|
For (1), I think it is fine not to announce it. We typically have feedback provided for the correct answer, which should suffice. If you want to be more pedantic, I would say, announce it if and only if the "correct answer feedback" is empty. But this should only happen in a minority of cases. |
Thanks @seanlip. This case is interesting. The current exploration structure does allow feedback to be empty, and so far as I'm aware we don't actually require it in any cases (though maybe it should be required if an answer is labeled as correct?). Either way, I like the framing of relying on feedback to provide context and otherwise to supplement the experience when we know for sure there's no correct context given. @rt4914 & @viktoriias do you have any concerns with this adjustment? |
I know very little about accessibility. On one hand, I would assume that talkback experience should be the same as visual. On the other hand, it may be overwhelming to hear too many sounds or repeating sounds, while seeing "Correct" confirmation twice is fine. To make sure I understand suggested adjustment:
|
Your understanding is correct & matches the proposal.
…On Fri, Nov 5, 2021 at 5:58 PM Viktoriia Schwartz ***@***.***> wrote:
Either way, I like the framing of relying on feedback to provide context
and otherwise to supplement the experience when we know for sure there's no
correct context given. @rt4914 <https://github.com/rt4914> & @viktoriias
<https://github.com/viktoriias> do you have any concerns with this
adjustment?
I know very little about accessibility. On one hand, I would assume that
talkback experience should be the same as visual. On the other hand, it may
be overwhelming to hear too many sounds or repeating sounds, while seeing
"Correct" confirmation twice is fine.
To make sure I understand suggested adjustment:
- We only want to pronounce "Correct" when there is no correct answer
feedback.
- "Correct" toast and confetti will remain unchanged and they will
appear regardless of feedback.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#3980 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADDB3PQOBXWCXGQ42VWBW5LUKSDTVANCNFSM5HHK5QGA>
.
|
I just realized that the feedback is not in the talkback, so we only hear "Correct" once. @rt4914 is not reading the answer feedback intended or will be changed? Screen.recording.mp4 |
@BenHenning Sounds good. We should announce only when Feedback is not available. |
@Viktoriia-Lozova This is happening for cases where we have ItemSelectionInteraction and I suspect thats because of nested recyclerview and there are chances it will happen for DragAndDrop too. Both of these should work perfectly once #3893 issue is fixed. |
@rt4914 I believe you still need to approve this--PTAL when convenient. |
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.
Nice clean implementation. Thanks.
@BenHenning Do you know why I'm getting |
@viktoriias it might be because you're not using a test application (which means Robolectric will use OppiaApplication, and that initializes WorkManager). I'm not certain, but it might be the case that even though Robolectric recreates the test suite object before each test, it still runs in the same process (so static state like WorkManager can be shared across test boundaries). This will result in WorkManager attempting to be initialized multiple times which isn't valid. @Sarthak2601 could probably confirm. If this is the cause, introducing a test application in your suite & configuring it using |
Confirming that this is correct and introducing a test application should fix it. |
Thank you, it's passing now! |
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 @viktoriias! Just one nit. Otherwise, the PR LGTM! Glad to hear that the fix worked.
app/src/test/java/org/oppia/android/app/accessibility/FakeAccessibilityServiceTest.kt
Outdated
Show resolved
Hide resolved
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 @viktoriias. LGTM!
…#3980) * Add announceForAccessibility * Use resources * Use resourceHandler * Only announce "Correct" if there is no feedback. * Wrap announceForAccessibility to accomodate testing. * Add kdoc. * Revert test exploration changes. * Expand comments. * Add checks * Add checks * Remove feedback_1. * Add comma. * feedback_1 * Revert feedback nulls * Remove ? from CharSequence. * Inject accessibilityChecker in the Builder's Factory. * Update kdoc. * Add question player test. * lint * Update utility/src/main/java/org/oppia/android/util/accessibility/FakeAccessibilityChecker.kt Co-authored-by: Ben Henning <[email protected]> * Update app/src/main/java/org/oppia/android/app/player/state/StatePlayerRecyclerViewAssembler.kt Co-authored-by: Ben Henning <[email protected]> * Expand file content checks, add tests. * lint * Fix RegexPatternValidationCheckTest * Fix RegexPatternValidationCheckTest * Fix RegexPatternValidationCheckTest * Fix RegexPatternValidationCheckTest * Fix RegexPatternValidationCheckTest * Move accessibilityChecker to the constructor. * Add more tests. * lint * Add KDoc * Add FakeAccessibilityCheckerTest * Add newline * Add exempted file * Rename AccessibilityChecker to AccessibilityService * Update BUILD file * Update test_file_exemptions.textproto * Update file_content_validation_checks.textproto * Update providers. * Inject accessibilityService in test. * lint * lint * Add TestModule * Undo add TestModule * Add Config * Add TestApplication * nit Co-authored-by: viktoriia <[email protected]> Co-authored-by: Ben Henning <[email protected]>
Explanation
Fixes #3907: [A11Y] Output Congratulations for screenreader. It's not "Congratulations", but "Correct".
To be able to test new behavior, announceForAccessibility is wrapped in its own container (AccessibilityService), and swapped with a test-only fake to determine if it's been called.
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: