Fix #3907: [A11Y] Output Congratulations for screenreader #3980

viktoriias · 2021-11-02T20:24:05Z

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

The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
Any changes to scripts/assets files have their rationale included in the PR explanation.
The PR follows the style guide.
The PR does not contain any unnecessary code changes from Android Studio (reference).
The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

viktoriias · 2021-11-03T00:01:19Z

Screen recording:

3907.mp4

rt4914

@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

BenHenning · 2021-11-05T00:39:46Z

@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:

This is a really good point. I actually need to ask @seanlip about this. Sean: pedagogically, how important is it to communicate the notion of the "correct" banner to the user? I.e. is it actually optional such that it's nice-to-have for sighted users but unimportant for a11y users (since we have redundancy in the feedback as @rt4914 mentioned)? The video @viktoriias linked above demonstrates the behavior with the extra "Correct" announcement (sound needs to be on). FWIW my thinking is that we should not announce it now that we can see what the behavior is here.
Regarding testing, once we actually start using announceForAccessibility we should wrap it in our own container, limit usage via regex exemptions, and swap it out with a test-only fake that we can use to determine if it's been called. I think that's the best that we can do here.

seanlip · 2021-11-05T00:48:24Z

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.

BenHenning · 2021-11-06T00:24:55Z

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?

viktoriias · 2021-11-06T00:58:08Z

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:

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.

BenHenning · 2021-11-06T01:31:05Z

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> .

viktoriias · 2021-11-06T17:50:49Z

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

rt4914 · 2021-11-08T12:17:04Z

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?

@BenHenning Sounds good. We should announce only when Feedback is not available.

rt4914 · 2021-11-08T18:53:05Z

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

@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.
For other cases like FractionInput, it works correctly (though we have to start from top because of DiffUtils issue)

Both of these should work perfectly once #3893 issue is fixed.

BenHenning · 2022-01-12T04:14:35Z

@rt4914 I believe you still need to approve this--PTAL when convenient.

rt4914

Nice clean implementation. Thanks.

viktoriias · 2022-01-14T04:22:02Z

@BenHenning Do you know why I'm getting WorkManager is already initialized. Did you try to initialize it manually without disabling WorkManagerInitializer? error in FakeAccessibilityServiceTest?

BenHenning · 2022-01-14T21:09:25Z

@BenHenning Do you know why I'm getting WorkManager is already initialized. Did you try to initialize it manually without disabling WorkManagerInitializer? error in FakeAccessibilityServiceTest?

@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 @Config should fix it.

Sarthak2601 · 2022-01-14T21:18:30Z

@BenHenning Do you know why I'm getting WorkManager is already initialized. Did you try to initialize it manually without disabling WorkManagerInitializer? error in FakeAccessibilityServiceTest?

@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 @Config should fix it.

Confirming that this is correct and introducing a test application should fix it.

viktoriias · 2022-01-14T22:51:22Z

@BenHenning Do you know why I'm getting WorkManager is already initialized. Did you try to initialize it manually without disabling WorkManagerInitializer? error in FakeAccessibilityServiceTest?

@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 @Config should fix it.

Confirming that this is correct and introducing a test application should fix it.

Thank you, it's passing now!

BenHenning

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

BenHenning

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 <henning.benmax@gmail.com> * Update app/src/main/java/org/oppia/android/app/player/state/StatePlayerRecyclerViewAssembler.kt Co-authored-by: Ben Henning <henning.benmax@gmail.com> * 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 <viktoriia@gmail.com> Co-authored-by: Ben Henning <henning.benmax@gmail.com>

Add announceForAccessibility

fa5836a

viktoriias requested a review from BenHenning as a code owner November 2, 2021 20:24

viktoriias marked this pull request as draft November 2, 2021 20:24

viktoriias changed the title ~~Add announceForAccessibility~~ Fix #3907: [A11Y] Output Congratulations for screenreader Nov 2, 2021

Use resources

f739d9a

viktoriias requested a review from rt4914 November 3, 2021 00:40

viktoriias marked this pull request as ready for review November 3, 2021 00:41

viktoriias assigned rt4914 Nov 3, 2021

