433114 - Polish/Fix up the appearance of the library toolbar for Windows

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Kai Liu]

Attachment: patch v1

1) Bug 403147 increased the height of the toolbar to 36px.  While this is necessary and desirable when used with the media toolbox appearance, I think that it is too large otherwise.  As a comparison, the navigation toolbar in the main browser, when in large icons mode, is 36px.  The library toolbar does not use large icons, and as a result, 36px looks too fat without the media toolbox appearance.  I propose that we use 36px only if the media toolbox appearance is in use.

2) The bottom border of the toolbar looks out of place on Classic.

3) Horizontal padding: in 36px mode, the margin after the search box should be increased so that it does not look mismatched compared to the vertical spacing, and in 30px mode, the margin before the back button should be decreased so that it does not look mismatched compared to the vertical spacing.

4) White dropmarkers should be used with the media toolbox appearance (bug 430903).

The attached patch addresses all four of these issues.

Comment 1

[Kai Liu]

Attachment: after screenshot

After screenshot on Vista Basic, Royale, and Classic.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 320304 [details] [diff] [review]
patch v1

>Index: browser/themes/winstripe/browser/places/organizer-aero.css

>-#placesToolbar {
>+#placesToolbar:-moz-system-metric(windows-default-theme) {
>   -moz-appearance: -moz-win-media-toolbox;
>   color: -moz-win-mediatext;
>+  min-height: 36px;
> }

The media-toolbox appearance isn't limited to what we recognize as the default theme.
Instead, I think we should do:

#placesToolbar {
  -moz-appearance: -moz-win-media-toolbox;
  color: -moz-win-mediatext;
}

#placesToolbar:-moz-system-metric(windows-default-theme) {
  min-height: 36px;
}

Although maybe the appearance should already imply this min-height?

>+#back-button:-moz-system-metric(windows-default-theme),
>+#forward-button[chromedir="rtl"]:-moz-system-metric(windows-default-theme) {
>+  -moz-margin-start: 8px;
>+}

I don't understand what #forward-button[chromedir="rtl"]:-moz-system-metric(windows-default-theme) is doing there.

>Index: browser/themes/winstripe/browser/places/organizer.css

> /* Toolbar */
> #placesToolbar {
>+  min-height: 31px;
>+}
>+
>+#placesToolbar:-moz-system-metric(windows-default-theme) {
>   border: none;
>-  min-height: 36px;
>+  min-height: 30px;
> }

border: none; doesn't do anything in the windows-default-theme case.
And again, don't overutilize that metric. This will give you the expected result:

#back-button {
  margin-top: 3px;
  margin-bottom: 3px;
}

Comment 3

[Dão Gottwald [:dao]]

> This will give you the expected result:
> 
> #back-button {
>   margin-top: 3px;
>   margin-bottom: 3px;
> }

Or maybe a top/bottom padding for the toolbar. That would be slightly more straightforward and should work as well, although I didn't test it.

Comment 4

[Kai Liu]

(In reply to comment #2)
> The media-toolbox appearance isn't limited to what we recognize as the default
> theme.
>
Okay.

> Although maybe the appearance should already imply this min-height?
> 
Not sure about the media toolbox, but the communications toolboxes used in Windows Mail and Windows Calendar are 30px (TBH, I'm not sure if 36px is a good idea even with the media toolbox style, but at least with the media toolbox, 36px doesn't look nearly as bad as it would otherwise).

> I don't understand what
> #forward-button[chromedir="rtl"]:-moz-system-metric(windows-default-theme) is
> doing there.
> 
Neither do I.  :P  I had just blindly copied-and-pasted the style from organizer.css.  This (and the existing style from which it was copied) is obviously wrong.  So I suppose this is a 5th issue that this bug should address: incorrect RTL spacing for the back/forward buttons.

> border: none; doesn't do anything in the windows-default-theme case.
>
You're right; the bottom border isn't shown if it's the last toolbar in the toolbox.

> Or maybe a top/bottom padding for the toolbar. That would be slightly more
> straightforward and should work as well, although I didn't test it.
> 
Or better yet, use the toolbar padding for the horizontal adjustments, too.  And that would also get rid of a quirk of using min-height (the border being counted as a part of the min-height in the classic case).

Comment 5

[Kai Liu]

Attachment: patch v2

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

pay attention to the button status (pressed, hover) and the fit of the reflection of the button with the reflection on the toolbar, IIRC there were problems there (about padding etc)

Comment 7

[Kai Liu]

Attachment: patch v2.1

Adjusted some of the spacing...

Comment 8

[Kai Liu]

Attachment: patch v2.1b

This is basically the same as 2.1, except that I decided to go back to using min-height for the default Aero style.  The media toolbox in the default Aero style is very fragile and must be exactly 36px.  So I think that going back to min-height for this case is a bit better semantically, even though non-Aero is using padding instead of min-height.

(In reply to comment #6)
> pay attention to the button status (pressed, hover) and the fit of the
> reflection of the button with the reflection on the toolbar, IIRC there were
> problems there (about padding etc)
> 

Thanks for the heads-up.  It should look fine with v2.1/v2.1b.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #4)
> > border: none; doesn't do anything in the windows-default-theme case.
> >
> You're right; the bottom border isn't shown if it's the last toolbar in the
> toolbox.

The border isn't there because -moz-appearance:toolbar kicks in.

> > Or maybe a top/bottom padding for the toolbar. That would be slightly more
> > straightforward and should work as well, although I didn't test it.
> > 
> Or better yet, use the toolbar padding for the horizontal adjustments, too. 
> And that would also get rid of a quirk of using min-height (the border being
> counted as a part of the min-height in the classic case).

Yes, that's what I meant: use padding in order to get rid of the min-height.

Comment 10

[Kai Liu]

In addition to the polish issues mentioned in comment 0, this also fixes a visual regression involving the appearance of the back/forward buttons in RTL introduced by bug 403140 (see comment 4).  Based on these gains and the fact that this is a Windows-only CSS-only patch, I would like to ask that this be considered for inclusion in the event that a RC2 has to be spun.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/index.cgi/rev/722d9d43e1dd

Comment 12

[Samuel Sidler (old account; do not CC)]

Comment on attachment 320375 [details] [diff] [review]
patch v2.1b

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified all the changes from comment 0 with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a1pre) Gecko/2008072203 Minefield/3.1a1pre ID:2008072203

Comment 14

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/themes/winstripe/browser/places/organizer-aero.css 	1.4
mozilla/browser/themes/winstripe/browser/places/organizer.css 	1.17 

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.2) Gecko/2008082911 Firefox/3.0.2 ID:2008082911

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Larger right padding

Kai, while verifying the fix for 1.9.0.2 I've noticed that on trunk we have a larger right padding for the search box.

Comment 17

[Kai Liu]

Attachment: before and after bug 449375

(In reply to comment #16)
> Kai, while verifying the fix for 1.9.0.2 I've noticed that on trunk we have a
> larger right padding for the search box.

This bug landed a long time ago on trunk, so that is a recent change caused by bug 449375.  I'll file a new bug for this later tonight.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 320375 [details] [diff] [review]
patch v2.1b

this is already fixed on various branches, including 1.9.1

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #17)
> This bug landed a long time ago on trunk, so that is a recent change caused by
> bug 449375.  I'll file a new bug for this later tonight.

Finally filed this as bug 486289.