RTL Support for Menu/Settings

VERIFIED FIXED

Status

()

Firefox for Android
Theme and Visual Design
P1
normal
VERIFIED FIXED
4 years ago
4 months ago

People

(Reporter: Danielle, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.116 Safari/537.36

Steps to reproduce:

RTL support for menu/settings. Blocks bug 702845.
(Reporter)

Updated

4 years ago
Blocks: 702845
OS: Mac OS X → Android
(Reporter)

Updated

4 years ago
Depends on: 702959, 924418, 924699, 925108, 927667
(Reporter)

Comment 1

4 years ago
Created attachment 819393 [details] [diff] [review]
bug-928663-fix.patch
(Reporter)

Comment 2

4 years ago
Created attachment 819394 [details] [diff] [review]
bug-928663-fix.patch
(Reporter)

Comment 3

4 years ago
Created attachment 831824 [details] [diff] [review]
rtlMenu.patch

The patch includes the RTL menu settings. To test, the menu should flip when the system language is set to hebrew or another RTL language.
Attachment #819393 - Attachment is obsolete: true
Attachment #819394 - Attachment is obsolete: true
Attachment #831824 - Flags: review?(sramasubramanian)
Comment on attachment 831824 [details] [diff] [review]
rtlMenu.patch

Review of attachment 831824 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good. I would like to see another version addressing the comments.

And for the checkin comment, it is good to use the bug number and the reviewer name.

Bug 928663: Changes to android menu to match RTL standards. (r=sriram)

For more examples: https://tbpl.mozilla.org/?tree=Fx-Team

::: mobile/android/base/menu/MenuItemDefault.java
@@ +27,5 @@
>      private boolean mChecked = false;
>      private boolean mHasSubMenu = false;
>  
> +    private boolean rtl = true;
> +

Is there a better way to get this? We can't hard code this in code.
And, it's better to name it mRTL or mRtl. (Using "m" for private).

On a second thought, you might not need this at all. This is not being used.

@@ +93,5 @@
>          if (mIcon != null) {
>              mIcon.setBounds(sIconBounds);
>              mIcon.setAlpha(isEnabled() ? 255 : 99);
>          }
> +        

White space. Remove this please.

@@ +94,5 @@
>              mIcon.setBounds(sIconBounds);
>              mIcon.setAlpha(isEnabled() ? 255 : 99);
>          }
> +        
> +        //Will change the order of the elements if rtl locale

"// This will change the order of icons based on locale."

@@ +100,3 @@
>  
> +        //Original
> +        //setCompoundDrawables(mIcon, null, mState, null);

You can remove this code.

::: mobile/android/base/resources/layout/menu_item_action_view.xml
@@ +12,5 @@
>              android:layout_weight="1.0"
>              android:background="@drawable/action_bar_button"
>              android:clickable="true"
> +            android:focusable="true"
> +            android:layoutDirection="rtl"/>

I am not sure about using "layoutDirection" in a layout that will be used in v11. Could you test it in an ICS device and make sure that this doesn't break?

@@ +22,5 @@
>                   android:paddingBottom="8dip"
>                   android:scaleType="centerInside"
>                   android:background="@drawable/action_bar_button"
>                   android:layout_gravity="center_vertical"
> +                 android:visibility="gone"  />

Extra spaces here. Not needed.
Attachment #831824 - Flags: review?(sramasubramanian) → review-
(Reporter)

Comment 5

4 years ago
Created attachment 831877 [details] [diff] [review]
rtlMenu_take2.patch

This is the second version of the patch with the requested changes for the RTL version of the menu.
Attachment #831824 - Attachment is obsolete: true
Attachment #831877 - Flags: review?(sramasubramanian)
(Reporter)

Comment 6

4 years ago
Created attachment 8335246 [details] [diff] [review]
rtlMenu_take3.patch
Attachment #831877 - Attachment is obsolete: true
Attachment #831877 - Flags: review?(sramasubramanian)
Attachment #8335246 - Flags: review?(sramasubramanian)
Comment on attachment 8335246 [details] [diff] [review]
rtlMenu_take3.patch

