[RTL] Tabs grid layout item decoration paddings are reversed in RTL

RESOLVED FIXED in Firefox 54

Status

()

Firefox for Android
Theme and Visual Design
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: Tom Klein, Assigned: Tom Klein)

Tracking

(Blocks: 1 bug, {rtl})

Trunk
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

4 months ago
Created attachment 8835081 [details]
Top is RTL, bottom is LTR

The item decorations on tabs in the grid view have different left and right paddings, dependent on their position in a given row.  It looks like those paddings are reversed in RTL mode.  This affects tablets, phones in landscape mode, and phones in portrait mode using the "compact tabs" setting.

I think we need to keep the tabs layout RecyclerView in RTL mode in order to get the tabs to layout in the correct order, but the padding on those tabs shouldn't have its sense of left and right switched since it really is checking something like "are we the leftmost tab in a row? then don't put any padding to our left" (it's a little more complicated than that in general[1]).  If there's no way to turn off RTL just for those decorations then I guess we need to add a check for RTL in the ItemDecoration itself?

(By the way I'm testing by switching the Android language setting to an RTL language - please let me know if that's not a sufficiently correct way to do it.)

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/widget/GridSpacingDecoration.java#43-46

Comment 1

4 months ago
Dupe of 1337897?
See Also: → bug 1337897

Comment 2

4 months ago
(In reply to Tomer Cohen :tomer from comment #1)
> Dupe of 1337897?
Not fully duplicate, as the other bug is only about having Compact Tabs enabled in Portrait Mode, while this issue remove the the dependency on Compact Tabs on landscape mode, and having it on portrait mode while Compact Tabs is enabled.
(Assignee)

Comment 3

4 months ago
(In reply to Tomer Cohen :tomer from comment #1)
> Dupe of 1337897?

I think they'll actually be fixed in different places in different ways, so I think they're separate issues.  This bug is (basically) about the distance between the thumbnails (I should have made that clearer), which is a runtime-computed value (computed at [1] in comment 0).  Bug 1337897 is about the distance between the close button and the tab title, which is set in xml layout, if I'm not mistaken.
(Assignee)

Updated

4 months ago
See Also: → bug 1337947

Updated

2 months ago
QA Contact: ioana.chiorean
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 months ago
I had what was RTL and not RTL reversed in my head in the first version of the patch, which gave the same result, but hopefully the second version makes more sense :)

Comment 7

2 months ago
mozreview-review
Comment on attachment 8855581 [details]
Bug 1337933 - Make the tabs grid layout decorations work with RTL.

https://reviewboard.mozilla.org/r/127424/#review130206

LGTM
Attachment #8855581 - Flags: review?(max) → review+

Updated

2 months ago
Assignee: nobody → twointofive

Comment 8

2 months ago
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/847803cc7003
Make the tabs grid layout decorations work with RTL. r=maliu

Comment 9

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/847803cc7003
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 10

2 months ago
Confirmed fixed in latest Nightly on Samsung Galaxy S5.
If someone can verify this also on a tablet, I'd suggest uplifting it to beta.
Flags: needinfo?(twointofive)
(Assignee)

Comment 11

a month ago
Comment on attachment 8855581 [details]
Bug 1337933 - Make the tabs grid layout decorations work with RTL.

Approval Request Comment
[Feature/Bug causing the regression]: RTL on android
[User impact if declined]: Spacing between tabs in the tabs trays that show tabs in a grid will be off (see the attachment for an example) - tablets, phones in landscape mode, and phones in portrait mode with "Compact tabs" on are affected.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Take a screenshot of the tabs tray on a phone with landscape orientation in LTR and RTL modes and compare the spacing between the tabs: it should be the same in LTR and RTL.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: For RTL mode we're just reusing the spacing calculations done in LTR and taking into account the way position indices change when in RTL mode.
[String changes made/needed]: None.
Flags: needinfo?(twointofive)
Attachment #8855581 - Flags: approval-mozilla-aurora?
Comment on attachment 8855581 [details]
Bug 1337933 - Make the tabs grid layout decorations work with RTL.

Fix an RTL issue and was verified. Aurora54+.
Attachment #8855581 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 13

a month ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c1dafe56c49
status-firefox54: affected → fixed
You need to log in before you can comment on or make changes to this bug.