Fixes #4586: Improve text scaling according to screen reader #4695

vrajdesai78 · 2022-11-04T20:17:07Z

Explanation

Fixes #4586: Changed layout_width to sp instead of dp as per this comment. This actually solves the problem of text scaling. Improved text scaling by setting layout_height and layout_width to wrap_content and setting min_height = 48dp and min_width = 48dp (may vary in some cases). Introduced a tests which will make sure that width is set in SP not in DP.

Note: In accessibility scanner it still shows to improve text scaling but visually or practically this solution looks accurate.

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

Screenshots For Mobile (Promoted Story Card)

Default Fonts	Large Fonts

Screenshots for Tablet (Promoted Story Card)

Default Fonts	Large Fonts

Failing robolectric tests when we change the width in dp.

BenHenning · 2022-11-05T02:50:02Z

Thanks @vrajdesai78. I think this seems sensible, but I have two questions:

Doesn't the rest of Fixes #4586: Improve text scaling according to accessibility scanner #4587 need to be rolled forward/re-added in order to fully fix Improve text scaling according to accessibility scanner #4586?
How do we ensure that this doesn't break again in the future? It'd be ideal to add some tests to ensure it doesn't break.

…d-list

…om/vrajdesai78/oppia-android into Improve-text-scaling-promoted-list

vrajdesai78 · 2022-11-05T10:14:07Z

Thanks @vrajdesai78. I think this seems sensible, but I have two questions:

Doesn't the rest of Fixes #4586: Improve text scaling according to accessibility scanner #4587 need to be rolled forward/re-added in order to fully fix Improve text scaling according to accessibility scanner #4586?

How do we ensure that this doesn't break again in the future? It'd be ideal to add some tests to ensure it doesn't break.

I have added all changes from previous PR. For test I have added one test but not sure it was not failing when we have width set in dp.

rt4914

@vrajdesai78 PTAL

app/src/main/res/layout/promoted_story_card.xml

app/src/main/res/layout/recently_played_fragment.xml

app/src/main/res/values/dimens.xml

BenHenning

Thanks @vrajdesai78. Took a full pass. Many of the comments are duplicates, but I wanted to make sure each case I found for repeated issues were covered by comments.

From a top-level perspective:

I think the PR title needs to be updated since this PR is now covering more than just the promoted story list fixes
In the original PR there were changes made to app/src/main/res/layout/recently_played_fragment.xml -- why were these dropped here?
Are there any other files that require similar changes (e.g. for toolbars & nav menus)?
How will developers make sure that they know how to address these sp vs. dp cases long-term? Having a test or check seems important here since it's clear that simply reusing an existing pattern may have different results depending on the view being modified.

app/src/main/res/layout/license_text_viewer_activity.xml

app/src/main/res/layout/mark_chapters_completed_fragment.xml

app/src/main/res/layout/mark_stories_completed_fragment.xml

app/src/main/res/layout/mark_topics_completed_fragment.xml

app/src/main/res/layout/my_downloads_fragment.xml

app/src/main/res/layout/promoted_story_card.xml

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt

BenHenning

Thanks @vrajdesai78. Took another pass--PTAL.

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt

app/src/main/res/values/dimens.xml

app/src/main/res/layout/profile_progress_header.xml

app/src/main/res/layout/topic_practice_footer_view.xml

app/src/main/res/values/dimens.xml

app/src/main/res/values/styles.xml

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt

vrajdesai78 · 2022-11-07T09:25:12Z

@BenHenning I have updated test and also added screenshot of failing test case in the PR description. Can you PTAL. Thanks

rt4914

@vrajdesai78 I have one suggested.

Also approving this PR not to block you and also relying for test on @BenHenning 's suggestions.

app/src/main/res/values/dimens.xml

oppiabot · 2022-11-07T17:25:14Z

Unassigning @rt4914 since they have already approved the PR.

rt4914

Blocking this because of this comment thread #4695 (comment) @BenHenning

BenHenning

Thanks @vrajdesai78. Just to clarify one part of your PR description: should "Failing robolectric tests when we change the width in sp." be "Failing robolectric tests when we change the width to dp" (notice the emphasis)? The current wording doesn't seem correct since the entire intent is to be using sp here.

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt

app/src/main/res/values/dimens.xml

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt

vrajdesai78 · 2022-11-08T05:43:32Z

@BenHenning, I have replied to all of your comments, can you PTAL. Thanks

rt4914 · 2022-11-08T15:02:45Z

My latest comment @vrajdesai78 @BenHenning #4695 (comment)

BenHenning

Thanks @vrajdesai78. PR LGTM, so I'm deferring to @rt4914 to resolve my last convo thread (but I'm happy to take another look if needed). I also took a pass on the doc--please let me know when it's ready for another pass.

app/src/main/res/values/dimens.xml

rt4914

Approving this PR.

Replying to @BenHenning comment

The implementation of promoted stories recyclerview will change as mentioned in [A11Y] Improve text scaling in promoted story card #4703 . This change is needed to mainly fix one primary issue, i.e., currently we have a fixed-width of 280 for promoted-story-cards but it should be either match_parent so as to provide better experience to non-sighted users. Other details related to implementation are mentioned in the issue description
No TODO has been mentioned in the code as this is neither related to xml files directly nor to bindable adapter, mostly we will need to create a new adapter to implement this functionality. Currently not sure if we can tie this to one single file, therefore not mentioned in code.
I will apply those wiki changes as mentioned in Fixes #4586: Improve text scaling according to screen reader #4695 (comment)

