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

Status: RESOLVED FIXED

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

Description

[Itiel]

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

Comment 1

[Tom Klein]

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

Comment 2

[Tom Klein]

Attachment: Bug 1337897 - RTL the padding between the close button and title in tab grid layouts.

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

Comment 3

[Tom Klein]

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 4

[Tom Klein]

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

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

Comment 5

[Tom Klein]

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

Comment 6

[Itiel]

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

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

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

https://hg.mozilla.org/mozilla-central/rev/2e61f82dd80d

Comment 10

[Itiel]

I can confirm this is fixed on latest Nightly.

Comment 11

[Tom Klein]

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.

Comment 12

[Gerry Chang [:gchang]]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f97503c9caea