RTL support for Top Sites Grid View

VERIFIED FIXED in Firefox 53

Status

()

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

People

(Reporter: Niv Yahel, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 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:

With RTL support enabled, the Grid View items overlap with each other, and the text is not aligned on the right side.
I am simply making the items layout stay the same, while making the text right aligned.
(Reporter)

Updated

4 years ago
Blocks: 924699
(Reporter)

Updated

4 years ago
No longer blocks: 924699
(Reporter)

Updated

4 years ago
Depends on: 924699
(Reporter)

Updated

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

Comment 1

4 years ago
Created attachment 814659 [details] [diff] [review]
topSitesGridView_rtl.patch

This appears to fix the issue EXCEPT for titles that are too long to fit in the Grid View Item URL field. Those simply disappear.
Attachment #814659 - Flags: feedback?
(Reporter)

Comment 2

4 years ago
I apologize for all of the comments. I'm still getting used to Bugzilla. I neglected to mention that this was tested on an ASUS Nexus 7 running Android 4.3.
(Reporter)

Updated

4 years ago
OS: Mac OS X → Android
(Reporter)

Updated

4 years ago
Attachment #814659 - Flags: feedback? → review?(sriram)
Comment on attachment 814659 [details] [diff] [review]
topSitesGridView_rtl.patch

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

Looks good overall.

::: mobile/android/base/resources/layout/top_sites_grid_item_view.xml
@@ +20,5 @@
>              android:layout_below="@id/thumbnail"
>              android:duplicateParentState="true"
>              android:drawablePadding="4dip"
> +            gecko:fadeWidth="20dip"
> +            android:layoutDirection="locale"/>

I am fine with this. But please ensure that this doesn't cause a crash in phones running version 8 or so.

::: mobile/android/base/resources/values/styles.xml
@@ +151,5 @@
>        <item name="android:textSize">12sp</item>
>        <item name="android:singleLine">true</item>
>        <item name="android:ellipsize">none</item>
>        <item name="android:paddingTop">5dip</item>
> +      <item name="android:textAlignment">viewStart</item>

Same here. Please ensure this doesn't cause a trouble in a phone running API level 8.
Attachment #814659 - Flags: review?(sriram) → review+
(Reporter)

Comment 4

4 years ago
Created attachment 830585 [details] [diff] [review]
topSitesGridView_rtl.patch

I will look into the cases you mentioned for the other patch. This patch contains the rotationY attribute I was talking about. My flipping both the Grid View and the Grid Item View, the grid gets populated from the right side, which is more natural to a RTL user.
Attachment #830585 - Flags: review?(sriram)
Comment on attachment 830585 [details] [diff] [review]
topSitesGridView_rtl.patch

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

Mostly looks good. I am confused about the padding part. Will give my review based on that.
Otherwise the rotation of 180 sounds fine with me. Android uses display lists and this is not a problem.

Also, please have your check-in comment in following format.

Bug 924700: Adding RTL support to TopSitesGridView. (r=sriram)

::: mobile/android/base/resources/values-large-v11/styles.xml
@@ -70,5 @@
>      </style>
>  
>      <style name="Widget.TopSitesGridView" parent="Widget.GridView">
> -        <item name="android:paddingLeft">5dp</item>
> -        <item name="android:paddingRight">5dp</item>

Why is this padding removed?

::: mobile/android/base/resources/values-ldrtl-large/styles.xml
@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<resources>
> +
> +    <style name="Widget.TopSitesGridView" parent="Widget.GridView">

Don't we need padding in left and right?

::: mobile/android/base/resources/values-ldrtl/styles.xml
@@ +15,5 @@
> +
> +    <style name="Widget.TopSitesGridItemView">
> +      <item name="android:layout_width">fill_parent</item>
> +      <item name="android:layout_height">fill_parent</item>
> +      <item name="android:padding">5dip</item>

Did you move the padding from the GridView to its item? I find it a bit wrong.
There was supposed to be some edge space outside the items.

Updated

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

Updated

7 months ago
QA Contact: ioana.chiorean

Updated

6 months ago
Blocks: 702845
Component: General → Theme and Visual Design
Hardware: x86 → Unspecified
Version: 27 Branch → unspecified

Comment 6

6 months ago
The remaining issue for this bug is that the grid items' text does not support BiDi.

Updated

6 months ago
Duplicate of this bug: 924703

Comment 8

5 months ago
(In reply to ItielMaN from comment #6)
> The remaining issue for this bug is that the grid items' text does not
> support BiDi.

Seems to be fixed in latest Nightly.
Closing as RESOLVED FIXED.
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED

Comment 9

4 months ago
This was implemented and works correctly - further bugs that might arise will be linked for tracking here.
Status: RESOLVED → VERIFIED
Target Milestone: --- → Firefox 53

Updated

4 months ago
Depends on: 1331947
Blocks: 1319302
You need to log in before you can comment on or make changes to this bug.