RTL support for URL toolbar

VERIFIED FIXED

Status

()

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

People

(Reporter: u482316, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 years ago
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.
Flags: needinfo?(amybanh1)
I believe this is part of a on-going project, it just needs to mark the dependency on the other open bugs and such
(Reporter)

Updated

4 years ago
Blocks: 702845
Depends on: 924418
Flags: needinfo?(amybanh1)
OS: Mac OS X → Android
(Reporter)

Comment 3

4 years ago
Created attachment 824867 [details] [diff] [review]
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.
(Reporter)

Comment 4

4 years ago
Created attachment 8334980 [details] [diff] [review]
bug-928688-fix.patch
Attachment #824867 - Attachment is obsolete: true
(Reporter)

Comment 5

4 years ago
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.
(Reporter)

Comment 6

4 years ago
Created attachment 8339070 [details] [diff] [review]
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.
Attachment #8334980 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #8339070 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8339070 - Flags: review?(lucasr.at.mozilla) → feedback+

Updated

3 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
No longer depends on: 924418
Priority: -- → P2
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 !
Flags: needinfo?(sorina.florean)
Flags: needinfo?(ioana.chiorean)
Created attachment 8816015 [details]
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

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

Comment 11

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

Comment 12

6 months ago
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)

Updated

6 months ago
Hardware: x86 → Unspecified
Version: 27 Branch → unspecified
(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)

Updated

6 months ago
Depends on: 1322119

Updated

6 months ago
Depends on: 1322144

Comment 14

5 months ago
(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

4 months ago
This is verified as fixed on Samsung Galaxy Tab 2 with Android  6.0.1
AR Fennec 53.0a1 2017-01-18
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Flags: needinfo?(sorina.florean)
Flags: needinfo?(ioana.chiorean)
QA Contact: ioana.chiorean
Resolution: --- → FIXED

Updated

4 months ago
Status: RESOLVED → VERIFIED

Comment 16

4 months ago
Arash, are you still able to reproduce your issues from comment 9? So I'd know if to open a dedicated bug for that.
Flags: needinfo?(mousavi.arash)
(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.
Flags: needinfo?(mousavi.arash)
Blocks: 1319302
You need to log in before you can comment on or make changes to this bug.