[RTL] Add padding to the right side of close buttons for each tab in grid layouts

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
Theme and Visual Design
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: ItielMaN, Assigned: Tom Klein)

Tracking

(Blocks: 1 bug, {rtl})

unspecified
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

MozReview Requests

()

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

Attachments

(4 attachments)

(Reporter)

Description

5 months ago
Created attachment 8835030 [details]
Close buttons too close to the page titles

STR:
1. Install Hebrew/Arabic build of Nightly
2. Enable Compact Tabs in settings
3. Open any web page (for reproducing the bug with RTL page title, open http://mozilla.org/he/firefox/new)
4. Open the tab tray
5. Observe the X button of the tab

AR:
The page's title is too close to the X button.

ER:
There should be padding in the right side of the X button.

See attached.

Updated

5 months ago
See Also: → bug 1337933
(Assignee)

Comment 1

5 months ago
Created attachment 8835095 [details]
phone in landscape mode

I'm seeing this in all of the tabs panels that use a grid (which makes sense; they all use the same basic layout to display the tabs).
(Assignee)

Updated

5 months ago
Summary: [RTL] Add padding to the right side of close buttons for each tab if Compact Tabs is enabled → [RTL] Add padding to the right side of close buttons for each tab in grid layouts
Comment hidden (mozreview-request)
(Assignee)

Comment 3

5 months ago
Doesn't the first screenshot also show, on the last (selected) tab, that the text isn't being faded as it approaches the close button?  Is that a known issue?
Comment hidden (mozreview-request)
(Assignee)

Comment 5

5 months ago
Created attachment 8835210 [details]
top: buggy (padding on both sides); bottom: worked around

Apparently there's a bug which causes *both* paddingRight and paddingEnd to be applied, which is what was happening in the first version of this patch.  See https://bugs.chromium.org/p/chromium/issues/detail?id=235118 #2 for an old report of the bug; apparently it came back because I'm seeing it on an API 23 emulator.

The workaround suggested on the bug report, which is to add explicit paddingLeft="0" and paddingStart="0", seems to work here (I verified both the buggy and fixed behavior by using Studio's Layout Inspector and checking the mPaddingLeft and mPaddingRight properties).

(I'm new to android's RTL support, so if there's a better way to do this I'd be happy to hear about it :)
(Reporter)

Comment 6

5 months ago
(In reply to Tom Klein from comment #3)
> Doesn't the first screenshot also show, on the last (selected) tab, that the
> text isn't being faded as it approaches the close button?  Is that a known
> issue?

I'm well aware of this issue, though I don't think there's a bug filed about it.
AFAIK this happens only to Hebrew and Arabic characters (it may happen for other langs as well).
Comment on attachment 8835163 [details]
Bug 1337897 - RTL the padding between the close button and title in tab grid layouts.

https://reviewboard.mozilla.org/r/110840/#review112782
Attachment #8835163 - Flags: review?(s.kaspari) → review+
Assignee: nobody → twointofive
Status: NEW → ASSIGNED

Comment 8

4 months ago
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2e61f82dd80d
RTL the padding between the close button and title in tab grid layouts. r=sebastian

Comment 9

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2e61f82dd80d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(Reporter)

Comment 10

4 months ago
I can confirm this is fixed on latest Nightly.
(Assignee)

Comment 11

4 months ago
Comment on attachment 8835163 [details]
Bug 1337897 - RTL the padding between the close button and title in tab grid layouts.

Approval Request Comment
[Feature/Bug causing the regression]: RTL for fennec.
[User impact if declined]: No padding in RTL mode between the close button and the tab title when viewing tabs in the tabs trays that show tabs in a grid (see first attachment).
[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]: No I guess.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: The old padding values still exist and haven't changed, we're simply adding extra padding information in the way required by android for RTL.
[String changes made/needed]: None.
Attachment #8835163 - Flags: approval-mozilla-aurora?

Updated

4 months ago
status-firefox53: --- → affected
Comment on attachment 8835163 [details]
Bug 1337897 - RTL the padding between the close button and title in tab grid layouts.

Fix an RTL padding issue between close button & title in tab grid layouts. This polishes the UI. Aurora53+.
Attachment #8835163 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 13

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f97503c9caea
status-firefox53: affected → fixed
You need to log in before you can comment on or make changes to this bug.