-
Notifications
You must be signed in to change notification settings - Fork 612
Fix: Improve TalkBack content description for concept card links #6033
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
base: develop
Are you sure you want to change the base?
Fix: Improve TalkBack content description for concept card links #6033
Conversation
|
I have signed the CLA. |
|
Hi! This is my first PR. |
|
Hi @CodeWithSangeeta, I checked your fauling CI checks and saw there are failing tests. Some for the code paths you changed have existing regression tests related to talkback/content descriptions, and the tests either need to be updated to have the latest content description, or help verify whether the fix is correct. I clicked on the failing test shards and searched "failures", e.g.: The Wiki also has a section on static checks and understanding CI failures, FYI. |
|
Thank you for explaining the failing tests and for rerunning the workflows. I understand now that the failures are due to UI tests still expecting the old TalkBack content descriptions. I will update the failing test files to match the new content descriptions from my change. Really appreciate your help and guidance! |
|
Hi! I’ve updated both StateFragmentTest and ExplorationActivityTest to match the new TalkBack concept card text and pushed the fix. The CI checks should rerun automatically now. Thanks again for the guidance! |
|
Hi @adhiamboperes ,just a gentle follow-up on this PR. I’ve updated the failing tests based on the earlier feedback and pushed the fixes. Please let me know if anything else is needed from my side. Thank you! |
content description for concept card links
adhiamboperes
left a comment
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 @CodeWithSangeeta!
You solution is definitely in the right direction, but I have left some comments. Please ping me incase you run into issues.
|
|
||
| /** Override default HTML parsing to include custom concept card tags when generating TalkBack-readable text for Solution and Flashback dialogs. | ||
| The previous implementation ignored custom tags, causing TalkBack to skip this fix ensures that TalkBack reads the concept card reference | ||
| exactly once and consistently. |
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.
Please reformat this KDoc so that the sentences are clearly seperated.
| /** Updated content description logic for accessibility. | ||
| TalkBack must read the skill ID in concept card links(e.g., “test_skill_id_1 concept card.”) | ||
| Previously only the visible text was read, causing TalkBack to skip concept card references in Hint, Solution, and Flashback flows. | ||
| */ |
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.
It is okay to remove this KDoc. Interface methods are typically documented inside the interface itself, rather than at the implementation site.
| exempted_file_path: "utility/src/main/java/org/oppia/android/util/parser/html/ | ||
| ConceptCardTagHandler.kt" |
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.
Please revert this change, it looks like an erronous formatting change.
app/src/test/java/org/oppia/android/app/player/state/StateFragmentLocalTest.kt
Show resolved
Hide resolved
|
|
||
| onView(withId(R.id.solution_summary)) | ||
| .perform(openClickableSpan("test_skill_id_1 concept card")) | ||
| .perform(openClickableSpan("Click on this test_skill_id_1 concept card..")) |
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.
Should the span content change always, or just when talkback is enabled? I believed it is for the latter scenario.
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.
Ditto the other modified tests.
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.
Good point, thanks for flagging this @adhiamboperes
The intention was to improve TalkBack output, not change visible span text universally.
I’ll update the implementation so the visible span remains unchanged and only the contentDescription reflects the TalkBack-specific wording.
Please tell me what to do??
app/src/main/java/org/oppia/android/app/hintsandsolution/SolutionViewModel.kt
Show resolved
Hide resolved
| */ | ||
| fun createConceptCardLinkClickListener(): ConceptCardLinkClickListener { | ||
| return object : ConceptCardLinkClickListener { | ||
| return object : ConceptCardTagHandler.ConceptCardLinkClickListener { |
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.
It is not clear to me why this change was required. Please verify and maybe revert.
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 pointing this out. You’re right — this change isn’t required for the TalkBack fix. I’ve reverted it to avoid unnecessary refactoring and verified that the app builds, runs correctly, and the TalkBack behavior remains unchanged
Hii @adhiamboperes , Could you please confirm whether:
|
|
Hi @CodeWithSangeeta, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
|
Hi @adhiamboperes, Quick update to keep this PR active. I’m currently investigating test failures where the flow moves to the success/confetti state after a correct answer, but some tests continue interacting with the previous question UI (concept card / hints). I’m working on confirming the expected behavior and will follow up with a fix or proposal soon. |
|
Hi @CodeWithSangeeta, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
|
Hi @adhiamboperes, |
adhiamboperes
left a comment
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 @CodeWithSangeeta! I have reviewed the PR and suggested fixes for the failing tests. PTAL.
This PR is cquite close to being merge ready.
| val solutionBoxStrokeWidth: Int | ||
| val solutionBoxStrokeWidth: Int, | ||
| private val consoleLogger: ConsoleLogger | ||
|
|
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.
Please remove the unneccessary newline.
| * The previous implementation ignored custom tags. | ||
| * This caused TalkBack to skip concept card references. | ||
| * | ||
| * This fix ensures that TalkBack reads the concept card reference exactly once | ||
| * and consistently. |
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.
Please move this information to the PR body instead of the KDoc, mainly because it is not relevant to describing what the code does.
app/src/main/java/org/oppia/android/app/hintsandsolution/SolutionViewModel.kt
Show resolved
Hide resolved
| onView(withId(R.id.hints_and_solution_summary)) | ||
| .inRoot(isDialog()) | ||
| .perform(openClickableSpan("test_skill_id_1 concept card")) | ||
| .perform(openClickableSpan("Click on this test_skill_id_1 concept card..")) |
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.
| "Remember that two halves, when added together, make one whole." + | ||
| "\nClick on this test_skill_id_1 concept card." | ||
| "Remember that two halves, when added together, make one whole.\n" + | ||
| "Click on this test_skill_id_1 concept card.." |
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.
There should only be one period at the end.
|
|
||
| onView(withId(R.id.solution_summary)) | ||
| .perform(openClickableSpan("test_skill_id_1 concept card")) | ||
| .perform(openClickableSpan("Click on this test_skill_id_1 concept card.")) |
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.
These 2 tests have the same issue as above. Please see the text portion of the stacktrace to see the corrected expected text and use it to update the test.
| * and the associated skill ID to enable further action handling. | ||
| */ | ||
| fun createConceptCardLinkClickListener(): ConceptCardLinkClickListener { | ||
| // return object : ConceptCardTagHandler.ConceptCardLinkClickListener { |
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.
Please remove the commented out code.
| text | ||
| val skillId = attributes.getJsonStringValue(CUSTOM_CONCEPT_CARD_SKILL_ID) | ||
| return if (!skillId.isNullOrBlank()) { | ||
| "$skillId concept card." |
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.
Is it possible that the period here is causing the double periods to appear in the text?(as per tests above?)
| "$skillId concept card." | |
| "$skillId concept card" |
I think this was a weird CI test flake that I noticed a few weeks back, but since I re-run the tests, you can see that only 4 tests are failng and are directly related to the PR. |
|
Hi @adhiamboperes, I’ve followed the review suggestions so far (including addressing the punctuation issue and updating the related tests) and pushed the changes. I’ll keep an eye on the remaining failing tests and follow up if anything else comes up. Thanks again for taking a look. |
|
Hii, I’ve fixed the failing test cases and pushed the updates. Thanks! |
|
Hii, I’ve pushed fixes for the test failures. Could you please re-run the TalkBack CI checks? Thanks! |
|
Hi, I’ve pushed commits to fix the latest TalkBack test failure. |
…ests" This reverts commit cd7b5e7.
|
Hi @adhiamboperes, I reverted my last change since it introduced a type mismatch and didn’t align with the runtime accessibility text. I’m seeing repeated failures in StateFragmentTest and ExplorationActivityTest caused by newline differences in the accessibility contentDescription. I tried aligning the tests with the current output, but the expected formatting (single vs double newline before the concept card text) seems inconsistent across cases. Could you please confirm the intended behavior for the newline/formatting before the concept card text? I’ll wait for confirmation before reapplying further changes. Thanks! |



Explanation
Fix #5915: Improve TalkBack reading for concept card links
This PR updates the accessibility content description for the
<oppia-noninteractive-skillreview>custom HTML tag.Earlier, TalkBack skipped the concept card link values.
Now, TalkBack consistently reads:
“<skill_id> concept card”
This improves the screen-reader experience in:
What this fixes
TalkBack was not reading concept-card links correctly because the custom HTML tag did not provide a clear accessibility description.
This PR fixes the issue and makes the content accessible for learners using screen readers.
What was changed
Updated
ConceptCardTagHandler.getContentDescription()→ Now returns a clean, consistent description using only the
skill_id.Updated
SolutionViewModel→ Now uses
CustomHtmlContentHandler.getContentDescription()together withConceptCardTagHandlerto ensure correct accessibility parsing across hint and solution screens.Additional note
This PR also includes an auto-format update to
scripts/assets/kdoc_validity_exemptions.textprotogenerated automatically by ktlint. No functional changes were made.
Testing done
Tested TalkBack behavior in all places where concept-card links appear:
TalkBack now reads the concept card link:
Ready for review
Screen recording demonstrating TalkBack reading concept card links correctly in all 3 places: 🔗https://drive.google.com/file/d/1y6pqNhccfw5rjqeOtr_VNsF78jvSTvPN/view?usp=sharing
Essential Checklist