Closed Bug 403147 Opened 12 years ago Closed 12 years ago

Style the library window like a Media collection app for Vista Aero

Categories

(Firefox :: Bookmarks & History, defect)

x86
Windows Vista
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: faaborg, Assigned: mak)

References

Details

(Keywords: polish, Whiteboard: [vistatheme])

Attachments

(6 files, 9 obsolete files)

87.41 KB, image/png
Details
27.94 KB, image/png
Details
12.73 KB, image/png
Details
13.68 KB, image/png
Details
3.80 KB, patch
mak
: review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
69.50 KB, image/png
beltzner
: ui-review-
Details
Change the background color of the bookmarks organizer on Vista to (238,243,250).  This is to visually integrate with other application designed for organizing media objects.

See bug# 393514 for a mockup.
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [vistatheme]
The code, tested using userChrome.css:

#placesView,
#placesList,
#placeContent{
	background-color: #EEF3FA !important;
}
I love it! Is this going to happen for Firefox 3?
Blocks: 423491
There is no system color for this.
if this is to be hardcoded then we should hardcode the text along with it.
Blocks: 425598
No longer blocks: 425598
Depends on: 425598
Blocks: 425582
Just playing with this, it makes it hard to see selected but not focused items (for instance, select and item and then click in the searchbar). If you're going to hardcode colors though, you can fix that I guess.
Blocks: 405605
Depends on: 426660
No longer depends on: 425598
Depends on: 428031
Just to clarify a bit more, this a screenshot of the current organizer (top), and the organizer with the above background color applied (bottom). Focus is in the search bar in both cases. Its actually hard to tell what's selected in both cases, so maybe its not a huge problem?
Can we override the inactive selection color?  If not, I don't think this is enough of a problem to alter the overall design of the window since the inactive selection color is light gray in other media collection applications on vista (although with a slight gradient).
Attached image screenshot - experimental css (obsolete) —
alex, this is an experimental styling with background changed, 1px splitter, media-toolbox toolbar, plus fix to selection style (no gradients though). As you can see there is still some glitch (some glyph / icon is not good fot dark background). 
What do you think?
Attachment #315563 - Flags: ui-review?(faaborg)
I'll be landing a new views icon for vista, as well as new drop marker icons.  Overall looks great.  A few suggestions:

-set toolbar to 37 pixels tall
-set context bar to the same background color
Attached patch patch (obsolete) — Splinter Review
This is a patch for you to tryout with the above fixes, i've used the new selector to apply this only to windows default theme.

making toolbar 37px is better, done for both xp and vista.

