630622 - Update the Library design to the browser aero style

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Marco Bonardo [:mak]]

Attachment: screenshot

Based on late cycle, this is a conservative change, when compositor is not available I just fallback to the old style. I tried to reduce additions to a minimum too.
The result is good enough to cry to be approved.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 508836 [details] [diff] [review]
patch v1.0

Dao, any hints about this? maybe I should manage the fallback by giving the toolbars a solid @customToolbarColor@?
The library css needs some cleanup, but I didn't want to change too much at this stage.

Comment 3

[Dão Gottwald [:dao]]

Some thoughts:
- MS discourages glass behind text (for good reasons)
- The alignment on both sides of the search bar doesn't seem to fit
- The patch doesn't seem to extend the draggable area to the toolbar

Comment 4

[Marco Bonardo [:mak]]

(In reply to comment #3)
> Some thoughts:
> - MS discourages glass behind text (for good reasons)

Yes, since this is a menubar (faked as a toolbar) I followed our main window menubar design. A classic solid bar with customToolbarColor could be fine too, I guess less appealing due to being more "classical". Maybe Stephen has ideas to make this appear native without losing too much glass.

> - The alignment on both sides of the search bar doesn't seem to fit

actually this is not a regression, the full style needs some cleanup but it's not the scope here.  Btw, the empty space on the left of the sidebar is flexible, so most of the effect is due to how my screenshot was taken.

> - The patch doesn't seem to extend the draggable area to the toolbar

Yep it doesn't, and actually that depends if we want to retain the glass appearance or not.

Comment 5

[Marco Bonardo [:mak]]

I meant "to the left of the search bar", not sidebar.

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #4)
> (In reply to comment #3)
> > Some thoughts:
> > - MS discourages glass behind text (for good reasons)
> 
> Yes, since this is a menubar (faked as a toolbar) I followed our main window
> menubar design.

I thought so, but the menu bar "design" is just a band-aid really; we can get away with it because the menu bar isn't enabled by default. 

> > - The alignment on both sides of the search bar doesn't seem to fit
> 
> actually this is not a regression,

I think it actually is one. Without the toolbar as a visual container, the search bar is just floating around with inconsistent and seemingly arbitrary distances to the adjoining UI elements and the window border.

Comment 9

[Shawn Thompson]

I think you should just put in the new back/forward, then replace the toolbar BG with one like the toolbar in the main window. It improves consistency, and also makes it look a bit more fitting in Windows 7.

Comment 10

[Marco Bonardo [:mak]]

Makes sense, the toolbar should be similar to the browser upper part (not maximized), so with solid color and a radius in the upper part. I hope to revisit the patch soon, I have to test it in the various theme combinations.

Comment 11

[Marco Bonardo [:mak]]

Attachment: screenshot

most like browser

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Tried to follow browser styles for the various properties,

Comment 13

[Stephen Horlander [:shorlander]]

Comment on attachment 511522 [details]
screenshot

This looks nice!

The Glass version you proposed initially would be awesome but I don't see a way around the text on glass issue without rethinking the design of the window.

Would it be possible to pickup the small style back/forward arrows from the main window for consistency?

Comment 14

[Marco Bonardo [:mak]]

(In reply to comment #13)
> Would it be possible to pickup the small style back/forward arrows from the
> main window for consistency?

Shouldn't be a problem, but those buttons have a lot of styling attached (the image only provides the arrow I think), maybe a follow-up or a part 2? Let's see what Dao thinks about that.

Comment 15

[Marco Bonardo [:mak]]

Attachment: replace buttons patch v1.0

this additional part replaces the buttons

Comment 17

[Dão Gottwald [:dao]]

Attachment: Looks broken on XP

Comment 18

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Yeah, we were doing something really dumb like using a min-height and setting random paddings and margins, that broke on since I tried to make the margins more consistent.

This addresses that problem removing crazy hacks for height and using paddings and margins more healthy. I'm happier about the results since the rules look cleaner.

Tests on Win7 glass, basic, classic and on XP luna and classic.

Comment 19

[Marco Bonardo [:mak]]

Attachment: buttons patch v1.1

additional part that replaces the buttons, updated to the latest changes in browser.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 527403 [details] [diff] [review]
patch v1.1

>+@media not all and (-moz-windows-classic) {
>+  #searchFilter {
>+    -moz-appearance: none;
>+    background-color: rgba(255,255,255,.725);
>+    color: black;
>+    padding: 2px;
>+    -moz-padding-start: 4px;
>+    background-clip: padding-box;
>+    border: 1px solid ThreeDDarkShadow;
>+    border-radius: 3.5px;
>+    box-shadow: 0 1px 0 rgba(0,0,0,.1) inset,
>+                0 1px 0 rgba(255,255,255,.4);
>+  }

Please use @media all and (-moz-windows-default-theme) for this.

>+@media all and (-moz-windows-compositor) {
>+  #places {
>+    -moz-appearance: -moz-win-borderless-glass;

>+  #placesToolbar,
>+  #placesView {
>+    border-color: @toolbarShadowColor@;

We're doing this in the main window because the standard glass borders don't mesh well with tabs. This doesn't seem to apply here.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 527404 [details] [diff] [review]
buttons patch v1.1

>+#placesToolbar > .toolbarbutton-1 {

Umm, I have no idea why these buttons have the toolbarbutton-1 class, let alone chromeclass-toolbar-additional. Can you get rid of them?

Please update both patches for bug 603790 as well.

Comment 22

[Marco Bonardo [:mak]]

(In reply to comment #20)
> >+@media all and (-moz-windows-compositor) {
> >+  #places {
> >+    -moz-appearance: -moz-win-borderless-glass;
> 
> >+  #placesToolbar,
> >+  #placesView {
> >+    border-color: @toolbarShadowColor@;
> 
> We're doing this in the main window because the standard glass borders don't
> mesh well with tabs. This doesn't seem to apply here.

I tried using the classic glass, but the rounded corners on top of the toolbar don't merge well with the glass border, resizing the window shows small black artifacts on this border, and often the window is coming up completely black (at least on my system, with d3d9 layers). Using the current method instead, it's pixel perfect.
Also, removing this complicates the other non-glass themes, since then I have to manage the borders differently, while now it's really simple.
So, I'd prefer to retain the current styling if possible.

Comment 23

[Marco Bonardo [:mak]]

Attachment: patch v1.2

updated search field style per comment 20 and bug 603790. Not changed to -moz-win-glass, per comment 22.

Comment 24

[Marco Bonardo [:mak]]

Attachment: buttons patch v1.2

the code was already up-to-date to bug 603790, so this only addresses the classes removal per comment 21 (pinstripe and gnomestripe never themed based on these classes, so no updates needed there).

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 527196 [details]
Looks broken on XP

obsoleting, since fixed in recent patches.

Comment 26

[Marco Bonardo [:mak]]

I'd love to have the new style in FF6 and remove these patches from my queue, do you have any guessed ETA on the review?

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #22)
> I tried using the classic glass, but the rounded corners on top of the
> toolbar don't merge well with the glass border, [...]

I don't see why you're trying to mesh rounded corners with standard glass. (The standard glass border is slightly round already.)

> Also, removing this complicates the other non-glass themes, since then I
> have to manage the borders differently, while now it's really simple.
> So, I'd prefer to retain the current styling if possible.

Native Aero basic doesn't have such a border either.

Comment 28

[Marco Bonardo [:mak]]

(In reply to comment #27)
> (In reply to comment #22)
> > I tried using the classic glass, but the rounded corners on top of the
> > toolbar don't merge well with the glass border, [...]
> 
> I don't see why you're trying to mesh rounded corners with standard glass.
> (The standard glass border is slightly round already.)

Afaict it's not the same rounding as the top part of the browser, that I want to mimic in the Library.

> Native Aero basic doesn't have such a border either.

Right, I have no rounded corners in basic indeed, but not adding any border is visually confusing since the Library background is all light blue, differently from other windows.

I think the screenshot above looks pretty clean, I don't see a problem with using the same "trick" as the browser window, and removing the rounding from top corners makes it look ugly when open near/inside the browser window.

Comment 29

[Dão Gottwald [:dao]]

This shouldn't completely mimic the browser window. As mentioned before, we went non-native there because of the tabs. The library window looks much like an ordinary explorer window, for instance.

Comment 30

[Marco Bonardo [:mak]]

I know that, but it's the only other window with back/forward buttons, and a search field, much similar to the browser window. Looking at the top part of the 2 windows together (that is most likely the most common situation, since the user opens the Library from the browser window) they would look inconsistent with different borders. Other windows like the download manager or preferences are more "tool-like" than the Library.

Comment 31

[Dão Gottwald [:dao]]

Sure, as for any other window, the borders wouldn't be inconsistent with the main window. I don't really buy that the back and forward buttons and the search field are a crucial factor mandating the black border. I also don't see why being "tool-like" matters here, or why the library wouldn't be "tool-like".

Comment 32

[Marco Bonardo [:mak]]

Also the visual inconsistence between Back button (the new one with 3.5px rounded border) and toolbox border and the right part of the searchbar (again 3.5px rounderd border) and the toolbox border is pretty bad with a squared toolbox.

Btw, I want to remove this 3 months old bug for FF4 from my queue, so just tell me whatever I have to do to get a positive review, since I don't want to get stressed to style a window that is planned for removal.

Comment 33

[Dão Gottwald [:dao]]

I realize this is planned for removal, but I'd like to avoid creating a precedent for random windows following a non-native style just because that's thought to be the new hot thing. Going native is the guideline -- we'll deviate from it when there's a compelling reason but shouldn't do it arbitrarily.

With regards to the buttons, I think it would actually make sense to update only the glyphs and use the native toolbar button appearance. (Again, there was a different motivation for using a custom style in the main window.)

Comment 34

[Marco Bonardo [:mak]]

Sorry, but if to get a review I have to make the Library ugly, I'm not interested in fixing this bug anymore.
I can survive to the removal of the rounded border (even if it will be inconsistent with all the other radius on the toolbox, as explained), but using the native toolbar buttons would be really bad.
Regarding the precedent, the Library has always been special and non-native, with its own buttons different from anything other, its light blue background and such. That has not been an issue till now, has never created a followed precedent, and I don't see why it is an issue now.

Comment 35

[Dão Gottwald [:dao]]

(In reply to comment #34)
> using the native toolbar buttons would be really bad.

Can you elaborate on this?

Comment 36

[Marco Bonardo [:mak]]

Native toolbar buttons would just be 2 non-adjacent "squared" buttons, they would appear really ugly as back/forward, especially when compared to the browser window. I don't want another print-preview window.

Comment 37

[Dão Gottwald [:dao]]

No, native toolbar buttons only get a button texture when hovered. (We're already using that appearance for Organize/View/Import and Backup.)

Comment 38

[Marco Bonardo [:mak]]

(In reply to comment #37)
>  (We're
> already using that appearance for Organize/View/Import and Backup.)

those are <menu> inside a <menubar> (apart on OS X)

Comment 39

[Dão Gottwald [:dao]]

Yep, but they have -moz-appearance: toolbarbutton.

Comment 40

[Marco Bonardo [:mak]]

Attachment: Minimalistic styling v1.0

Ok, the new minimalistic look is not that bad, and it has a positive effect: there is less styling to maintain.
Since the buttons patch has become mostly code removal I've merged the 2 patches.

Comment 41

[Marco Bonardo [:mak]]

Comment on attachment 532178 [details] [diff] [review]
Minimalistic styling v1.0

Dao, could you please add a [:dao] shortcut to yout bugzilla nick, I keep wrongly trying to use it and it doesn't match :(

Comment 42

[Dão Gottwald [:dao]]

Comment on attachment 532178 [details] [diff] [review]
Minimalistic styling v1.0

>+@media all and (-moz-windows-default-theme) {

>+  #searchFilter:-moz-placeholder {
>+    color: #777;
>+  }

What's the point of this?

>+@media all and (-moz-windows-compositor) {
>+  #places {
>+    -moz-appearance: -moz-win-glass;
>+    background: transparent;
>+  }

This is a no-op, isn't it?

>+#back-button:-moz-locale-dir(rtl) > .toolbarbutton-icon,
>+#forward-button:-moz-locale-dir(rtl) {
>+  -moz-transform: scaleX(-1);
> }

I think you want the icon in both cases.

Comment 43

[Dão Gottwald [:dao]]

Comment on attachment 532178 [details] [diff] [review]
Minimalistic styling v1.0

>--- a/browser/themes/winstripe/browser/places/organizer-aero.css
>+++ b/browser/themes/winstripe/browser/places/organizer-aero.css

>+#placesToolbar {

>+  background-image: -moz-linear-gradient(@toolbarHighlight@, rgba(255,255,255,0));

We probably don't want this gradient for classic and maybe not for aero basic either. (In the browser window, we want it because it connects with the active tab.)

Comment 44

[Marco Bonardo [:mak]]

Attachment: Minimalistic styling v1.1

addressed comments.
I have kept the hilighting in Glass only, it looks much lighter and consistent.
I've merged the @media all and (-moz-windows-default-theme) rules in organizer-aero.css there was no reason to keep them splitted.

Comment 45

[Marco Bonardo [:mak]]

ehr I should also merge the #searchFilter rules there, done locally.

Comment 46

[Dão Gottwald [:dao]]

Comment on attachment 532210 [details] [diff] [review]
Minimalistic styling v1.1

>--- a/browser/themes/winstripe/browser/places/organizer-aero.css
>+++ b/browser/themes/winstripe/browser/places/organizer-aero.css

>+#placesToolbox {
>+  -moz-appearance: none;
>+  background-color: transparent;
>+}
>+
>+#placesToolbar {
>+  -moz-appearance: none;
>+  background-color: -moz-Dialog;
>+  color: -moz-dialogText;
>+}
>+
>+#placesView {
>   border-top: none;
> }

Is this wanted for aero basic and classic? Shouldn't they use just use the organizer.css styling?

Comment 47

[Marco Bonardo [:mak]]

(In reply to comment #46)

> Is this wanted for aero basic and classic? Shouldn't they use just use the
> organizer.css styling?

I'll try to comment them and see how it looks, I have some fear regarding Basic, but let's see.

Comment 48

[Marco Bonardo [:mak]]

Attachment: Minimalistic styling v1.2

So, in Classic it doesn't make any difference, while in Basic removing those makes use of the "glassy" toolbox background. Unfortunately this decreases readability and consistency with the browser window.
I've moved the 2 rules in a "media not all and (-moz-windows-classic)", I feel like this is the best compromise, looking at it. I'd suggest to try it in Nightly, eventually it's easy to change it.
The #placesView has to be present though, since otherwise the thick dark border on top of the view is annoying and unneeded.

Comment 49

[Dão Gottwald [:dao]]

> The #placesView has to be present though, since otherwise the thick dark
> border on top of the view is annoying and unneeded.

You're removing the border on Win 7 but not on XP -- regardless of the theme. This doesn't seem to make much sense.

Comment 50

[Marco Bonardo [:mak]]

I know, I think that border was introduces for some reason to fix something in Luna, but right now I don't remember what and I don't have an XP box at hand.
It's an easy follow-up though if we feel like, this didn't really change in my patch, so the worst case is that we are where we were before.

Comment 51

[Dão Gottwald [:dao]]

It's a change for Win 7 classic. What does this look like with your patch?

Comment 52

[Marco Bonardo [:mak]]

Attachment: win7 classic screenshot

seems fine to me

Comment 53

[Dão Gottwald [:dao]]

Comment on attachment 532226 [details] [diff] [review]
Minimalistic styling v1.2

>--- a/browser/themes/winstripe/browser/places/organizer.css
>+++ b/browser/themes/winstripe/browser/places/organizer.css

> #placesToolbar {
>-  padding: 3px;  /* b/f buttons have a 1px image padding */
>-  -moz-padding-end: 4px;
>+  padding: 3px 6px;
> }

I think we want to keep a smaller start padding here. E.g.:
  padding: 3px;
  -moz-padding-end: 6px;

>+#placesToolbar > toolbarbutton[disabled="true"] > .toolbarbutton-icon {
>+  opacity: .5;
> }

browser.css uses .4

[disabled] should work as well as [disabled="true"]

> /* Menu */
> #placesMenu {
>-  -moz-margin-start: 8px;
>   -moz-appearance: none;
>   border: none;
> }

The margin probably should be reduced rather than being removed entirely.

>--- a/browser/themes/winstripe/browser/places/organizer-aero.css
>+++ b/browser/themes/winstripe/browser/places/organizer-aero.css

>+  #searchFilter[focused="true"] {
>+    background-color: white;
>+  }

[focused] should be sufficient

Comment 54

[Marco Bonardo [:mak]]

> > /* Menu */
> > #placesMenu {
> >-  -moz-margin-start: 8px;
> >   -moz-appearance: none;
> >   border: none;
> > }
> 
> The margin probably should be reduced rather than being removed entirely.

wouldn't this make the back/forward buttons farer from the others? I thought we didn't want any specific visual difference across them

Comment 55

[Dão Gottwald [:dao]]

Looking at attachment 532233 [details], I imagine it would look slightly cleaner to have a small gap between the buttons and the menu / menu buttons.

Comment 56

[Marco Bonardo [:mak]]

ok, makes sense. I'll go for 6px so it's consistent with external padding.

Comment 57

[Marco Bonardo [:mak]]

Attachment: Minimalistic styling v1.3

Addressed comments.

Comment 58

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/26e32eea767a

Comment 59

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/26e32eea767a

Comment 60

[Simona Badau, Desktop QA]

Verified on:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110520 Firefox/6.0a1