Closed Bug 924703 Opened 11 years ago Closed 8 years ago

RTL Support for Two Line Page Rows

Categories

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

Unspecified
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 924700

People

(Reporter: nivivon, Unassigned)

References

Details

Attachments

(4 files)

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.
Blocks: 924699
Depends on: 924418
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?
This is simply to illustrate #2, which isn't fixed by #1.
OS: Mac OS X → Android
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 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-
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)
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+
Status: UNCONFIRMED → NEW
Ever confirmed: true
No longer depends on: 924418
Blocks: rtl-meta
Hardware: x86 → Unspecified
Version: 27 Branch → unspecified
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
Closed: 8 years ago
Resolution: --- → DUPLICATE
QA Contact: ioana.chiorean
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: