1322430 - Clean up toolbar button margin and padding rules

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

The toolbar button margin and padding rules are somewhat messy on Mac (I think more so than on other platforms) and suffer from at least two notable bugs that UX folks brought to my attention:

- the overflow button is taller than other buttons due to being outside of #nav-bar-customization-target
- type="menu-button" buttons lack horizontal margin

This is an attempt to fix these issues and generally clean things up a bit.

Since I hacked this together by applying the Mac theme on Linux, I need the reviewer to carefully test this on Mac.

Comment 1

[Dão Gottwald [:dao]]

Attachment: before (Mac theme on Linux)

Comment 2

[Dão Gottwald [:dao]]

Attachment: after (Mac theme on Linux)

Comment 3

[:Gijs (he/him)]

Comment on attachment 8817286 [details] [diff] [review]
patch

Review of attachment 8817286 [details] [diff] [review]:
-----------------------------------------------------------------

Tested, also in the devedition theme and with a lwtheme. I noticed other bugs, but nothing related to this, so r=me

Comment 4

[:Gijs (he/him)]

(and apologies for the delay in reviewing...)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6e6111f917
Clean up toolbar button margin and padding rules. r=gijs

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out on suspicion of causing sanitization tests like browser_sanitize-timespans.js to fail on OS X:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac2a2ae05c7012f69b8ba5a835625671193a4c8a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=53cf106dd90cfcfd2b920571e789ffbeee55b651
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40445899&repo=mozilla-inbound

13:22:10     INFO - TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | Checking for today form history entry creation - 
13:22:10     INFO - TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | Checking for b4today form history entry creation - 
13:22:10     INFO - TEST-PASS | browser/base/content/test/general/browser_sanitize-timespans.js | 9 checks made - 
13:22:10     INFO - Buffered messages finished
13:22:10     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_sanitize-timespans.js | Uncaught exception - at chrome://browser/content/sanitize.js:187 - Error: Error sanitizing
13:22:10     INFO - Stack trace:
13:22:10     INFO -     Sanitizer.prototype._sanitize<@chrome://browser/content/sanitize.js:187:13
13:22:10     INFO -     Sanitizer.prototype.sanitize<@chrome://browser/content/sanitize.js:68:19
13:22:10     INFO -     onHistoryReady@chrome://mochitests/content/browser/browser/base/content/test/general/browser_sanitize-timespans.js:103:9
13:22:10     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:932:23
13:22:10     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
13:22:10     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
13:22:10     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
13:22:10     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
13:22:10     INFO -     handleCompletion@chrome://mochitests/content/browser/browser/base/content/test/general/browser_sanitize-timespans.js:483:29
13:22:10     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
13:22:10     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
13:22:10     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
13:22:10     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
13:22:10     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
13:22:10     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59

Comment 7

[Daniel Holbert [:dholbert]]

I'm not sure, but I think the relevant part of the log might be "searchBar.textbox is undefined".

> console.error:
> 13:22:11     INFO -   Error sanitizing formdata
> 13:22:11     INFO -   Message: TypeError: searchBar.textbox is undefined
> 13:22:11     INFO -   Stack:
> 13:22:11 INFO - Sanitizer.prototype.items.formdata.clear<@chrome://browser/content/sanitize.js:419:15

https://treeherder.mozilla.org/logviewer.html#?job_id=40445899&repo=mozilla-inbound#L4770

I don't know offhand how the patch caused this error, but it does seem to be perma-fail & triggered by this commit & mac-specific (just like this bug's patch).

Comment 8

[Dão Gottwald [:dao]]

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-inbound/sha512/b5880784d8d0fcf26484c224c2dc03ca55ac54cafef5620f8cda7c4d8fcf1fa5dee992d4485a3444cf6b9c2d822016b7ba60205c2fd94e0abc95aa4193af32ce

Somehow this change caused the search bar to be pushed off the toolbar into the overflow panel. I'm not sure how and why exactly.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #8)
> Somehow this change caused the search bar to be pushed off the toolbar into
> the overflow panel. I'm not sure how and why exactly.

I narrowed the failure down to:

-#nav-bar-customization-target {
-  padding: 4px;
-}

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a5a298a5be1c3064367f6e814d6c7141272613b

Still can't make sense of this.

Comment 10

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > Somehow this change caused the search bar to be pushed off the toolbar into
> > the overflow panel. I'm not sure how and why exactly.
> 
> I narrowed the failure down to:
> 
> -#nav-bar-customization-target {
> -  padding: 4px;
> -}
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1a5a298a5be1c3064367f6e814d6c7141272613b
> 
> Still can't make sense of this.

Guessing while jetlagged and not actually looking at the code: the JS that takes care of deciding whether/what to take out of the navbar and stick in the overflow bucket (and/or the gecko-only layout 'max left/right scroll' property (whose exact name escapes me) it relies on) doesn't work correctly if padding on the customization target is 0, and ends up dumping *everything* in the overflow panel (maybe only after window resizes or something).

Comment 11

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Dão Gottwald [:dao] from comment #9)
> > I narrowed the failure down to:
> > 
> > -#nav-bar-customization-target {
> > -  padding: 4px;
> > -}
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=1a5a298a5be1c3064367f6e814d6c7141272613b
> > 
> > Still can't make sense of this.
> 
> Guessing while jetlagged and not actually looking at the code: the JS that
> takes care of deciding whether/what to take out of the navbar and stick in
> the overflow bucket (and/or the gecko-only layout 'max left/right scroll'
> property (whose exact name escapes me) it relies on) doesn't work correctly
> if padding on the customization target is 0, and ends up dumping
> *everything* in the overflow panel (maybe only after window resizes or
> something).

The padding is 0 on Windows and Linux though.

Comment 12

[Dão Gottwald [:dao]]

Narrowed it down further. Setting just padding-right to 0 causes the failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=caf82c493f564c7fb472c03612cddba0ae110181

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bedb73fd7ec
Clean up toolbar button margin and padding rules. r=gijs

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8bedb73fd7ec