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)
Firefox for Android Graveyard
Theme and Visual Design
Unspecified
Android
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: icatchcows, Unassigned)
References
Details
Attachments
(2 files, 6 obsolete files)
2.51 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
95.37 KB,
image/png
|
Details |
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.
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 4•11 years ago
|
||
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-
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)
Attachment #831877 -
Attachment is obsolete: true
Attachment #831877 -
Flags: review?(sramasubramanian)
Attachment #8335246 -
Flags: review?(sramasubramanian)
Comment 7•11 years ago
|
||
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+
Requested by Lauren to remove the changes to the manifest since another patch already takes care of that.
Attachment #8335386 -
Flags: review?(sramasubramanian)
Comment 9•11 years ago
|
||
(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 10•11 years ago
|
||
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•11 years ago
|
||
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 12•11 years ago
|
||
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•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Updated•8 years ago
|
QA Contact: ioana.chiorean
Comment 13•7 years 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)
Comment 14•7 years ago
|
||
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)
Comment 15•7 years ago
|
||
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•7 years 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)
Comment 17•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Comment 21•7 years ago
|
||
(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•7 years 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•7 years 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)
Comment 24•7 years 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
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•