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

[BUG]: Arabic and Portuguese lessons are missing images #719

Closed
seanlip opened this issue Jun 8, 2023 · 14 comments · Fixed by oppia/oppia-android#5040
Closed

[BUG]: Arabic and Portuguese lessons are missing images #719

seanlip opened this issue Jun 8, 2023 · 14 comments · Fixed by oppia/oppia-android#5040
Assignees
Labels
bug Something isn't working

Comments

@seanlip
Copy link
Member

seanlip commented Jun 8, 2023

Describe the bug
Some images are missing in Arabic versions of the lessons. This is serious because learners will not be able to proceed through the lessons without those images.

To Reproduce
Steps to reproduce the behavior:

  1. Download the 0.11-beta release.
  2. Set the language of the app to Arabic.
  3. Play the first 4 cards of Fractions 1. The first, third and fourth cards are supposed to have images (the fourth card has multiple), but they do not show up. The second card has an image and that renders fine.

Expected behavior
All images should show up in the lesson.

Demonstration

Arabic cards English cards
Screenshot_20230608-191448 Screenshot_20230608-191822
Screenshot_20230608-191453 Screenshot_20230608-191830
Screenshot_20230608-191457 Screenshot_20230608-191835

Environment

  • Device/emulator being used: Moto device
  • Android or SDK version (e.g. Android 5 or SDK 21): Android 8
  • App version (you can get this through system app settings or via the admin controls menu in-app): 0.11-beta-8c81c98d8b
@adhiamboperes
Copy link

Traced this to oppia/oppia-android@fbf58d7 which sets up the images from text (content description text
from alt-with-value). This fails because images for pt-BR and ardon't have content descriptions.

@adhiamboperes adhiamboperes changed the title [BUG]: Arabic lessons are missing images [BUG]: Arabic and Portoguese lessons are missing images Jun 8, 2023
@adhiamboperes adhiamboperes changed the title [BUG]: Arabic and Portoguese lessons are missing images [BUG]: Arabic and Portuguese lessons are missing images Jun 8, 2023
@adhiamboperes
Copy link

Edited issue title because both languages are affected.

@seanlip
Copy link
Member Author

seanlip commented Jun 9, 2023

@adhiamboperes Thanks, this is a great find!

It's not a guarantee yet on the server that every image embedding has the necessary alt text, though this is something we do plan to look into (there are a bunch of older image translations which do not). I didn't realize Android was relying on the existence of alt text so strongly though. Is it possible to modify things so that images without alt text can still be displayed?

@seanlip
Copy link
Member Author

seanlip commented Jun 9, 2023

Also @adhiamboperes -- just to confirm, checkouts before the commit you mention result in the necessary images showing?

@adhiamboperes
Copy link

It is possible to add some logic to show images using the non-interactive-image attribute.

I believe the reason the alt-text usage was introduced was for a11y, as per oppia/oppia-android#3432

@seanlip
Copy link
Member Author

seanlip commented Jun 9, 2023

If we were to show images using the non-interactive-image attribute, what impact does that have on a11y?

@adhiamboperes
Copy link

Also @adhiamboperes -- just to confirm, checkouts before the commit you mention result in the necessary images showing?

I haven't verified that yet, but what we did verify, (@BenHenning also) was that modifying some of the logic introduced in the commit allowed the images to display.

@adhiamboperes
Copy link

If we were to show images using the non-interactive-image attribute, what impact does that have on a11y?

My best guess right now is that the content description of an image would not be read out by TalkBack.

@seanlip
Copy link
Member Author

seanlip commented Jun 9, 2023 via email

@BenHenning
Copy link
Member

FYI this is effectively shares a root cause with #718, though that also is caused by pipeline issues.

@BenHenning BenHenning self-assigned this Jun 9, 2023
adhiamboperes referenced this issue in adhiamboperes/oppia-android 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]>
@KolliAnitha
Copy link

Image is missing in the Fractions chapter in 5th page

screen-20230612-152522.mp4

@KolliAnitha KolliAnitha reopened this Jun 12, 2023
@seanlip
Copy link
Member Author

seanlip commented Jun 13, 2023

This is a content issue, transferring to lessons team.

@seanlip seanlip transferred this issue from oppia/oppia-android Jun 13, 2023
@adhiamboperes adhiamboperes removed their assignment Jun 13, 2023
@BenHenning
Copy link
Member

Hi. Is there any estimate on when this might get fixed? It's considered a priority issue for Oppia Android, so it'd be nice to understand a rough timeline so that we can plan accordingly when this might be available to ship to production.

@seanlip
Copy link
Member Author

seanlip commented Sep 22, 2023

This is now fixed on the website. Closing.

@seanlip seanlip closed this as completed Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

4 participants