1815637 - [Bug][RTL] Potentially don't set automatic direction based on the text directionality, for text across the UI

Status: VERIFIED FIXED

(Fenix :: General, defect)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

From github: https://github.com/mozilla-mobile/fenix/issues/7861.

Original bug filed under GeckoView by @itiel. Screenshots attached to original bug to demonstrate.

Currently, (almost) everywhere in the UI the alignment of varios text elements is decided based of the text directionality.
This usually makes sense, but this an issue where English text actually should appear.
See the attached screenshots with examples for this.

While it's possible to address each text element separately, I don't see any reason (well, from RTL POV anyway) not to make the text align to the left for LTR, and to the right for RTL.

┆Issue is synchronized with this Jira Task

Change performed by the Move to Bugzilla add-on.

Comment 1

[Mike a [:mavduevskiy]]

Solution discussion

Comment 2

[BMO Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1815637 - Add rtl support (#825)

Comment 3

[BMO Github Automation]

Authored by mike a
https://github.com/mozilla-mobile/firefox-android/commit/548379df0165436f3e785c4efa0574d1ff406a02
[main] Bug 1815637 - Add rtl support

Comment 4

[Adina Petridean]

Verified as fixed on Nightly 115.0a1 from 05/10 with Google Pixel 7 PRO (Android 13) and Motorola Moto G9 plus (Android 11). The alignment of text is aligned to the right for RTL.

Comment 5

[Adina Petridean]

Attachment: Motorola.JPEG

Comment 6

[Adina Petridean]

Attachment: GooglePixel.JPEG

Comment 7

[Adina Petridean]

Attachment: GooglePixel2.JPEG

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Did we want to nominate this for v114 uplift?

Comment 9

[Mike a [:mavduevskiy]]

it was a pretty risk update, I think we would like take a bit more time to be sure that we haven't introduced any problems :)

Comment 10

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Revert "Bug 1815637 - Add rtl support" (#2835)

Comment 11

[BMO Github Automation]

Authored by https://github.com/Amejia481
https://github.com/mozilla-mobile/firefox-android/commit/787ad6e5f46d68de09a644cbe7b2c4f8b9e049db
[main] Reverts "Bug 1815637 - Add rtl support."

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 13

[Joe M [:jmahon]]

:mavduevskiy - since this got reverted to fix bug 1839239, should it:

• be revisited to try to fix it in a way that doesn't lead to the regression?
• be left alone for the time being (e.g. applying a P3 priority and coming back to it sometime in the future)?
• Be closed as wontfix?

(based on what you know about this bug, your original fix, and the regression that Arturo addressed)

Comment 14

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Reverts "Bug 1815637 - Add rtl support." (backport #2835) (#2849)

Comment 15

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Reverts "Bug 1815637 - Add rtl support." (backport #2835) (#2850)

Comment 16

[BMO Github Automation]

Authored by https://github.com/mergify[bot]
https://github.com/mozilla-mobile/firefox-android/commit/ec16fbbde11627db5662c7b36898d32ca774120e
[releases_v116] Reverts "Bug 1815637 - Add rtl support." (#2849)

Comment 17

[Dianna Smith [:diannaS]]

This was backed out of 117 nightly & 116 beta in bug 1839239

Comment 18

[Mike a [:mavduevskiy]]

:jmahon

Yes, I think the patch should be iterated with the new info coming from the regression bug.
The regression bug appears to be localized. Most of dialogs are based on AlertDialog which causes no issues, but this particular case was using a TimePickerDialog. I was able to fix the crash by playing around with lifecycle – apparently I was adjusting the button position too early for the time picker.

Comment 19

[BMO Github Automation]

Authored by https://github.com/mergify[bot]
https://github.com/mozilla-mobile/firefox-android/commit/ec7317b07a52bfda53a6ea74c32fffec86bb0589
[releases_v115] Reverts "Bug 1815637 - Add rtl support." (#2850)

Comment 20

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1815637 – RTL support (#3016)

Comment 21

[BMO Github Automation]

Authored by mike a
https://github.com/mozilla-mobile/firefox-android/commit/8fd38cae82759585482a1ae74be7f246ac092839
[main] Bug 1815637 - cherry picking 548379d, the original rtl commit

Authored by mike a
https://github.com/mozilla-mobile/firefox-android/commit/50120cb7fe13c6459799e1740a9141985b946353
[main] Bug 1815637 – fixed the crash that was causing the 1839239 bug

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:mavduevskiy, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.Also, don't forget to request an uplift for the patches in the regression caused by this fix.
• If no, please set status-firefox117 to wontfix.

For more information, please visit BugBot documentation.

Comment 23

[Adina Petridean]

Verified as fixed on Nightly 118.0a1 from 07/08 with Google Pixel 7 PRO (Android 14) and Motorola Moto G9 plus (Android 11). The alignment of text is aligned to the right for RTL.

Comment 24

[Adina Petridean]

Attachment: Screenshot 2023-08-07 at 10.15.11.png

Comment 25

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1815637 – RTL support (backport #3016) (#3268)

Comment 26

[Mike a [:mavduevskiy]]

Comment on attachment 9348928 [details] [review]
[mozilla-mobile/firefox-android] Bug 1815637 – RTL support (backport #3016) (#3268)

Beta/Release Uplift Approval Request

• User impact if declined: RTL languages won't have full support in the App
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We have landed the patch before, and after making it into release it started crashing in a very particular case (older devices, upon user interaction with the webpage content). we could thoroughly – though manually – test that case and make sure it works on a the range of android versions
• String changes made/needed:
• Is Android affected?: Yes

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9348928 [details] [review]
[mozilla-mobile/firefox-android] Bug 1815637 – RTL support  (backport #3016) (#3268)

Approved for 117.0b8.

Comment 28

[BMO Github Automation]

Authored by mike a
https://github.com/mozilla-mobile/firefox-android/commit/a0a2fc0bbbd3831c323e10e483081a4450ece0f8
[releases_v117] Bug 1815637 - cherry picking 548379d, the original rtl commit

Authored by mike a
https://github.com/mozilla-mobile/firefox-android/commit/ef5c2fefc85676e6aab577dadf281b1a2bf5322b
[releases_v117] Bug 1815637 – fixed the crash that was causing the 1839239 bug

Comment 29

[Lorand Janos]

Verified as fixed on the latest Beta build (117.0b8).
The text is aligned to the right side, as expected.

Devices used:

• Google Pixel 7 (Android 14).
• Oppo Find X5 (Android 13).
• Lenovo M10 (Android 10).
• Samsung Galaxy S9 (Android 8).

Marking the ticket as verified for 117 as well.