oppiabot · 2022-11-09T06:46:30Z

Unassigning @rt4914 since they have already approved the PR.

vrajdesai78 added 9 commits September 12, 2022 00:00

Fixed text-scaling issue by setting min Height and width

4f1b02e

Navigation drawer scaling fixed

1e58d73

text-scaling fixed in promoted-story-card

3797b3d

Removed minHeight=48dp

ce34ecc

Fixed more text scaling issues

db60ba7

Optimized code resume lesson

c2eeb36

layout_height set to 0dp

581c7d1

Updated promoted_story_card

90956b3

Changed width to sp from dp

8438a1e

vrajdesai78 requested a review from rt4914 as a code owner November 4, 2022 20:17

vrajdesai78 assigned BenHenning and rt4914 Nov 4, 2022

BenHenning assigned vrajdesai78 and unassigned BenHenning Nov 5, 2022

vrajdesai78 and others added 7 commits November 5, 2022 11:53

Merge branch 'develop' into Improve-text-scaling-promoted-list

b7b7b0e

Merged to latest Develop

7b806b1

Merge branch 'Improve-text-scaling' into Improve-text-scaling-promote…

8bc5fab

…d-list

Merge branch 'Improve-text-scaling-promoted-list' of https://github.c…

bc592ae

…om/vrajdesai78/oppia-android into Improve-text-scaling-promoted-list

Updated promoted_story_card

ea13310

Test added

0637587

Optimized code

15075d4

vrajdesai78 requested a review from BenHenning as a code owner November 5, 2022 10:13

vrajdesai78 assigned BenHenning Nov 5, 2022

rt4914 suggested changes Nov 5, 2022

View reviewed changes

rt4914 removed their assignment Nov 5, 2022

Changed minWidth to sp

435f4a9

vrajdesai78 assigned rt4914 Nov 6, 2022

BenHenning requested changes Nov 6, 2022

View reviewed changes

vrajdesai78 assigned BenHenning Nov 6, 2022

BenHenning reviewed Nov 7, 2022

View reviewed changes

BenHenning removed their assignment Nov 7, 2022

Updated test and changed width to sp

8ed3827

vrajdesai78 assigned BenHenning Nov 7, 2022

rt4914 approved these changes Nov 7, 2022

View reviewed changes

app/src/main/res/values/dimens.xml Show resolved Hide resolved

app/src/main/res/values/dimens.xml Show resolved Hide resolved

oppiabot bot unassigned rt4914 Nov 7, 2022

Merge branch 'develop' into Improve-text-scaling-promoted-list

e61872e

rt4914 suggested changes Nov 7, 2022

View reviewed changes

BenHenning reviewed Nov 8, 2022

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt Outdated Show resolved Hide resolved

app/src/main/res/values/dimens.xml Show resolved Hide resolved

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt Outdated Show resolved Hide resolved

BenHenning removed their assignment Nov 8, 2022

Nit changes

d2fe307

vrajdesai78 assigned BenHenning and rt4914 Nov 8, 2022

Merge branch 'develop' into Improve-text-scaling-promoted-list

973042c

BenHenning approved these changes Nov 8, 2022

View reviewed changes

app/src/main/res/values/dimens.xml Show resolved Hide resolved

BenHenning assigned vrajdesai78 and unassigned vrajdesai78 and BenHenning Nov 8, 2022

rt4914 approved these changes Nov 9, 2022

View reviewed changes

oppiabot bot unassigned rt4914 Nov 9, 2022

oppiabot bot added the PR: LGTM label Nov 9, 2022

vrajdesai78 added 2 commits November 9, 2022 12:19

Merge branch 'develop' into Improve-text-scaling-promoted-list

cc23b9c

Merge branch 'develop' into Improve-text-scaling-promoted-list

f8550cb

rt4914 merged commit 69c9a2b into oppia:develop Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #4586: Improve text scaling according to screen reader #4695

Fixes #4586: Improve text scaling according to screen reader #4695

vrajdesai78 commented Nov 4, 2022 •

edited

Loading

BenHenning commented Nov 5, 2022

vrajdesai78 commented Nov 5, 2022

rt4914 left a comment

BenHenning left a comment

BenHenning left a comment

vrajdesai78 commented Nov 7, 2022

rt4914 left a comment

oppiabot bot commented Nov 7, 2022

rt4914 left a comment

BenHenning left a comment

vrajdesai78 commented Nov 8, 2022

rt4914 commented Nov 8, 2022

BenHenning left a comment

rt4914 left a comment

oppiabot bot commented Nov 9, 2022

Fixes #4586: Improve text scaling according to screen reader #4695

Fixes #4586: Improve text scaling according to screen reader #4695

Conversation

vrajdesai78 commented Nov 4, 2022 • edited Loading

Explanation

Essential Checklist

Screenshots For Mobile (Promoted Story Card)

Screenshots for Tablet (Promoted Story Card)

BenHenning commented Nov 5, 2022

vrajdesai78 commented Nov 5, 2022

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

vrajdesai78 commented Nov 7, 2022

rt4914 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Nov 7, 2022

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

vrajdesai78 commented Nov 8, 2022

rt4914 commented Nov 8, 2022

BenHenning left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Nov 9, 2022

vrajdesai78 commented Nov 4, 2022 •

edited

Loading