1337947 - [RTL] Dividers and padding are reversed in RTL in the tab strip on tablets

Status: VERIFIED FIXED

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

Description

[Tom Klein]

Attachment: Top: missing padding to the right; bottom: missing divider to the left

This bug has a similar underlying cause to bug 1337933.

The two screen shots here are both from a tablet with the tab strip scrolled all the way to the right.

In this case, the ItemDecoration for the tab strip RecyclerView draws a divider to the left of each tab[1], and also adjusts the padding of the leftmost and rightmost tabs in order to view the full tab[2].  In RTL mode the notions of left and right are reversed, so we do the wrong thing.

Issue [1] is visible in the bottom screenshot: the rightmost visible tab here doesn't get a divider to its left because it thinks it's the first (leftmost(!)) tab and therefore doesn't need a divider to its left.

Issue [2] is visible in the top screenshot where we've added extra padding to the left of the rightmost tab because we think it's actually the leftmost tab, which makes the right side of that tab cut off because it didn't get that extra padding (I think that's what's happening, I haven't actually checked).

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/tabs/TabStripDividerItem.java#81-86
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/tabs/TabStripDividerItem.java#66-69

Comment 1

[Tom Klein]

Attachment: Bug 1337947 - Fix tab strip divider drawing for RTL.

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

Comment 2

[Tom Klein]

Attachment: Bug 1337947 - Fix tab strip item offsets for RTL.

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

Comment 3

[Max Liu [:maliu]]

Comment on attachment 8856526 [details]
Bug 1337947 - Fix tab strip divider drawing for RTL.

https://reviewboard.mozilla.org/r/128476/#review131200

LGTM

Comment 4

[Max Liu [:maliu]]

Comment on attachment 8856527 [details]
Bug 1337947 - Fix tab strip item offsets for RTL.

https://reviewboard.mozilla.org/r/128478/#review131230

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/169ddc500c53
Fix tab strip divider drawing for RTL. r=maliu
https://hg.mozilla.org/integration/autoland/rev/d41d243d363b
Fix tab strip item offsets for RTL. r=maliu

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/169ddc500c53
https://hg.mozilla.org/mozilla-central/rev/d41d243d363b

Comment 7

[Tom Klein]

Comment on attachment 8856526 [details]
Bug 1337947 - Fix tab strip divider drawing for RTL.

Approval Request Comment
[Feature/Bug causing the regression]: RTL for fennec
[User impact if declined]: Dividers and padding in the tab strip on tablets are incorrect in RTL at some positions.
[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]: See comments and screenshot in comment 0.
[List of other uplifts needed for the feature/fix]: Please uplift both patches.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: The fix changes paddings and dividers in RTL mode by simply taking into account the fact that tab positions in the tab strip are counted starting from the right in RTL mode.
[String changes made/needed]: None.

Comment 8

[Gerry Chang [:gchang]]

Hi Mihai,
Can you help check if this is fixed in the latest nightly?

Comment 9

[Gerry Chang [:gchang]]

Comment on attachment 8856526 [details]
Bug 1337947 - Fix tab strip divider drawing for RTL.

Fix an RTL issue. Aurora54+.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6430228451e7
https://hg.mozilla.org/releases/mozilla-aurora/rev/30801f76e945

Comment 11

[Ioana Chiorean]

Nightly 56.0a1 2017-07-30
Pixel C - Android 7.1.2 - Arabic languages

I am not seeing the issues described in the description/screenshots
Marking bug as Verified Fixed.

Comment 12

[Stefan Deiac]

Based on comment 11 I will remove the qe-verify flag, thanks.