928688 - RTL support for URL toolbar

Status: VERIFIED FIXED

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

Description

[u482316]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/536.30.1 (KHTML, like Gecko) Version/6.0.5 Safari/536.30.1

Steps to reproduce:

Fixing the URL toolbar to support RTL interface.

Comment 1

[Andre Klapper]

Hi, unfortunately this report is not very useful because it does not describe the problem well. If you have time and can still reproduce the problem, please read https://developer.mozilla.org/en/Bug_writing_guidelines and add a more useful description to this report by also providing a testcase why you think that there is no support, and clear and exact steps to reproduce the problem.

Comment 2

[Aaron Train [:aaronmt]]

I believe this is part of a on-going project, it just needs to mark the dependency on the other open bugs and such

Comment 3

[u482316]

Attachment: Work in progress

This will right-align the text and icons in the url bar for both tablet and mobile devices. Still need to re-position the buttons on the browser toolbar for mobile.

Comment 4

[u482316]

Attachment: bug-928688-fix.patch

Comment 5

[u482316]

bug-928688-fix.patch: completed.

What was added:
- New resource files were added for both the tablet and mobile view of the browser toolbar. 
- Mobile version of the browser toolbar is re-desgined for right-to-left languages with the URL bar placed between the tabs (left of it) and the menu (right of it). 
- The left and right edge of the Url Bar is animated to expand in both direction. 
- Previous menu icon on the mobile browser toolbar was replaced with a different drawable resource to make the three dots less transparent. Icon does not use Tab's background anymore.
- The bug dealing with the cursor splitting in half is fixed.

Comment 6

[u482316]

Attachment: browser_toolbar.patch (Changed according to the new update of the source code)

Updated and newer version of the patch since the previous patch was incompatible with the current update.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8339070 [details] [diff] [review]
browser_toolbar.patch (Changed according to the new update of the source code)

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

This is looking good overall. I'd like us to experiment with a solution based on multiple styles (as opposed to multiple layout files).

::: mobile/android/base/CustomEditText.java
@@ +19,5 @@
>  
>      public CustomEditText(Context context, AttributeSet attrs) {
>          super(context, attrs);
>          mOnKeyPreImeListener = null;
> +        setTextAlignment(TEXT_ALIGNMENT_VIEW_START);

You have to enclose setTextAlignment() calls with a version check because this method is not available on pre-17 Android. Something like:

if (Build.VERSION.SDK_INT >= 17) {
   setTextAlignment(...);
}

::: mobile/android/base/resources/layout-ldrtl-large-v11/browser_toolbar.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="utf-8"?>

I guess this file should actually in layout-ldrtl-large-v17 instead as there's no RTL support before Android 17, right?

::: mobile/android/base/resources/layout-ldrtl/browser_toolbar.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="utf-8"?>

Instead of duplicating the entire layout file for the RTL versions of the toolbar, it would probably be worth investigating a solution based same layout files using different styles instead. I'm not entirely sure how feasible this is but I'd like us to try this route before adding more copies of browser_toolbar.xml.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +273,5 @@
> +        // This will clip the left edge's image at 60% of its width
> +        mUrlBarLeftEdge = (ImageView) findViewById(R.id.url_bar_left_edge);
> +        if (mUrlBarLeftEdge != null) {
> +            mUrlBarLeftEdge.getDrawable().setLevel(6000);
> +        }

Instead of having separate views for left and right edge, maybe try to do it using the same view but different styles?

@@ +325,5 @@
> +        //direction if RTL is used instead
> +        mDirection = 0;
> +        if(Build.VERSION.SDK_INT >= 17){
> +            Locale currentLocale = getResources().getConfiguration().locale;
> +            mDirection = TextUtils.getLayoutDirectionFromLocale(currentLocale);

We should probably have this code in a util class like LocaleUtils with static isRtl() method. Have a look at other classes in our util package.

@@ +1216,5 @@
> +
> +        if(mDirection == 0){
> +            animator.attach(mTabs,
> +                            PropertyAnimator.Property.TRANSLATION_X,
> +                            curveTranslation);

You can probably avoid duplication here by simply doing:

curveTranslation * (direction == 0 ? 1 : -1) in all attach calls here.

Comment 8

[Rachelle Yang [:ryang][[e-mail]]]

Hi Ioana and Sorina ,

would you please help us verify this RTL bug on Fennec after Bug 1319302 is already resolved fixed ? 

Thank you very much !

Comment 9

[Arash Mousavi [:Arash-M]]

Attachment: toolbar-nightly_2016-11-29.png

Hi,

As per my comment Bug 1319302, I provide this screenshot. Currently when both back/forward buttons are visible the lock icon (for SSL pages) overlaps the forward button makes it impossible to use.

Also back/forward icons should be mirrored as mentioned on the RTL spec page 3[1]

The provided screenshot is taken on an Android tablet using version 4.4.2 with the locale set to Persian (for whole device and FF). Please let me know if more information is needed.

[1] https://bug702845.bmoattachments.org/attachment.cgi?id=8787324

Comment 10

[Khaled Hosny]

Looks fine on phone, but there is no back button there so can’t confirm comment 9.

Comment 11

[Khaled Hosny]

Actually I do have a tablet, and I can confirm the bug above.

Comment 12

[Itiel]

Following Arash's comment 9, a few more issues:
1. According to Sorina's screenshot from Asus ZenPad 8 Android 6.0.1 from bug 928663 comment 15 (https://imgur.com/SRLQFBS), the search icon also overlaps the forward button (possibly when going from a New Tab to another page, and then going back to the New Tab?)
2. According to comment 9's attachment 8816015 [details], also the refresh button is mirrored
3. The entire text in the URL bar should be RTL'd (to match Firefox for desktop's URL bar)

Comment 13

[Sorina Florean [:sflorean]]

(In reply to ItielMaN from comment #12)
> Following Arash's comment 9, a few more issues:
> 1. According to Sorina's screenshot from Asus ZenPad 8 Android 6.0.1 from
> bug 928663 comment 15 (https://imgur.com/SRLQFBS), the search icon also
> overlaps the forward button (possibly when going from a New Tab to another
> page, and then going back to the New Tab?)
Steps to reproduce:
- Go to a page;
- Tap on Firefox back button.
> 2. According to comment 9's attachment 8816015 [details], also the refresh
> button is mirrored
> 3. The entire text in the URL bar should be RTL'd (to match Firefox for
> desktop's URL bar)

Comment 14

[Itiel]

(In reply to ItielMaN from comment #12)
> 3. The entire text in the URL bar should be RTL'd (to match Firefox for
> desktop's URL bar)

This part seems to be fixed in latest Nightly build.

Comment 15

[Ioana Chiorean]

This is verified as fixed on Samsung Galaxy Tab 2 with Android  6.0.1
AR Fennec 53.0a1 2017-01-18

Comment 16

[Itiel]

Arash, are you still able to reproduce your issues from comment 9? So I'd know if to open a dedicated bug for that.

Comment 17

[Arash Mousavi [:Arash-M]]

(In reply to ItielMaN from comment #16)
> Arash, are you still able to reproduce your issues from comment 9? So I'd
> know if to open a dedicated bug for that.

I just checked with the latest Nightly and it doesn't happen anymore. The lock icon now slides neatly to left and makes space for the forward button.