RTL Support for Two Line Page Rows

RESOLVED DUPLICATE of bug 924700

Status

()

Firefox for Android
Theme and Visual Design
RESOLVED DUPLICATE of bug 924700
4 years ago
2 months ago

People

(Reporter: Niv Yahel, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

When using the native RTL support, two line page rows work for the most part. However, there are two elements that do not align as they should:
1. The URL text is still left-aligned.
2. If there is an icon, it remains on the most right hand side of the text field.
(Reporter)

Updated

4 years ago
Blocks: 924699
Depends on: 924418
(Reporter)

Comment 1

4 years ago
Created attachment 814660 [details] [diff] [review]
twoLinePageRow_rtl.patch

This fixes #1 (the URL being left-aligned). However, the text begins immediately to the left of the star, which appears in the same place.
Attachment #814660 - Flags: feedback?
(Reporter)

Comment 2

4 years ago
Created attachment 814661 [details]
Screen Shot 2013-10-08 at 8.52.23 PM.png

This is simply to illustrate #2, which isn't fixed by #1.
(Reporter)

Updated

4 years ago
OS: Mac OS X → Android

Comment 3

4 years ago
Created attachment 819515 [details] [diff] [review]
twoLinePageRow_rtl.patch

This updated patch fixes issue #2. There is still an issue with extremely long website titles in a RTL language just showing up as blank. Perhaps caused by FadedTextView? will look into it

Comment 4

4 years ago
Comment on attachment 819515 [details] [diff] [review]
twoLinePageRow_rtl.patch

You can see an example of this in an apk linked to on bug 702845!
Attachment #819515 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 819515 [details] [diff] [review]
twoLinePageRow_rtl.patch

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

Needs safety checks for pre-17 Android.

::: mobile/android/base/home/TwoLinePageRow.java
@@ +114,5 @@
>              return;
>          }
>  
>          mUrlIconId = urlIconId;
> +        mUrl.setCompoundDrawablesRelativeWithIntrinsicBounds(mUrlIconId, 0, mBookmarkIconId, 0);

setCompoundDrawablesRelativeWithIntrinsicBounds() is only available on Android >= 17. You have to do a version check here:

if (Build.VERSION.SDK_INT >= 17) {
    ...setCompoundDrawablesRelativeWithIntrinsicBounds(...)
} else {
    ...setCompoundDrawablesWithIntrinsicBounds(...)
}

@@ +131,5 @@
>              return;
>          }
>  
>          mBookmarkIconId = bookmarkIconId;
> +        mUrl.setCompoundDrawablesRelativeWithIntrinsicBounds(mUrlIconId, 0, mBookmarkIconId, 0);

Ditto.
Attachment #819515 - Flags: review?(lucasr.at.mozilla) → review-

Comment 6

4 years ago
Created attachment 8342892 [details] [diff] [review]
RTL support for TwoLinePageRow

Good call, fixed that up here. I guess I forgot to check for backwards compatibility on some of these patches, haha, sorry about that...
Attachment #8342892 - Flags: review?(lucasr.at.mozilla)

Updated

4 years ago
Attachment #8342892 - Attachment is patch: true
Comment on attachment 8342892 [details] [diff] [review]
RTL support for TwoLinePageRow

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

Yep.

::: mobile/android/base/home/TwoLinePageRow.java
@@ +152,5 @@
>              return;
>          }
>  
>          mBookmarkIconId = bookmarkIconId;
> +        

nit: remove trailing space here.
Attachment #8342892 - Flags: review?(lucasr.at.mozilla) → review+

Updated

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

Updated

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

Comment 8

6 months ago
Closing as RESOLVED DUPLICATE as the only remaining issue for this bug is the BiDi for the grid items' text, and this will be taken care of in bug 924700.
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 924700

Updated

2 months ago
QA Contact: ioana.chiorean
You need to log in before you can comment on or make changes to this bug.