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

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, NeedInfo)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 55
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

(Assignee)

Description

4 months ago
Created attachment 8835114 [details]
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 months ago
mozreview-review
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 4

2 months ago
mozreview-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)

Updated

2 months ago
Assignee: nobody → twointofive

Comment 5

2 months ago
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
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Comment 7

a month ago
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+

Comment 10

a month ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6430228451e7
https://hg.mozilla.org/releases/mozilla-aurora/rev/30801f76e945
status-firefox54: affected → fixed

Updated

a month ago
Flags: needinfo?(mihai.ninu) → needinfo?(ioana.chiorean)
You need to log in before you can comment on or make changes to this bug.