Clean up toolbar button margin and padding rules

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 53
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments)

Posted patch patchSplinter Review
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.
Attachment #8817286 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8817286 - Flags: review?(gijskruitbosch+bugs) → review+
(and apologies for the delay in reviewing...)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6e6111f917
Clean up toolbar button margin and padding rules. r=gijs
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
Flags: needinfo?(dao+bmo)
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).
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.
Flags: needinfo?(dao+bmo)
(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.
(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).
(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.
Narrowed it down further. Setting just padding-right to 0 causes the failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=caf82c493f564c7fb472c03612cddba0ae110181
Blocks: 1322953
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bedb73fd7ec
Clean up toolbar button margin and padding rules. r=gijs
Blocks: 1322975
https://hg.mozilla.org/mozilla-central/rev/8bedb73fd7ec
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1323844
No longer blocks: 1340173
Depends on: 1340173
You need to log in before you can comment on or make changes to this bug.