notice that the splitter is really 3px wide, having it 1px made it really difficult to grab without attention, so it has a colored left border (and that explain the slightly padding in the right pane, if you put this in the left pane you end up with a detached scrollbar so it's worst).
I'm not set up to build on my vista machine at the moment, can you attach another screen shot?
Attached image updated screenshot (obsolete) —
Attachment #315563 - Attachment is obsolete: true
Attachment #315563 - Flags: ui-review?(faaborg)
Comment on attachment 315571 [details] [diff] [review]
patch

>+#placesView > splitter:-moz-system-metric(windows-default-theme) {
>+  width: 3px !important;
>+  min-width: 3px;
>+  -moz-border-left-colors: #A9B7C9;
>+  border-right: none;
>+  background-color: transparent;

Why transparent rather than #EEF3FA?
-moz-border-left-... is not RTL sensitive either.
Can't review, but it seems like the splitter border should probably be on the right not the left, so that its flush with the info pane.

I also don't think you need to hardcode the selected+focus colors, as -moz-menuhover and -moz-menuhovertext work fine there. Whatever you do though, should carry over to both tree's.

Is the appearance from Bug 428031 going to carry over?
(In reply to comment #12)
> Why transparent rather than #EEF3FA?
i'd prefer setting the correct background in placesView and having the splitter transparent, not a real reason though

> -moz-border-left-... is not RTL sensitive either.
needs to be fixed

also probably searchModifiers should use the same style selection as tree rows, should feel like more consistent.

(In reply to comment #14)
> Can't review, but it seems like the splitter border should probably be on the
> right not the left, so that its flush with the info pane.

as explained before, if you put it on the left you will have a flying scrollbar, detached from the border (very awful)

> I also don't think you need to hardcode the selected+focus colors, as
> -moz-menuhover and -moz-menuhovertext work fine there. Whatever you do though,
> should carry over to both tree's.

is carried over in both trees. thank you for hint on menuhover

> Is the appearance from Bug 428031 going to carry over?

having a dialog (white) background doesn't apply well on vista, feel free to report your ideas.
What makes you say white background doesn't apply well on Vista ? 
Here is a screen shot from 
https://bugzilla.mozilla.org/show_bug.cgi?id=420236#c76 
showing various Vista windows and they have white backgrounds.  
I use Vista HP, and the TaskManager, Explorer etc.. all have a white background, not powderblue of whatever it is. 



i'm also using vista from more than 1 year. still this dialog is following media player style rather than explorer style, see mockup in bug 393514. Hwv i was talking about the infoPane.
> having a dialog (white) background doesn't apply well on vista, feel free to
> report your ideas.

Sorry about the splitter comments. I guess I was skimming to fast (toolkit could use a better splitter object it seems).

I have to use XP right now, but I think I had just set the background to transparent and got the gray color from the window background. I don't have a huge preference for one or the other though. It does help split up the window (better than blue and white, which looks unfinished), but the combo of blue and gray isn't really my favorite either.
Attached image updated screenshot (obsolete) —
more experimentation with new toolbar icons:
- added a border around the full window contents (media player like)
- removed bottom border under the toolbar
- toolbar height at 36px (37 causes a 1px disabled icons move, to investigate)
- selected state simulated with background gradient images (i can't make vista menu background working, ideas?)
- used splitter code mostly from bug 426000
- used new libraryNavigation.png
- save button no more class small
Attachment #315571 - Attachment is obsolete: true
Attachment #315572 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attached file selection backgrounds (zipped pngs) (obsolete) —
about the window border: the accelerated version of Aero has it, while the non accelerated one does not have a border. Still the window without the border is really awful (imo) and appear less clean... but the same is true for the download manager, so could be a consistency problem.
Comment on attachment 317022 [details]
updated screenshot

Mike, any comment / idea?
Attachment #317022 - Flags: ui-review?(beltzner)
(In reply to comment #22)
The added border prevents the scrollbar from being mile-wide, and will look really bad on Aero.
(Also, why are the search scope buttons not native?)
Comment on attachment 317022 [details]
updated screenshot

So, I definitely like the way this looks, but I don't think we should be packaging our own gradients for the selection of items.

Let's take that part out, and just use standard selection until we can get -moz-appearance: menuItem working properly
Attachment #317022 - Flags: ui-review?(beltzner) → ui-review+
(In reply to comment #25)
> Let's take that part out, and just use standard selection until we can get
> -moz-appearance: menuItem working properly

the only remaining issue about this is that when a tree has NO FOCUS the selected item will be mostly unrecognizable since it has a light grey very near to the background.
So we should at least overload the selected unfocused grey, do you have a valid color for that?
Attached image selection screenshot
since aero defines 2 colors for the border, what about taking the internal border color as background, without gradients? This way the selection is quite clear in both status (focus / unfocus) and is more similar to the real one.
We could also use the same styling for search modifiers, so hover would be gray, checked would be blue-ish.
Or alternatively we have to find a valid gray for the unfocused selection.
Attachment #317022 - Attachment is obsolete: true
Attachment #317023 - Attachment is obsolete: true
Attachment #317024 - Attachment is obsolete: true
Attachment #317877 - Flags: ui-review?(beltzner)
how search modifiers would appear using the same selection style
Attachment #317878 - Flags: ui-review?(beltzner)
(In reply to comment #27)
> Or alternatively we have to find a valid gray for the unfocused selection.

That sentence indicates that you already know this, but I'll say it anyway: A border color isn't such a valid gray since, well, since it's a border color, and there's usually no text on borders.

I suppose adding a border is actually enough to fix the problem in attachment 315397 [details].
Attached patch patch (obsolete) — Splinter Review
a try-it-by-yourself patch for testing
fixes splitters color too (they are still threeddarkshadow for me), remove small save button style (should be styled using its identifier, i suggest using only a save icon though to gain some space)
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Just as a point of reference, here is the native appearance for tree selection in media collection applications on Vista.  Left is focused, right is unfocused.
Given beltzner's ui-r+ let's get this landed without the changes to tree view selection, and file a follow up bug on that.

I'm modifying the summary since this patch will make a large number of changes to the window's appearance.
Summary: Change bookmarks organizer window background color on Vista → Style the library window like a Media collection app for Vista Aero
as requested by Alex, same patch as above without selection changes.
Asking review to Dao
Attachment #318177 - Flags: review?(dao)
Comment on attachment 318177 [details] [diff] [review]
same patch - without selection changes

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

>-.small, .small[disabled="true"] {
>-  font-size: x-small;
>-  min-width: 0px;
>-  padding: 0px 4px 0px 4px;
>-  margin: 0px;  
>-  border: 0px;
>-}
>-
>-.small .button-text,
>-.small .button-box {
>-  padding: 0px;
>-  border: 0px;
>-}
>-

Why is that part of this bug? I see no rationale for this other than "should be styled using its identifier", which you aren't doing.

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

> #sidebar-splitter:-moz-system-metric(windows-default-theme) {
>   border: 0;
>-  -moz-border-end: 1px solid #a9b7c9;
>+  -moz-border-end: 1px solid #A9B7C9;
>+  -moz-border-right-colors: #A9B7C9;

-moz-border-left-colors is missing, and you can remove the color from -moz-border-end.
(same in organizer-aero.css)

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

>+#placesToolbar:-moz-system-metric(windows-default-theme) {
>+  -moz-appearance: media-toolbox !important;
>+  color: white !important;
>+}
>+
>+#placesMenu > menu:-moz-system-metric(windows-default-theme) {
>+  color: white !important;
>+}
>+

Please drop :-moz-system-metric(windows-default-theme) and use -moz-win-mediatext as the text color once bug 427045 has landed.
Are all the !important flags needed?
Depends on: 427045
(In reply to comment #34)
> (From update of attachment 318177 [details] [diff] [review])
> >Index: browser/themes/gnomestripe/browser/places/organizer.css
> 
> >-.small, .small[disabled="true"] {
> >-  font-size: x-small;
> >-  min-width: 0px;
> >-  padding: 0px 4px 0px 4px;
> >-  margin: 0px;  
> >-  border: 0px;
> >-}
> >-
> >-.small .button-text,
> >-.small .button-box {
> >-  padding: 0px;
> >-  border: 0px;
> >-}
> >-
> 
> Why is that part of this bug? I see no rationale for this other than "should be
> styled using its identifier", which you aren't doing.

there was previous requests to restore the save styling (that is completely wrong actually), so i put this as a first step to restore it to default. Can be removed though clearly.

> >Index: browser/themes/winstripe/browser/browser-aero.css
> 
> > #sidebar-splitter:-moz-system-metric(windows-default-theme) {
> >   border: 0;
> >-  -moz-border-end: 1px solid #a9b7c9;
> >+  -moz-border-end: 1px solid #A9B7C9;
> >+  -moz-border-right-colors: #A9B7C9;
> 
> -moz-border-left-colors is missing, and you can remove the color from
> -moz-border-end.
> (same in organizer-aero.css)

there is not a left-border, however easily settable to transparent

> 
> >Index: browser/themes/winstripe/browser/places/organizer-aero.css
> 
> >+#placesToolbar:-moz-system-metric(windows-default-theme) {
> >+  -moz-appearance: media-toolbox !important;
> >+  color: white !important;
> >+}
> >+
> >+#placesMenu > menu:-moz-system-metric(windows-default-theme) {
> >+  color: white !important;
> >+}
> >+
> 
> Please drop :-moz-system-metric(windows-default-theme) and use
> -moz-win-mediatext as the text color once bug 427045 has landed.

sure, waiting for that

> Are all the !important flags needed?

mostly, but can do a final check to see if i can reduce them to a minimum

(In reply to comment #35)
> > >Index: browser/themes/winstripe/browser/browser-aero.css
> > 
> > > #sidebar-splitter:-moz-system-metric(windows-default-theme) {
> > >   border: 0;
> > >-  -moz-border-end: 1px solid #a9b7c9;
> > >+  -moz-border-end: 1px solid #A9B7C9;
> > >+  -moz-border-right-colors: #A9B7C9;
> > 
> > -moz-border-left-colors is missing, and you can remove the color from
> > -moz-border-end.
> > (same in organizer-aero.css)
> 
> there is not a left-border, however easily settable to transparent

There's a left border in RTL, since you're using -moz-border-end.
Attached patch patch (obsolete) — Splinter Review
reduced to a minimum (no more save button styling).
fixed Dao's comments (thank you)
i've seen bug 427045 is checkin-needed so i've already inserted -moz-win-mediatext
Attachment #318342 - Flags: review?(dao)
Attachment #318177 - Attachment is obsolete: true
Attachment #318177 - Flags: review?(dao)
Attachment #317877 - Flags: ui-review?(beltzner)
Attachment #317878 - Flags: ui-review?(beltzner)
Comment on attachment 318342 [details] [diff] [review]
patch

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

>+#searchModifiers:-moz-system-metric(windows-default-theme) {
>+  border-bottom: 1px solid;
>+  -moz-border-bottom-colors: #A9B7C9;

border-bottom: 1px solid #A9B7C9;

>+#infoPaneBox:-moz-system-metric(windows-default-theme) {
>+  -moz-border-top-colors: #A9B7C9;

border-top-color: #A9B7C9;

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

>-#organizerScopeBar> toolbarbutton:not([disabled="true"]):hover {
>+#organizerScopeBar> toolbarbutton:not([disabled="true"]):not([checked="true"]):hover {
>   border-color: ThreeDShadow;
> }

nit: space after #organizerScopeBar missing

> #organizerScopeBar > toolbarbutton[checked="true"] {
>   border-color: ThreeDDarkShadow !important;
> }

If you add :not([checked="true"]) in the above rule, you can remove "!important" here, right?
Attachment #318342 - Flags: review?(dao) → review+
Attached patch patchSplinter Review
(In reply to comment #38)
> If you add :not([checked="true"]) in the above rule, you can remove
> "!important" here, right?

no that's still needed to have a flat look, however this should be restyled (in another bug) to appear more OS consistent on Vista (as seen in above screenshots).

I hope Dao's review is enough to have approval since this is more a theme bug than a places' one.

Final patch contains:
- library background
- splitters correct color
- toolbar media styling (dark background, white text)

everything other should be addressed in followups
Attachment #317880 - Attachment is obsolete: true
Attachment #318342 - Attachment is obsolete: true
Attachment #318375 - Flags: review+
Attachment #318375 - Flags: approval1.9?
Whiteboard: [vistatheme] → [vistatheme][needs beltzner approval]
Depends on: 431309
Just wanted to give you a heads-up about bug 431309, which will prepend -win- to the media-toolbox keyword, in case that ends up landing before this bug.
(In reply to comment #39)
> Final patch contains:
> - library background

This took me a few minutes to get used to, but I agree that it's consistent. It doesn't carry over to the sidebars, but that's OK, I guess. 

> - splitters correct color
> - toolbar media styling (dark background, white text)

I'm not seeing the white text (see attachment) - we definitely need that.
Attachment #318827 - Flags: ui-review-
Attachment #318375 - Flags: approval1.9? → approval1.9-
could require an !important, are we still using color: -moz-win-mediatext; or the name has changed?
(In reply to comment #42)
> could require an !important, are we still using color: -moz-win-mediatext; or
> the name has changed?
> 

No, the keyword hasn't been renamed (or was it slated for renaming).  Take a look at bug 427045 comment 31.  Was that patch tested before it was landed?
so, let's wait for bug 427045 correct patch
Whiteboard: [vistatheme][needs beltzner approval] → [vistatheme][waiting for bug 427045]
Note that the white drop down arrow for aero has now landed, we should use it when using the black media toolbar:

http://mxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/places/dropDown-aero.png
(In reply to comment #45)
> Note that the white drop down arrow for aero has now landed, we should use it
> when using the black media toolbar:
> 
> http://mxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/places/dropDown-aero.png

Can a new bug be filed for this? Or handle that leftover in bug 403137. This bug already has a patch that should be ready to go. Note that the white dropmaker should have some darker outline, since we're going to use the media toolbox appearance regardless of whether it's the default theme.
>since we're going to use the media
>toolbox appearance regardless of whether it's the default theme.

The toolbar will gracefully degrade back to a normal toolbar for windows classic, but what about the background color of the window?  It seems like we should make all of these changes to be just for Aero to maintain the correct appearance for classic.
Yes, the hardcoded background and border colors use the windows-default-theme metric.
Comment on attachment 318375 [details] [diff] [review]
patch

as Dao said let's fix that in a followup since here we have review and a tested patch. Moreover I don't know how to access the menu glyph, so i'll leave that to Dao.

I tested the patch with lastest hurly, and the text is now correctly white, so reasking approval.
Attachment #318375 - Flags: approval1.9- → approval1.9?
It's too bad we can't just draw the dropdown in SVG using the text color, that way it would automatically change color on the media-toolbox for classic & aero.

I suppose the easiest way though is to draw the glyph more like this:
http://mxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/tree/twisty-clsd-aero.png
since that would work on almost any color.
What needs to be done for the next version is for dropmarker arrows to be drawn directly (either with SVG or with a -moz-appearance: dropmarker), so that the correct foreground color could be used and kludges like bug 413961, which is the cause of the smaller arrows in Firefox 3, can be avoided.  In the meantime, a hard-coded white arrow (no border needed) would be good enough, as the windows-default-theme metric would ensure that it won't get used on a light background.
FWIW, I still can't get this patch to apply such that it creates white text. What am I missing?
(In reply to comment #52)
> FWIW, I still can't get this patch to apply such that it creates white text.
> What am I missing?
> 

What do you see when you look at attachment 318835 [details]?
Comment on attachment 318375 [details] [diff] [review]
patch

a=beltzner for 1.9, there must have been something wrong with my build, works fine now
Attachment #318375 - Flags: approval1.9? → approval1.9+
probably you were lacking the last fix for bug 427045 since it has been checked-in in 2 pieces?
Keywords: checkin-needed
Whiteboard: [vistatheme][waiting for bug 427045] → [vistatheme][has patch][has reviews]
Blocks: 430903
Checking in browser/themes/winstripe/browser/browser-aero.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser-aero.css,v  <--  browser-aero.css
new revision: 1.13; previous revision: 1.12
done
Checking in browser/themes/winstripe/browser/places/organizer-aero.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/places/organizer-aero.css,v  <--  organizer-aero.css
new revision: 1.2; previous revision: 1.1
done
Checking in browser/themes/winstripe/browser/places/organizer.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/places/organizer.css,v  <--  organizer.css
new revision: 1.15; previous revision: 1.14
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [vistatheme][has patch][has reviews] → [vistatheme]
Target Milestone: --- → Firefox 3
(In reply to comment #31)
> Created an attachment (id=317943) [details]
> Native media collection tree selection
> 
> Just as a point of reference, here is the native appearance for tree selection
> in media collection applications on Vista.  Left is focused, right is
> unfocused.

Alex, in comment 31 you give us a screen shot with an example. The unfocused folder selection looks much better there as what we have currently. If I select a folder in the left pane and select a bookmark within the right pane, it's really hard to see which folder in the left pane is selected. Will there be an update of the CSS?
(In reply to comment #57)
> Will there be an update of the CSS?
> 
To make it look like the screenshot, it'll have to be done in the native widget code.  All that could be done with CSS is to change the highlight color (it'll have to be hard-coded, but the background is already hard-coded...)
(In reply to comment #58)
> To make it look like the screenshot, it'll have to be done in the native widget
> code.  All that could be done with CSS is to change the highlight color (it'll
> have to be hard-coded, but the background is already hard-coded...)

Kai, shall I file a new bug about? Or is no further change planned?

Meanwhile verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050806 Minefield/3.0pre ID:2008050806. Everything looks like it was given with the screenshots.
Status: RESOLVED → VERIFIED
(In reply to comment #59)
> Kai, shall I file a new bug about? Or is no further change planned?
> 
To do this, we need to implement -moz-appearance: treeitem for Windows.  The CSS keyword is already present, but it doesn't do anything on Windows right now.  Probably something for the next release, but if you want, you can file a bug about it so that it's less likely to be forgotten.
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.