924703 - RTL Support for Two Line Page Rows

Status: RESOLVED DUPLICATE of bug 924700

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

Description

[Niv Yahel]

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.

Comment 1

[Niv Yahel]

Attachment: 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.

Comment 2

[Niv Yahel]

Attachment: Screen Shot 2013-10-08 at 8.52.23 PM.png

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

Comment 3

[Lauren]

Attachment: 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

[Lauren]

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!

Comment 5

[Lucas Rocha (:lucasr)]

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.

Comment 6

[Lauren]

Attachment: 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...

Comment 7

[Lucas Rocha (:lucasr)]

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.

Comment 8

[Itiel]

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.