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.

Sign up for GitHub

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

Jump to bottom

Improve text scaling according to accessibility scanner #4586

Closed
vrajdesai78 opened this issue Sep 11, 2022 · 5 comments · Fixed by #4587 or #4695
Closed

Improve text scaling according to accessibility scanner #4586

vrajdesai78 opened this issue Sep 11, 2022 · 5 comments · Fixed by #4587 or #4695
Assignees
@vrajdesai78
Labels
Impact: Low Low perceived user impact (e.g. edge cases). Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@vrajdesai78
Copy link
Contributor

Describe the bug
In Android 12, accessibility scanner is showing suggestion to improve text scaling for cases in which we have set fixed width or height of view group which will create a problem in scaling test.

Expected behavior
Accessibility scanner should not show any improvements for text scaling

Demonstration
image

image

Environment

  • Device/emulator being used: Realme GT Master Edition
  • Android version: Android 12
  • App version : 1.0.0
@vrajdesai78 vrajdesai78 self-assigned this Sep 11, 2022
@vrajdesai78 vrajdesai78 mentioned this issue Sep 11, 2022
Merged
6 tasks
@BenHenning BenHenning added issue_type_bug Impact: Low Low perceived user impact (e.g. edge cases). Z-ibt Temporary label for Ben to keep track of issues he's triaged. labels Sep 14, 2022
rt4914 pushed a commit that referenced this issue Sep 20, 2022
@vrajdesai78
Fixes #4586: Improve text scaling according to accessibility scanner (#…
91014af 
…4587)

* Fixed text-scaling issue by setting min Height and width

* Navigation drawer scaling fixed

* text-scaling fixed in promoted-story-card

* Removed minHeight=48dp

* Fixed more text scaling issues

* Optimized code resume lesson

* layout_height set to 0dp

* Updated promoted_story_card
BenHenning added a commit that referenced this issue Oct 31, 2022
@BenHenning
Revert "Fixes #4586: Improve text scaling according to accessibility …
419801c 
…scanner (#4587)"

This reverts commit 91014af.
@BenHenning BenHenning mentioned this issue Oct 31, 2022
Merged
6 tasks
@BenHenning
Copy link
Member

Reopening due to #4587 being reverted in #4686.

@BenHenning BenHenning reopened this Nov 2, 2022
@vrajdesai78
Copy link
Contributor Author

vrajdesai78 commented Nov 4, 2022 •
edited
Loading

@BenHenning @rt4914 for this particular issue only in promotes story card we are getting text scaling issues, other than that almost all of the issues are already solved in this #4686 PR.

For promoted story card I get following suggestion from accessibility scanner.
image

The suggestion shows that ViewGroup has a fixed width and contains a TextView with scalable text. Here, to solve this issue we need to set width as wrap content. As per the #4684 we can't set the width as wrap content, we need to give fixed width such that all promoted story cards have same width.

I think that here we need to compromise with the text scaling issue as we can't fix it for promoted story card.

@rt4914
Copy link
Contributor

rt4914 commented Nov 4, 2022

@BenHenning @vrajdesai78 I have tried solving this issue and so far using sp instead of dp for fixed width is the only solution that looks best to me. Have a look at below screenshots (smallest text vs largest text)

Other solution attempts were:

  • removing fixed width altogether - results in distorted cards
  • using autotextview - works only above version 26

@vrajdesai78
Copy link
Contributor Author

vrajdesai78 commented Nov 4, 2022 •
edited
Loading

@rt4914 I agree with your solution, but if we make the fontsize in dp then accessibility scanner will give another suggestion that fontsize should in sp instead of dp. So any approach we take, we have to compromise to atleast one problem either accessibility or inconsistent size of card. I think we can't keep the size of card inconsistent and also doing dp will give another suggestion of changing it back to sp. So, I think that even if we go with the current implementation it's fine. @BenHenning WDYT

@rt4914
Copy link
Contributor

rt4914 commented Nov 4, 2022

instead

Just to be clear I have not used dp for font size. Instead I have used sp for the card width. Also, notice that the width/height of card is different in above screenshots that's because of using sp, if it was dp then it would have been fixed width/height.

@vrajdesai78 vrajdesai78 mentioned this issue Nov 4, 2022
Merged
6 tasks
rt4914 pushed a commit that referenced this issue Nov 9, 2022
@vrajdesai78
Fixes #4586: Improve text scaling according to screen reader (#4695)
69c9a2b 
## Explanation
Fixes #4586: Changed layout_width to sp instead of dp as per this
[comment](#4586 (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
<!-- 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)).

## Screenshots For Mobile (Promoted Story Card)

Default Fonts          |  Large Fonts
:-------------------------:|:-------------------------:
![Screenshot from 2022-11-05
01-27-47](https://user-images.githubusercontent.com/43074241/200065857-3408aca5-7cac-484b-bef8-91aac6a35047.png)
| ![Screenshot from 2022-11-05
01-28-36](https://user-images.githubusercontent.com/43074241/200065890-89013801-4b13-4fec-a53c-e77b802eedde.png)
![Screenshot from 2022-11-05
01-30-00](https://user-images.githubusercontent.com/43074241/200066086-30a9b533-5a29-4f86-919d-a0fdc3ab8e9b.png)
| ![Screenshot from 2022-11-05
01-29-23](https://user-images.githubusercontent.com/43074241/200066127-8718ce6c-d22f-4d7d-96b4-9f2698bc3185.png)

## Screenshots for Tablet (Promoted Story Card)
Default Fonts          |  Large Fonts
:-------------------------:|:-------------------------:
![Screenshot from 2022-11-05
01-31-14](https://user-images.githubusercontent.com/43074241/200066306-d0ebedeb-af87-43e4-9509-a7a50e95d77e.png)
| ![Screenshot from 2022-11-05
01-31-36](https://user-images.githubusercontent.com/43074241/200066339-6f52bd98-012a-42c7-ab44-ea6ca1593554.png)
![Screenshot from 2022-11-05
01-24-34](https://user-images.githubusercontent.com/43074241/200066489-10dcf823-b6bd-4704-9dba-9bdc6d0cf44e.png)
| ![Screenshot from 2022-11-05
01-25-16](https://user-images.githubusercontent.com/43074241/200066530-6c43a78e-8465-4693-8cdf-7438b23e8b3a.png)

Failing robolectric tests when we change the width in dp. 
![Screenshot from 2022-11-07
14-39-57](https://user-images.githubusercontent.com/43074241/200274329-2040e1bc-ae14-465f-a0a2-3c562e53fb66.png)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vrajdesai78 vrajdesai78

Labels
Impact: Low Low perceived user impact (e.g. edge cases). Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Projects
[Team] Core Learner and Mastery flows & UI Frontend - Android
Archived in project
Milestone
No milestone
4 participants
@rt4914 @seanlip @BenHenning @vrajdesai78