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

Fix #5022, #5030, and #5031: Fix localization issues in Beta 0.11 #5040

Merged
merged 5 commits into from
Jun 9, 2023

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Jun 9, 2023

Explanation

Fixes oppia/lessons-team#718
Fixes oppia/lessons-team#719
Fixes #5031

First, it was found during investigation that some images aren't showing up in Arabic or Brazilian Portuguese translations of lessons due to their image control character (that indicates to Android an image needs to be rendered) being replaced by empty reading text. This has been addressed with a more robust content description check and three new tests to detect this case and addresses oppia/lessons-team#718 and oppia/lessons-team#719. This was determined due to an excellent investigation & finding by @adhiamboperes.

Second, some specific images wouldn't render in Arabic specifically. This was due to a bug in AndroidSVG (our dependency library used for rendering SVG files). Per https://github.com/oppia/androidsvg/blob/4bc1d26412f0fb9fd4ef263fa93f6a64f4d4dbcf/androidsvg/src/main/java/com/caverock/androidsvg/utils/CSSFontVariationSettings.java#L73 AndroidSVG uses DecimalFormat to format certain CSS font settings (particularly in the case for dynamic fonts), and AndroidSVG isn't following the conventional advice of https://developer.android.com/reference/java/util/Locale (under "Be wary of the default locale") and https://docs.oracle.com/javase/8/docs/api/java/text/DecimalFormat.html (specific advice to create formatters for specific locales). In summary, AndroidSVG was formatting certain CSS parameters to floats without accounting for the fact that the default locale may change the numeral system when formatting those decimals. A patch to Oppia's fork of AndroidSVG fixes this problem (oppia/androidsvg@1265eb1). Note that this fix will also be included in the later AndroidSVG changes that the app will later be moved to. Additionally, these are unfortunately not changes that can be easily tested without finding specific SVGs to include in tests, and either scanning logcat or relying on screenshot testing. A more suitable testing strategy would be end-to-end tests vis-a-vis #3839.

Third, bullets weren't rendering correctly in Arabic views of lessons (#5031). This was due to two issues:

  1. The previous logic was always assuming a rightward shift to center the bullet which doesn't work in RTL.
  2. The previous logic was using the canvas width to invert X coordinates rather than the clipped space. This wasn't caught previously because the main tested content for bullets was a test revision card whose content spans the width of the screen (which corresponds to the drawing area of the canvas). However, lessons use a more constrained area due to content margins, so the bullets were being placed off screen in the margin whitespace that was being clipped.

See the following screenshots for a demonstration of the new content description tests failing without the fix, and passing with it:

Without fix With fix
image image

A few caveats with the bullet fixes:

  • The FAQ bullets will still not render correctly in Arabic (technically in all non-English languages) due to [BUG]: Bullet lists aren't showing in FAQs for all non-English languages #5034 which requires fixes in the translation strings. They're actually missing the necessary markup elements needed to show their bullet lists correctly.
  • The FAQ style fix can only be tested via basic measurement verification, but fortunately the existing test did require an update and changed in the expected way (the calculated x position of the bullets in the test have moved left 18 pixels). This isn't nearly as good as a visual test, but that will require Intoduce screenshot-testing framework #1815 to be finished, first.

Note that this PR will need to be cherry-picked into the 0.11 release branch.

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

Demonstrations of fixes for oppia/lessons-team#718 (note that, for the first Arabic screenshots, the lesson image isn't actually localized in the content; also, the Naija image does load without the PR fixes but it can be a bit delayed):

Before (without fixes) After (with the fixes of this PR)
image image
image image
image image

Demonstrations of fixes for oppia/lessons-team#719 (only some images actually hit this bug):

Before (without fixes) After (with the fixes of this PR)
image image
image image
image image

Demonstrations of fixes for oppia/lessons-team#718, oppia/lessons-team#719, and #5031 (since Arabic lessons have conflated failures for images, and some of these images also demonstrate the bullet list issue):

Before (without fixes) After (with the fixes of this PR)
image image
image image
image image

First, it was found during investigation that some images aren't showing
up in Arabic or Brazilian Portuguese translations of lessons due to
their image control character (that indicates to Android an image needs
to be rendered) being replaced by empty reading text. This has been
addressed with a more robust content description check and three new
tests to detect this case.

Second, bullets weren't rendering correctly in Arabic views of lessons.
This was due to two issues:
1. The previous logic was always assuming a rightward shift to center
   the bullet which doesn't work in RTL.
