66.06 KB, image/jpeg
176.11 KB, image/png
59 bytes, text/x-review-board-request
166.38 KB, image/png
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.
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).
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
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?
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 :)
(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
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/2e61f82dd80d RTL the padding between the close button and title in tab grid layouts. r=sebastian
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
I can confirm this is fixed on latest Nightly.
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?
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+
status-firefox53: affected → fixed
You need to log in before you can comment on or make changes to this bug.