Closed Bug 928663 Opened 11 years ago Closed 7 years ago

RTL Support for Menu/Settings

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect, P1)

Unspecified
Android
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: icatchcows, Unassigned)

References

Details

Attachments

(2 files, 6 obsolete files)

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.
Blocks: rtl-meta
OS: Mac OS X → Android
Depends on: 702959, 924418, 924699, 925108, 927667
Attached patch bug-928663-fix.patch (obsolete) — Splinter Review
Attached patch bug-928663-fix.patch (obsolete) — Splinter Review
Attached patch rtlMenu.patch (obsolete) — Splinter Review
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-
Attached patch rtlMenu_take2.patch (obsolete) — Splinter Review
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)
Attached patch rtlMenu_take3.patch (obsolete) — Splinter Review
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+
Attached patch rtlMenu_take3.patch (obsolete) — Splinter Review
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)
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+
Status: UNCONFIRMED → NEW
Ever confirmed: true
No longer depends on: 702959
No longer depends on: 924418
QA Contact: ioana.chiorean
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
(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)
Hardware: x86 → Unspecified
Version: 27 Branch → unspecified
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)
(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)
See attached.
You can see the 2 highlighted sentences are LTR. The "Nightly" words should be located at the rightmost of the sentence.
Attached image 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)
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)
(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)
Depends on: 1323765
Depends on: 1323763
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
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Depends on: 1333089
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.