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

VERIFIED FIXED in Firefox 3.1a1

Status

()

--
minor
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: kliu, Assigned: kliu)

Tracking

({polish, regression, verified1.9.0.2})

Trunk
Firefox 3.1a1
x86
Windows Vista
polish, regression, verified1.9.0.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.1 -
wanted1.9.0.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [addresses RTL regression, see comments 4,10])

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 320304 [details] [diff] [review]
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.
Attachment #320304 - Flags: review?(dao)
Flags: wanted1.9.0.x?
(Assignee)

Comment 1

11 years ago
Created attachment 320306 [details]
after screenshot

After screenshot on Vista Basic, Royale, and Classic.
Attachment #320306 - Flags: ui-review?(faaborg)
Attachment #320306 - Flags: ui-review?(faaborg) → ui-review+

Comment 2

11 years ago
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;
}
Attachment #320304 - Flags: review?(dao) → review-

Comment 3

11 years ago
> 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.
(Assignee)

Comment 4

11 years ago
(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).
(Assignee)

Comment 5

11 years ago
Created attachment 320360 [details] [diff] [review]
patch v2
Attachment #320304 - Attachment is obsolete: true
Attachment #320360 - Flags: review?(dao)
(Assignee)

Updated

11 years ago
Attachment #320360 - Flags: review?(dao)

Comment 6

11 years ago
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)
(Assignee)

Comment 7

11 years ago
Created attachment 320369 [details] [diff] [review]
patch v2.1

Adjusted some of the spacing...
Attachment #320360 - Attachment is obsolete: true
Attachment #320369 - Flags: review?(dao)
(Assignee)

Comment 8

11 years ago
Created attachment 320375 [details] [diff] [review]
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.
Attachment #320369 - Attachment is obsolete: true
Attachment #320375 - Flags: review?(dao)
Attachment #320369 - Flags: review?(dao)

Comment 9

11 years ago
(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.

Updated

11 years ago
Attachment #320375 - Flags: review?(dao) → review+
(Assignee)

Comment 10

11 years ago
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.
Keywords: regression
Summary: Polish up the appearance of the library toolbar for on Windows → Polish up the appearance of the library toolbar for Windows
Whiteboard: [RC2?]

Updated

11 years ago
Blocks: 403140
(Assignee)

Updated

11 years ago
Summary: Polish up the appearance of the library toolbar for Windows → Polish/Fix up the appearance of the library toolbar for Windows
Whiteboard: [RC2?] → [addresses RTL regression, see comments 4,10][RC2?]
(Assignee)

Updated

11 years ago
Flags: blocking1.9.0.1?
Whiteboard: [addresses RTL regression, see comments 4,10][RC2?] → [addresses RTL regression, see comments 4,10][RC2-]
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/index.cgi/rev/722d9d43e1dd
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1
(Assignee)

Updated

11 years ago
Attachment #320375 - Flags: approval1.9.0.1?
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-
Attachment #320375 - Flags: approval1.9.0.1? → approval1.9.0.2?
(Assignee)

Updated

10 years ago
Target Milestone: Firefox 3.1 → Firefox 3.1a1
Comment on attachment 320375 [details] [diff] [review]
patch v2.1b

Approved for 1.9.0.2. Please land in CVS. a=ss
Attachment #320375 - Flags: approval1.9.0.2? → approval1.9.0.2+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
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
Status: RESOLVED → VERIFIED
Blocks: 430903
Whiteboard: [addresses RTL regression, see comments 4,10][RC2-] → [needs 1.9.0 checkin][addresses RTL regression, see comments 4,10]
mozilla/browser/themes/winstripe/browser/places/organizer-aero.css 	1.4
mozilla/browser/themes/winstripe/browser/places/organizer.css 	1.17 
Keywords: checkin-needed → fixed1.9.0.2
Whiteboard: [needs 1.9.0 checkin][addresses RTL regression, see comments 4,10] → [addresses RTL regression, see comments 4,10]
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
Keywords: fixed1.9.0.2 → verified1.9.0.2
Created attachment 336273 [details]
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.
(Assignee)

Comment 17

10 years ago
Created attachment 336275 [details]
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.
Attachment #320375 - Flags: approval1.9.1?
Comment on attachment 320375 [details] [diff] [review]
patch v2.1b

this is already fixed on various branches, including 1.9.1
Attachment #320375 - Flags: approval1.9.1?
Blocks: 486289
(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.
No longer blocks: 486289
You need to log in before you can comment on or make changes to this bug.