Closed Bug 583078 Opened 14 years ago Closed 13 years ago

Too much padding on toolbars on Mac

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mkaply, Assigned: mstange)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 559033
This needs to block.  Also should we consider introducing lines to visually separate the third party toolbars?
blocking2.0: --- → ?
Michael, what are the extensions in that screenshot?
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.
I tried Web Developer toolbar, Operator, google toolbar. All have the same problem.
To be fixed by 547787?
blocking2.0: ? → betaN+
Nope. Needs a separate fix.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
No longer depends on: 547787
Any plans to fix this?
Since it is blocking, I would assume the answer to that needs to be yes :)
Attached patch v1 (obsolete) — Splinter Review
This reduces the bottom padding for toolbars from 4px to 2px and brings back the separator lines.
Attachment #475535 - Flags: review?(dao)
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?
(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.
Attached patch v2 (obsolete) — Splinter Review
Attachment #475535 - Attachment is obsolete: true
Attachment #477311 - Flags: review?(dao)
Attachment #475535 - Flags: review?(dao)
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?
(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.
You should adjust the comment to reflect that rather than dropping it.
I'll do so; dropping the comment wasn't intentional.
Attached patch v3Splinter Review
Attachment #477311 - Attachment is obsolete: true
Attachment #477494 - Flags: review?(dao)
Attachment #477311 - Flags: review?(dao)
Whiteboard: [has patch][needs review dao]
dao: ping review
dao: ping review again
The patch applies now that bug 547787 has landed. :)
Attachment #477494 - Flags: review?(dao) → review+
Attachment #477494 - Flags: approval2.0?
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?
Keywords: checkin-needed
Whiteboard: [has patch][needs review dao] → [can land]
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.

Attachment

General

Created:
Updated:
Size: