Button left-/right-padding too wide

VERIFIED FIXED in Firefox 65

Status

()

defect
P1
normal
VERIFIED FIXED
8 months ago
8 months ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

({regression})

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 unaffected, firefox65 verified, firefox66 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

See the spec: https://design.firefox.com/photon/components/buttons.html#default-3

To the 8px padding 10px are added from https://searchfox.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#249

This leads to invisible font size label.

PS: Is it okay that the menulist popups are center aligned? This looks a bit weird. Worth for a new bug?
Priority: -- → P3
Duplicate of this bug: 1513306
Posted patch 1512857-remove-padding.patch (obsolete) — Splinter Review
The !important isn't needed but we have to set 0 to override https://searchfox.org/mozilla-central/source/toolkit/themes/windows/global/button.css#21.

And because this needs to be overridden only on Windows, moved to windows's common.css.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9030634 - Flags: review?(dao+bmo)
Comment on attachment 9030634 [details] [diff] [review]
1512857-remove-padding.patch

>--- a/toolkit/themes/windows/global/in-content/common.css
>+++ b/toolkit/themes/windows/global/in-content/common.css
>@@ -39,8 +39,13 @@
>      border: 2px dotted Highlight;
>   }
> }
> 
> html|button {
>   /* XUL button min-width */
>   min-width: 6.3em;
> }
>+
>+xul|button > xul|*.button-box {
>+  padding-right: 0;
>+  padding-left: 0;
>+}

I believe !important is still needed to override this:

https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/toolkit/themes/windows/global/button.css#59

I wouldn't mind removing this behavior, though.
Attachment #9030634 - Flags: review?(dao+bmo) → review-
Added the !important.
Attachment #9030634 - Attachment is obsolete: true
Attachment #9030645 - Flags: review?(dao+bmo)
Comment on attachment 9030645 [details] [diff] [review]
1512857-remove-padding.patch v2

Can we remove the rule I linked to?
Attachment #9030645 - Flags: review?(dao+bmo)
This shifted the button content 1px to the bottom/right when clicked. The button gives the feedback too by changing the boder/background color - so it isn't really needed.
Attachment #9030645 - Attachment is obsolete: true
Attachment #9030647 - Flags: review?(dao+bmo)
Comment on attachment 9030647 [details] [diff] [review]
1512857-remove-padding.patch v3

Thanks!
Attachment #9030647 - Flags: review?(dao+bmo) → review+
Keywords: checkin-needed
Keywords: regression
Priority: P3 → P1
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eed399e61358
Remove the additional padding in .button-box/.menulist-label-box from common.css buttons/menulists. r=dao
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eed399e61358
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment on attachment 9030647 [details] [diff] [review]
1512857-remove-padding.patch v3

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1511394

User impact if declined: See comment 0.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See comment 0.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Small CSS changes.

String changes made/needed: none
Attachment #9030647 - Flags: approval-mozilla-beta?
Comment on attachment 9030647 [details] [diff] [review]
1512857-remove-padding.patch v3

[Triage Comment]
CSS tweaks, approved for 65.0b5.
Attachment #9030647 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I successfully reproduced the issue on Firefox Nightly 65.0a1 (2018-12-09) under Windows 10 (x64) using the information from Comment 0 and STR from bug 1513306.

The issue is no longer reproducible on latest Firefox Nightly 66.0a1 (2018-12-13) under Windows 10 (x64).
Status: RESOLVED → VERIFIED
The issue is no longer reproducible on Firefox Beta 65.0b5 (20181216183345) under Windows 10 (x64).
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.