2. The previous logic was using the canvas width to invert X coordinates
   rather than the clipped space. This wasn't caught previously because
   the main tested content for bullets was a test revision card whose
   content spans the width of the screen (which corresponds to the
   drawing area of the canvas). However, lessons use a more constrained
   area due to content margins, so the bullets were being placed off
   screen in the margin whitespace that was being clipped.
@BenHenning BenHenning added the PR: Cherrypick requested Indicates that a PR is being requested for being cherrypicked into the ongoing release branch. label Jun 9, 2023
This bumps the AndroidSVG version being used to one that inclueds a fix
for the rendering issue.
@BenHenning
Copy link
Member Author

BenHenning commented Jun 9, 2023

PTAL @adhiamboperes. I've self-reviewed, and an earlier commit before the AndroidSVG version update passed all CI checks. I verified locally that Gradle is building (since I forgot to push the gradle change during the previous commit's almost-finished run), so I'm fairly confident CI will pass.

Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me. I also tested all the changes locally and the linked issues are already fixed. Thanks @BenHenning!

@oppiabot
Copy link

oppiabot bot commented Jun 9, 2023

Unassigning @adhiamboperes since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Jun 9, 2023
@oppiabot
Copy link

oppiabot bot commented Jun 9, 2023

Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@adhiamboperes
Copy link
Collaborator

Re: Caveat(1) on bullet fixes in the PR description: This has now been fixed and merged in.

@seanlip seanlip enabled auto-merge (squash) June 9, 2023 12:19
@seanlip seanlip merged commit 979bd09 into develop Jun 9, 2023
@seanlip seanlip deleted the fix-beta-0.11-issues branch June 9, 2023 12:26
adhiamboperes added a commit to adhiamboperes/oppia-android that referenced this pull request Jun 9, 2023
oppia#5040)

## Explanation
Fixes #5022
Fixes #5030
Fixes oppia#5031

First, it was found during investigation that some images aren't showing
up in Arabic or Brazilian Portuguese translations of lessons due to
their image control character (that indicates to Android an image needs
to be rendered) being replaced by empty reading text. This has been
addressed with a more robust content description check and three new
tests to detect this case and addresses #5022 and #5030. This was
determined due to an excellent investigation & finding by
@adhiamboperes.

Second, some specific images wouldn't render in Arabic specifically.
This was due to a bug in AndroidSVG (our dependency library used for
rendering SVG files). Per
https://github.com/oppia/androidsvg/blob/4bc1d26412f0fb9fd4ef263fa93f6a64f4d4dbcf/androidsvg/src/main/java/com/caverock/androidsvg/utils/CSSFontVariationSettings.java#L73
AndroidSVG uses ``DecimalFormat`` to format certain CSS font settings
(particularly in the case for dynamic fonts), and AndroidSVG isn't
following the conventional advice of
https://developer.android.com/reference/java/util/Locale (under "Be wary
of the default locale") and
https://docs.oracle.com/javase/8/docs/api/java/text/DecimalFormat.html
(specific advice to create formatters for specific locales). In summary,
AndroidSVG was formatting certain CSS parameters to floats without
accounting for the fact that the default locale may change the numeral
system when formatting those decimals. A patch to Oppia's fork of
AndroidSVG fixes this problem
(oppia/androidsvg@1265eb1).
Note that this fix will also be included in the later AndroidSVG changes
that the app will later be moved to. Additionally, these are
unfortunately not changes that can be easily tested without finding
specific SVGs to include in tests, and either scanning logcat or relying
on screenshot testing. A more suitable testing strategy would be
end-to-end tests vis-a-vis oppia#3839.

