Closed Bug 1337947 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

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

People

(Reporter: twointofive, Assigned: twointofive)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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 on attachment 8856526 [details]
Bug 1337947 - Fix tab strip divider drawing for RTL.

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

LGTM
Attachment #8856526 - Flags: review?(max) → review+
Comment on attachment 8856527 [details]
Bug 1337947 - Fix tab strip item offsets for RTL.

https://reviewboard.mozilla.org/r/128478/#review131230
Attachment #8856527 - Flags: review?(max) → review+
Assignee: nobody → twointofive
Pushed by twointofive@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/169ddc500c53
https://hg.mozilla.org/mozilla-central/rev/d41d243d363b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.
Attachment #8856526 - Flags: approval-mozilla-aurora?
Hi Mihai,
Can you help check if this is fixed in the latest nightly?
Flags: qe-verify+
Flags: needinfo?(mihai.ninu)
Comment on attachment 8856526 [details]
Bug 1337947 - Fix tab strip divider drawing for RTL.

Fix an RTL issue. Aurora54+.
Attachment #8856526 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(mihai.ninu) → needinfo?(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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(ioana.chiorean)
Based on comment 11 I will remove the qe-verify flag, thanks.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.