Review of attachment 8335246 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I leave it to mfinkle for bumping the API level.
Attachment #8335246 - Flags: review?(sramasubramanian)
Attachment #8335246 - Flags: review?(mark.finkle)
Attachment #8335246 - Flags: review+
(Reporter)

Comment 8

4 years ago
Created attachment 8335386 [details] [diff] [review]
rtlMenu_take3.patch

Requested by Lauren to remove the changes to the manifest since another patch already takes care of that.
Attachment #8335386 - Flags: review?(sramasubramanian)
(In reply to Danielle from comment #8)
> Created attachment 8335386 [details] [diff] [review]
> rtlMenu_take3.patch
> 
> Requested by Lauren to remove the changes to the manifest since another
> patch already takes care of that.

It's the same patch still ;)
Comment on attachment 8335246 [details] [diff] [review]
rtlMenu_take3.patch

Bug 924418 already makes the AndroidManifest.xml changes, so you can remove them from this patch and post a new one.
Attachment #8335246 - Flags: review?(mark.finkle)
(Reporter)

Comment 11

4 years ago
Created attachment 8336963 [details] [diff] [review]
rtlMenu_take3.patch

Removed the android manifest code.
Attachment #8335246 - Attachment is obsolete: true
Attachment #8335386 - Attachment is obsolete: true
Attachment #8335386 - Flags: review?(sramasubramanian)
Attachment #8336963 - Flags: review?(mark.finkle)
Comment on attachment 8336963 [details] [diff] [review]
rtlMenu_take3.patch

This can't land until the dependent patches in other bugs land first.
Attachment #8336963 - Flags: review?(mark.finkle) → review+

Updated

3 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
No longer depends on: 702959
No longer depends on: 924418
Priority: -- → P1

Updated

9 months ago
QA Contact: ioana.chiorean

Comment 13

6 months ago
Overall it looks like it is fixed in latest Nightly builds, however there's a minor issue where strings starting with an english word makes the string appear as LTR. A BiDi issue.
We could possibly fix it by adding RTL markers to the specific problematic strings.

(An example for such a string is "Nightly will tell sites that you do not want to be tracked" under Settings > Privacy)
Hi Max, could you please take a look of the comment13 ?

Do we have any better idea to handle this ?  or we are fine with the current solution with your patch ?

Thank you very much !!
Flags: needinfo?(max)
Hi all,

Tested and verified RTL support on latest Nightly (Arabic) and the results are here:
Phone:
- LG G4 (Android 5.1) - https://i.imgur.com/DJ83rqO.png
- Lenovo A536 (Android 4.4.2) - https://i.imgur.com/80cUiwN.png
Tablet:
- Asus ZenPad 8 (Android 6.0.1) - https://i.imgur.com/SRLQFBS.png

Comment 16

6 months ago
(In reply to Sorina Florean [:sorina] from comment #15)
> Hi all,
> 
> Tested and verified RTL support on latest Nightly (Arabic) and the results
> are here:
> Phone:
> - LG G4 (Android 5.1) - https://i.imgur.com/DJ83rqO.png
> - Lenovo A536 (Android 4.4.2) - https://i.imgur.com/80cUiwN.png
> Tablet:
> - Asus ZenPad 8 (Android 6.0.1) - https://i.imgur.com/SRLQFBS.png

A few issues in your screenshots Sorina:
1. The back arrow in the screenshot for LG G4 is pointing left instead of right
2. In the screenshot for Lenovo A536, the back, forward and refresh buttons are mirrored
3. In the screenshot for Asus ZenPad 8, the forward button overlaps the search icon (will point that out in another bug)

Can you please check if issues #1+#2 exist in your other devices as well?
Thank you.
Flags: needinfo?(sorina.florean)

Updated

6 months ago
Hardware: x86 → Unspecified
Version: 27 Branch → unspecified

Comment 17

6 months ago
Hi ItielMaN,

Just to clarify, when the system locale is in RTL language, do we expect non-translated string in English still right to left?
Flags: needinfo?(max) → needinfo?(itiel_yn8)

Comment 18

6 months ago
(In reply to Max Liu [:maliu] from comment #17)
> Hi ItielMaN,
> 
> Just to clarify, when the system locale is in RTL language, do we expect
> non-translated string in English still right to left?

Are you referring to comment 13?
If so, not quite. Strings such as the one I've mentioned before starts with an English word, and so its translation.
In such case, firefox will account for that string bidirectionality as LTR, and it'll be aligned to the left, even though the string is translated to a RTL language.
In short, every string in the settings page (except for "learn more" and "show my health report") should be aligned to the right.
If that's not clear enough I'll attach a screenshot explaining the issue.
Flags: needinfo?(itiel_yn8)

Comment 19

6 months ago
See attached.
You can see the 2 highlighted sentences are LTR. The "Nightly" words should be located at the rightmost of the sentence.

Comment 20

6 months ago
Created attachment 8816750 [details]
Screenshot
(In reply to ItielMaN from comment #16)
> (In reply to Sorina Florean [:sorina] from comment #15)
> > Hi all,
> > 
> > Tested and verified RTL support on latest Nightly (Arabic) and the results
> > are here:
> > Phone:
> > - LG G4 (Android 5.1) - https://i.imgur.com/DJ83rqO.png
> > - Lenovo A536 (Android 4.4.2) - https://i.imgur.com/80cUiwN.png
> > Tablet:
> > - Asus ZenPad 8 (Android 6.0.1) - https://i.imgur.com/SRLQFBS.png
> 
> A few issues in your screenshots Sorina:
> 1. The back arrow in the screenshot for LG G4 is pointing left instead of
> right
Yes, the issue is reproducible on LG G4 (Android 5.1) and Huawei Honor 5x(Android 5.1.1) but isn't reproducible for LG G4 (Android 6.0), Nexus 9 (Android 7.0), Asus ZenPad 8 (Android 6.0.1) and Lenovo A536 (Android 4.4.2).

> 2. In the screenshot for Lenovo A536, the back, forward and refresh buttons
> are mirrored
Reproducible also with LG G4 (Android 5.1) and Huawei Honor 5x(Android 5.1.1).

> 3. In the screenshot for Asus ZenPad 8, the forward button overlaps the
> search icon (will point that out in another bug)
Indeed, reproducible on Asus ZenPad 8 but not on Nexus 9 (Android 7.0).
> 
> Can you please check if issues #1+#2 exist in your other devices as well?
> Thank you.

Should I file another bugs for the issues found? I notice also that:
1. Progress bar is working from right to left only on Nexus 9 (Android 7.0) from devices that I have to test.
2. Text typed into the search field is left-aligned not right-aligned (reproducible on all devices - https://i.imgur.com/edjgQd9.png).
Flags: needinfo?(sorina.florean) → needinfo?(itiel_yn8)

Comment 22

6 months ago
You sure have a lot of phones :)

(In reply to Sorina Florean [:sorina] from comment #21)
> Should I file another bugs for the issues found?
I think Max should be the one to decide about that.

> 1. Progress bar is working from right to left only on Nexus 9 (Android 7.0)
> from devices that I have to test.
Yeah, noticed that also on Samsung Galaxy S5 (Android 6.0.1).
Leave that to me. I'll file a bug about that.

> 2. Text typed into the search field is left-aligned not right-aligned
> (reproducible on all devices - https://i.imgur.com/edjgQd9.png).
That should be covered in bug 928688.

Thanks for testing!
Flags: needinfo?(itiel_yn8) → needinfo?(max)

Comment 23

6 months ago
(In reply to Sorina Florean [:sorina] from comment #21)
> Should I file another bugs for the issues found? I notice also that:
Yes. Please file another bug and indicates that the symptom can only be reproduced on specific brand/platform.
Flags: needinfo?(max)

Updated

6 months ago
Depends on: 1323765

Updated

6 months ago
Depends on: 1323763

Comment 24

4 months ago
As all side effects are treated in different bugs we can close this one. Anything arising after will be linked though for tracking.
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED

Updated

4 months ago
Status: RESOLVED → VERIFIED
Blocks: 1319302
Depends on: 1333089
You need to log in before you can comment on or make changes to this bug.