Closed
Bug 583078
Opened 14 years ago
Closed 13 years ago
Too much padding on toolbars on Mac
Categories
(Firefox :: Theme, defect)
Tracking
()
RESOLVED
FIXED
Firefox 4.0b9
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: mkaply, Assigned: mstange)
References
Details
Attachments
(2 files, 2 obsolete files)
73.69 KB,
image/png
|
Details | |
3.01 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
With the changes for bug 559033, 6 pixels of padding were added around toolbars. They previously had none. This makes toolbars look really strange. Way too much space around them. Screenshot attached.
Comment 1•14 years ago
|
||
This needs to block. Also should we consider introducing lines to visually separate the third party toolbars?
blocking2.0: --- → ?
Assignee | ||
Comment 2•14 years ago
|
||
Michael, what are the extensions in that screenshot?
Reporter | ||
Comment 3•14 years ago
|
||
Google Toolbar and an addon that I'm working on that will be released in the next few days. I'll try some other toolbars as well.
Reporter | ||
Comment 4•14 years ago
|
||
I tried Web Developer toolbar, Operator, google toolbar. All have the same problem.
Assignee | ||
Comment 6•14 years ago
|
||
Nope. Needs a separate fix.
Reporter | ||
Comment 7•14 years ago
|
||
Any plans to fix this?
Comment 8•14 years ago
|
||
Since it is blocking, I would assume the answer to that needs to be yes :)
Assignee | ||
Comment 9•14 years ago
|
||
This reduces the bottom padding for toolbars from 4px to 2px and brings back the separator lines.
Attachment #475535 -
Flags: review?(dao)
Comment 10•14 years ago
|
||
Comment on attachment 475535 [details] [diff] [review] v1 >+#PersonalToolbar, >+#navigator-toolbox[tabsontop="true"] > #nav-bar, >+#navigator-toolbox[tabsontop="true"]:not([customizing]) > #nav-bar[collapsed="true"] + toolbar, >+#navigator-toolbox[tabsontop="true"]:not([customizing]) > #nav-bar[collapsed="true"] + #customToolbars + #PersonalToolbar { > -moz-appearance: none; > margin-top: -1px; /* overlay the bottom border of the toolbar above us */ > background-color: -moz-mac-chrome-active; > border-bottom: 1px solid rgba(0, 0, 0, 0.57); > } > #navigator-toolbox[tabsontop="true"] > #nav-bar, > #navigator-toolbox[tabsontop="true"]:not([customizing]) > #nav-bar[collapsed="true"] + toolbar, > #navigator-toolbox[tabsontop="true"]:not([customizing]) > #nav-bar[collapsed="true"] + #customToolbars + #PersonalToolbar { > margin-top: 0 !important; /* don't overlay the bottom border of the tabs toolbar */ > padding-top: 4px !important; > background-image: -moz-linear-gradient(rgba(255,255,255,.43), rgba(255,255,255,0)) !important; /* override lwtheme style */ >+ background-origin: border-box !important; > } This is the same selector except for #PersonalToolbar. Seems like #PersonalToolbar should have a separate margin rule at least. -moz-appearance: none; background-color: -moz-mac-chrome-active; isn't needed for all toolbars?
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #10) > -moz-appearance: none; background-color: -moz-mac-chrome-active; isn't needed > for all toolbars? No, only for those that shouldn't have separator lines. When the toolbar touches the title bar-, moz-appearance: toolbar draws a unified toolbar, and when it doesn't it draws -moz-mac-chrome-(in)active with a light border at the top and a dark border at the bottom.
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #475535 -
Attachment is obsolete: true
Attachment #477311 -
Flags: review?(dao)
Attachment #475535 -
Flags: review?(dao)
Comment 13•14 years ago
|
||
Comment on attachment 477311 [details] [diff] [review] v2 >+#PersonalToolbar { > -moz-appearance: none; >- margin-top: -1px; /* overlay the bottom border of the toolbar above us */ >+ margin-top: -1px; I'm not sure why you're removing that comment, it seems that it should stay. > #navigator-toolbox[tabsontop="true"] > #nav-bar, > #navigator-toolbox[tabsontop="true"]:not([customizing]) > #nav-bar[collapsed="true"] + toolbar, > #navigator-toolbox[tabsontop="true"]:not([customizing]) > #nav-bar[collapsed="true"] + #customToolbars + #PersonalToolbar { >- margin-top: 0 !important; /* don't overlay the bottom border of the tabs toolbar */ >+ -moz-appearance: none; >+ margin-top: 0; > padding-top: 4px !important; >+ border-bottom: 1px solid rgba(0, 0, 0, 0.57); >+ background-color: -moz-mac-chrome-active; > background-image: -moz-linear-gradient(rgba(255,255,255,.43), rgba(255,255,255,0)) !important; /* override lwtheme style */ >+ background-origin: border-box !important; >+} Isn't margin-top: 0 a no-op now?
Assignee | ||
Comment 14•14 years ago
|
||
(In reply to comment #13) > > #navigator-toolbox[tabsontop="true"] > #nav-bar, > > #navigator-toolbox[tabsontop="true"]:not([customizing]) > #nav-bar[collapsed="true"] + toolbar, > > #navigator-toolbox[tabsontop="true"]:not([customizing]) > #nav-bar[collapsed="true"] + #customToolbars + #PersonalToolbar { > >- margin-top: 0 !important; /* don't overlay the bottom border of the tabs toolbar */ > >+ -moz-appearance: none; > >+ margin-top: 0; > > padding-top: 4px !important; > >+ border-bottom: 1px solid rgba(0, 0, 0, 0.57); > >+ background-color: -moz-mac-chrome-active; > > background-image: -moz-linear-gradient(rgba(255,255,255,.43), rgba(255,255,255,0)) !important; /* override lwtheme style */ > >+ background-origin: border-box !important; > >+} > > Isn't margin-top: 0 a no-op now? It overwrites the margin-top: -1px from the #PersonalToolbar rule. That's the case when the navbar is hidden and the bookmarks toolbar is visible: in that case the bookmarks toolbar is adjacent to the tab bar, should get the gradient, and shouldn't overlay the bottom pixel of the tab bar.
Comment 15•14 years ago
|
||
You should adjust the comment to reflect that rather than dropping it.
Assignee | ||
Comment 16•14 years ago
|
||
I'll do so; dropping the comment wasn't intentional.
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #477311 -
Attachment is obsolete: true
Attachment #477494 -
Flags: review?(dao)
Attachment #477311 -
Flags: review?(dao)
Updated•14 years ago
|
Whiteboard: [has patch][needs review dao]
Comment 18•14 years ago
|
||
dao: ping review
Reporter | ||
Comment 19•14 years ago
|
||
dao: ping review again
Assignee | ||
Comment 20•14 years ago
|
||
The patch applies now that bug 547787 has landed. :)
Updated•13 years ago
|
Attachment #477494 -
Flags: review?(dao) → review+
Updated•13 years ago
|
Attachment #477494 -
Flags: approval2.0?
Assignee | ||
Comment 21•13 years ago
|
||
Comment on attachment 477494 [details] [diff] [review] v3 This is already pre-approved due to the bug's blocking status.
Attachment #477494 -
Flags: approval2.0?
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [has patch][needs review dao] → [can land]
Assignee | ||
Comment 22•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/dcf9db112efd
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → Firefox 4.0b9
You need to log in
before you can comment on or make changes to this bug.
Description
•