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

Status: RESOLVED FIXED

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

Description

[Tom Klein]

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

[Tomer Cohen :tomer]

Dupe of 1337897?

Comment 2

[Tomer Cohen :tomer]

(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.

Comment 3

[Tom Klein]

(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.

Comment 4

[Tom Klein]

Attachment: Bug 1337933 - Make the tabs grid layout decorations work with RTL.

Note: the tab item views we're decorating don't report being RTL when in RTL
mode, only the parent RecyclerView does.

This patch affects all grided tabs panel layouts: tablets, phones in landscape
mode, and phones in portrait mode with the "Compact tabs" setting on.

Review commit: https://reviewboard.mozilla.org/r/127424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127424/

Comment 5

[Tom Klein]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127424/diff/1-2/

Comment 6

[Tom Klein]

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

[Max Liu [:maliu]]

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

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

LGTM

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/847803cc7003
Make the tabs grid layout decorations work with RTL. r=maliu

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/847803cc7003

Comment 10

[Itiel]

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.

Comment 11

[Tom Klein]

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.

Comment 12

[Gerry Chang [:gchang]]

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

Fix an RTL issue and was verified. Aurora54+.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7c1dafe56c49