Closed Bug 630622 Opened 13 years ago Closed 13 years ago

Update the Library design to the browser aero style

Categories

(Firefox :: Theme, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 6

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fixed-in-places])

Attachments

(1 file, 14 obsolete files)

10.05 KB, patch
Details | Diff | Splinter Review
Attached image screenshot (obsolete) —
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.
Attachment #508835 - Flags: feedback?
Attachment #508835 - Flags: feedback? → feedback?(shorlander)
Attached patch patch v1.0 (obsolete) — Splinter Review
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.
Attachment #508836 - Flags: feedback?(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
Component: Bookmarks & History → Theme
QA Contact: bookmarks → theme
(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.
I meant "to the left of the search bar", not sidebar.
(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.
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.
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Summary: Extend glass in the Library when possible → Update the Library design to the browser aero style
Attachment #508835 - Flags: feedback?(shorlander)
Attachment #508836 - Attachment is obsolete: true
Attachment #508836 - Flags: feedback?(dao)
Attached image screenshot (obsolete) —
most like browser
Attachment #508835 - Attachment is obsolete: true
Attached patch patch v1.0 (obsolete) — Splinter Review
Tried to follow browser styles for the various properties,
Attachment #511523 - Flags: review?(dao)
Attachment #511522 - Flags: feedback?(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?
Attachment #511522 - Flags: feedback?(shorlander) → feedback+
(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.
Status: ASSIGNED → NEW
Attached patch replace buttons patch v1.0 (obsolete) — Splinter Review
this additional part replaces the buttons
Attachment #512983 - Flags: review?(dao)
Attached image Looks broken on XP (obsolete) —
Attachment #512983 - Flags: review?(dao) → review-
Attached patch patch v1.1 (obsolete) — Splinter Review
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.
Attachment #511523 - Attachment is obsolete: true
Attachment #527403 - Flags: review?(dao)
Attachment #511523 - Flags: review?(dao)
Attached patch buttons patch v1.1 (obsolete) — Splinter Review
additional part that replaces the buttons, updated to the latest changes in browser.
Attachment #512983 - Attachment is obsolete: true
Attachment #527404 - Flags: review?(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.
Attachment #527403 - Flags: review?(dao) → review-
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.
(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.
Attached patch patch v1.2 (obsolete) — Splinter Review
updated search field style per comment 20 and bug 603790. Not changed to -moz-win-glass, per comment 22.
Attachment #527403 - Attachment is obsolete: true
Attachment #528724 - Flags: review?(dao)
Attached patch buttons patch v1.2 (obsolete) — Splinter Review
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).
Attachment #527404 - Attachment is obsolete: true
Attachment #528727 - Flags: review?(dao)
Attachment #527404 - Flags: review?(dao)
Comment on attachment 527196 [details]
Looks broken on XP

obsoleting, since fixed in recent patches.
Attachment #527196 - Attachment is obsolete: true
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?
(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.
(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.
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.
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.
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".
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.
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.)
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.
(In reply to comment #34)
> using the native toolbar buttons would be really bad.

Can you elaborate on this?
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.
No, native toolbar buttons only get a button texture when hovered. (We're already using that appearance for Organize/View/Import and Backup.)
(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)
Yep, but they have -moz-appearance: toolbarbutton.
Attached patch Minimalistic styling v1.0 (obsolete) — Splinter Review
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.
Attachment #511522 - Attachment is obsolete: true
Attachment #528724 - Attachment is obsolete: true
Attachment #528727 - Attachment is obsolete: true
Attachment #532178 - Flags: review?
Attachment #528724 - Flags: review?(dao)
Attachment #528727 - Flags: review?(dao)
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 :(
Attachment #532178 - Flags: review? → review?(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 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.)
Attached patch Minimalistic styling v1.1 (obsolete) — Splinter Review
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.
Attachment #532178 - Attachment is obsolete: true
Attachment #532210 - Flags: review?(dao)
Attachment #532178 - Flags: review?(dao)
ehr I should also merge the #searchFilter rules there, done locally.
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?
(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.
Attached patch Minimalistic styling v1.2 (obsolete) — Splinter Review
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.
Attachment #532210 - Attachment is obsolete: true
Attachment #532226 - Flags: review?(dao)
Attachment #532210 - Flags: review?(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.
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.
It's a change for Win 7 classic. What does this look like with your patch?
Attached image win7 classic screenshot (obsolete) —
seems fine to me
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
Attachment #532226 - Flags: review?(dao) → review+
> > /* 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
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.
ok, makes sense. I'll go for 6px so it's consistent with external padding.
Addressed comments.
Attachment #532226 - Attachment is obsolete: true
Attachment #532233 - Attachment is obsolete: true
http://hg.mozilla.org/projects/places/rev/26e32eea767a
Whiteboard: [fixed-in-places]
http://hg.mozilla.org/mozilla-central/rev/26e32eea767a
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
Verified on:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110520 Firefox/6.0a1
Status: RESOLVED → VERIFIED
Blocks: 486289
You need to log in before you can comment on or make changes to this bug.