Use resourceHandler

aa84dce

viktoriias assigned BenHenning Nov 3, 2021

rt4914 reviewed Nov 3, 2021

View reviewed changes

rt4914 removed their assignment Nov 3, 2021

BenHenning assigned rt4914 and seanlip and unassigned BenHenning Nov 5, 2021

seanlip assigned BenHenning and unassigned seanlip Nov 5, 2021

BenHenning assigned viktoriias and unassigned BenHenning Nov 6, 2021

rt4914 removed their assignment Nov 8, 2021

Only announce "Correct" if there is no feedback.

5b43501

Merge branch 'oppia:develop' into 3907

ba3b3ae

rt4914 approved these changes Jan 12, 2022

View reviewed changes

rt4914 assigned viktoriias and unassigned rt4914 Jan 12, 2022

oppiabot bot added the PR: LGTM label Jan 12, 2022

Viktoriia-Lozova added 4 commits January 12, 2022 10:55

Add TestModule

6c5eb82

Merge remote-tracking branch 'origin/3907' into 3907

1d0554d

Undo add TestModule

868bc5b

Add Config

a619c0d

Add TestApplication

4c13673

viktoriias assigned BenHenning and unassigned viktoriias Jan 14, 2022

BenHenning approved these changes Jan 15, 2022

View reviewed changes

app/src/test/java/org/oppia/android/app/accessibility/FakeAccessibilityServiceTest.kt Outdated Show resolved Hide resolved

BenHenning assigned viktoriias and unassigned BenHenning Jan 15, 2022

nit

4f3c827

viktoriias assigned BenHenning and unassigned viktoriias Jan 15, 2022

viktoriias added 4 commits January 16, 2022 21:44

Merge branch 'oppia:develop' into 3907

f6af5c2

Merge branch 'oppia:develop' into 3907

cf48548

Merge branch 'oppia:develop' into 3907

adedf8a

Merge branch 'oppia:develop' into 3907

b2d80b8

BenHenning approved these changes Jan 19, 2022

View reviewed changes

BenHenning merged commit da47af6 into oppia:develop Jan 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3907: [A11Y] Output Congratulations for screenreader #3980

Fix #3907: [A11Y] Output Congratulations for screenreader #3980

viktoriias commented Nov 2, 2021 •

edited

viktoriias commented Nov 3, 2021 •

edited

rt4914 left a comment

BenHenning commented Nov 5, 2021

seanlip commented Nov 5, 2021

BenHenning commented Nov 6, 2021

viktoriias commented Nov 6, 2021

BenHenning commented Nov 6, 2021 via email

viktoriias commented Nov 6, 2021

rt4914 commented Nov 8, 2021

rt4914 commented Nov 8, 2021

BenHenning commented Jan 12, 2022

rt4914 left a comment

viktoriias commented Jan 14, 2022

BenHenning commented Jan 14, 2022

Sarthak2601 commented Jan 14, 2022

viktoriias commented Jan 14, 2022

BenHenning left a comment •

edited

BenHenning left a comment

Fix #3907: [A11Y] Output Congratulations for screenreader #3980

Fix #3907: [A11Y] Output Congratulations for screenreader #3980

Conversation

viktoriias commented Nov 2, 2021 • edited

Explanation

Essential Checklist

For UI-specific PRs only

viktoriias commented Nov 3, 2021 • edited

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning commented Nov 5, 2021

seanlip commented Nov 5, 2021

BenHenning commented Nov 6, 2021

viktoriias commented Nov 6, 2021

BenHenning commented Nov 6, 2021 via email

viktoriias commented Nov 6, 2021

rt4914 commented Nov 8, 2021

rt4914 commented Nov 8, 2021

BenHenning commented Jan 12, 2022

rt4914 left a comment

Choose a reason for hiding this comment

viktoriias commented Jan 14, 2022

BenHenning commented Jan 14, 2022

Sarthak2601 commented Jan 14, 2022

viktoriias commented Jan 14, 2022

BenHenning left a comment • edited

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

viktoriias commented Nov 2, 2021 •

edited

viktoriias commented Nov 3, 2021 •

edited

BenHenning left a comment •

edited