Third, bullets weren't rendering correctly in Arabic views of lessons
(oppia#5031). This was due to two issues:
1. The previous logic was always assuming a rightward shift to center
the bullet which doesn't work in RTL.
2. The previous logic was using the canvas width to invert X coordinates
rather than the clipped space. This wasn't caught previously because the
main tested content for bullets was a test revision card whose content
spans the width of the screen (which corresponds to the drawing area of
the canvas). However, lessons use a more constrained area due to content
margins, so the bullets were being placed off screen in the margin
whitespace that was being clipped.

See the following screenshots for a demonstration of the new content
description tests failing without the fix, and passing with it:
| Without fix | With fix |
|--------|--------|
|
![image](https://github.com/oppia/oppia-android/assets/12983742/bef30eb2-a966-426c-8902-8ee23850d94a)
|
![image](https://github.com/oppia/oppia-android/assets/12983742/4a69feae-13de-47ad-acb5-c28f4ee72dc3)
|

A few caveats with the bullet fixes:
- The FAQ bullets will still not render correctly in Arabic (technically
in all non-English languages) due to oppia#5034 which requires fixes in the
translation strings. They're actually missing the necessary markup
elements needed to show their bullet lists correctly.
- The FAQ style fix can only be tested via basic measurement
verification, but fortunately the existing test did require an update
and changed in the expected way (the calculated x position of the
bullets in the test have moved left 18 pixels). This isn't nearly as
good as a visual test, but that will require oppia#1815 to be finished,
first.

**Note** that this PR will need to be cherry-picked into the 0.11
release branch.

## Essential Checklist
- [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

Demonstrations of fixes for #5022 (note that, for the first Arabic
screenshots, the lesson image isn't actually localized in the content;
also, the Naija image does load without the PR fixes but it can be a bit
delayed):
| Before (without fixes) | After (with the fixes of this PR) |
|--------|--------|
|
![image](https://github.com/oppia/oppia-android/assets/12983742/6039721d-37e0-4d63-9c70-47cf60548e51)
|
![image](https://github.com/oppia/oppia-android/assets/12983742/7375e028-0d00-4aee-9c14-11cbde10d536)
|
|
![image](https://github.com/oppia/oppia-android/assets/12983742/8fdc7a59-4f49-4ce0-b166-68e3c69377f4)
|
![image](https://github.com/oppia/oppia-android/assets/12983742/90f80b21-308f-49df-b977-2454ed0a7cef)
|
|
![image](https://github.com/oppia/oppia-android/assets/12983742/b5a912b1-7583-40d8-9632-1a7e5562c1e1)
|
![image](https://github.com/oppia/oppia-android/assets/12983742/04a6e2b5-0e71-4e30-8b47-b1c99e16d3cf)
|

Demonstrations of fixes for #5030 (only some images actually hit this
bug):

| Before (without fixes) | After (with the fixes of this PR) |
|--------|--------|
|
![image](https://github.com/oppia/oppia-android/assets/12983742/95a989d6-88d0-4c85-ade8-c86356a0986a)
|
![image](https://github.com/oppia/oppia-android/assets/12983742/905dc36b-de9a-45c1-8ee3-1c202d2602c4)
|
|
![image](https://github.com/oppia/oppia-android/assets/12983742/2cab94b6-c13c-48e5-b53c-7c2826c6f413)
|
![image](https://github.com/oppia/oppia-android/assets/12983742/b5f4d2f9-dafb-476c-8763-eca84f407431)
|
|
![image](https://github.com/oppia/oppia-android/assets/12983742/0c292e78-a00a-45cb-abb9-9bd141dfc7ff)
|
![image](https://github.com/oppia/oppia-android/assets/12983742/6d67652f-b15a-421e-8b87-8ed4365d19f3)
|

Demonstrations of fixes for #5022, #5030, and oppia#5031 (since Arabic
lessons have conflated failures for images, and some of these images
also demonstrate the bullet list issue):

| Before (without fixes) | After (with the fixes of this PR) |
|--------|--------|
|
![image](https://github.com/oppia/oppia-android/assets/12983742/78a6d788-3968-4441-afcf-f7ea9cde270c)
|
![image](https://github.com/oppia/oppia-android/assets/12983742/08b02727-a6fe-43d9-9631-acdd9f03fe02)
|
|
![image](https://github.com/oppia/oppia-android/assets/12983742/8e4943e6-6277-4e97-80f8-a57e384c5354)
|
![image](https://github.com/oppia/oppia-android/assets/12983742/d292e871-7ebc-425f-9127-49edbad92ff7)
|
|
![image](https://github.com/oppia/oppia-android/assets/12983742/315d65a9-10ff-43e8-b472-322df5f4ef37)
|
![image](https://github.com/oppia/oppia-android/assets/12983742/b62e0226-aec4-4a49-acc4-f6f83c1c7509)
|

---------

Co-authored-by: Adhiambo Peres <[email protected]>
@adhiamboperes adhiamboperes added the PR: Cherrypick completed Indicates a cherrypick request was approved & completed for a PR. label Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Cherrypick completed Indicates a cherrypick request was approved & completed for a PR. PR: Cherrypick requested Indicates that a PR is being requested for being cherrypicked into the ongoing release branch. PR: LGTM
Projects
None yet
3 participants