Closed Bug 1337933 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: twointofive, Assigned: twointofive)

References

(Blocks 1 open bug)

Details

(Keywords: rtl)

Attachments

(2 files)

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
Dupe of 1337897?
See Also: → 1337897
(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.
(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.
See Also: → 1337947
QA Contact: ioana.chiorean
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 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+
Assignee: nobody → twointofive
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/847803cc7003
Make the tabs grid layout decorations work with RTL. r=maliu
https://hg.mozilla.org/mozilla-central/rev/847803cc7003
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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)
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+
You need to log in before you can comment on or make changes to this bug.