Skip to content
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

No differentiation between completed lessons and locked lessons in topics tab #4648

Closed
vrajdesai78 opened this issue Oct 12, 2022 · 4 comments · Fixed by #4958
Closed

No differentiation between completed lessons and locked lessons in topics tab #4648

vrajdesai78 opened this issue Oct 12, 2022 · 4 comments · Fixed by #4958
Assignees
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@vrajdesai78
Copy link
Contributor

Describe the bug
In topic lessons tab, when we use screen reader there is not differentiation between completed lessons and locked lessons. Also, locked lessons are having content description as "lessons in progress" which should be locked lessons.

To Reproduce
Steps to reproduce the behavior:

  1. Go to Any Story
  2. Click on Lessons Tab
  3. Turn on talkback reader
  4. Navigate to the list of lessons.

Expected behavior
The content description for completed lessons, locked lessons and In Progress lesson should be appropriate to it's state. For example, for locked chapter content description should have locked chapter in the end.

Demonstrationtion
https://user-images.githubusercontent.com/43074241/195417529-357236fe-1d37-45a1-888f-51158ea89822.mp4

Environment

  • Device/emulator being used: Realme GT Master Edition
  • Android or SDK version (e.g. Android 5 or SDK 21): Android 12
  • App version (you can get this through system app settings or via the admin controls menu in-app): 1.0
@JishnuGoyal
Copy link
Contributor

JishnuGoyal commented Oct 12, 2022

Hi @vrajdesai78! Nice find! Although the TalkBack should have automatically worked as expected correctly, as there weren't any specific changes introduced to that part, but I'll investigate on this and work on a fix.

Thanks!

@BenHenning BenHenning added issue_type_bug Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Z-ibt Temporary label for Ben to keep track of issues he's triaged. labels Dec 23, 2022
@JishnuGoyal
Copy link
Contributor

Unassigning myself so that someone else can pick this issue. Although I haven't digged into the details but this might be a good first/second issue. WDYT @BenHenning ?

@JishnuGoyal JishnuGoyal removed their assignment Jan 10, 2023
@BenHenning
Copy link
Member

BenHenning commented Jan 11, 2023

I think so, but @JishnuGoyal could you provide a few code points and a brief write-up (e.g. bullet items of what roughly needs to change; this should be in a reply comment not a doc) of how you think this can be solved? That's more or less the bar that we're trying to establish for starter issues moving forward (to reduce the back-and-forths needed with new contributors who are trying to familiarize themselves).

@seanlip seanlip added bug End user-perceivable behaviors which are not desirable. and removed issue_type_bug labels Mar 29, 2023
@kkmurerwa
Copy link
Collaborator

Problem Statement and Cause

I was able to successfully replicate the issue. As stated in the issue, In-progress and completed lessons are read correctly but locked lessons are considered in-progress lessons.

This is caused by a logic flaw. The control structure in the computeChapterPlayStateIconContentDescription() of the ChapterSummaryViewModel assumes that any ChapterPlayState that is not ChapterPlayState.COMPLETED is automatically a ChapterPlayState.IN_PROGRESS_*. This is what makes Talkback to read the content description to locked lessons as in-progress lessons.

Proposed Solution

Below is my suggested approach to the problem;

  • Update the ChapterSummaryViewModel and add a string constructor parameter val previousChapterTitle: String. This will be needed to compile the content description needed for Talkback in the locked lessons.

  • Update the StorySummaryViewModel on the computeChapterSummaryItemList() method and add an argument for the previousChapterTitle parameter.

  • Add a condition in the if statement on the computeChapterPlayStateIconContentDescription() method for the ChapterPlayState.NOT_PLAYABLE_MISSING_PREREQUISITES state. This will differentiate locked lessons from in-progress lessons, since they typically have the missing prerequisites state.

  • In the ChapterPlayState.NOT_PLAYABLE_MISSING_PREREQUISITES statement body, invoke the resourceHandler.getStringInLocaleWithWrapping() with the R.string.chapter_prerequisite_title_label string already existing as a string resource.

  • Update the call to also include index instead of index + 1. Because we need to pass the previous chapter title to the talkback. We will also add our previousChapterTitle variable to the method call. Our final condition should look like this;

    ChapterPlayState.NOT_PLAYABLE_MISSING_PREREQUISITES -> {
        resourceHandler.getStringInLocaleWithWrapping(
          R.string.chapter_prerequisite_title_label, (index).toString(), previousChapterTitle
        )
    }
    

That should be able to successfully fix the issue.

BenHenning pushed a commit that referenced this issue May 9, 2023
… and in-progress lessons (#4958)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->
Fix #4648: When this PR is merged, it will fix the issue of locked,
completed and in progress lessons not being differentiated by Talkback.

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] 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: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
<!-- Delete these section if this PR does not include UI-related
changes. -->
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](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Z-ibt Temporary label for Ben to keep track of issues he's triaged.
5 participants