Update the Library design to the browser aero style

VERIFIED FIXED in Firefox 6

Status

()

defect
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
Firefox 6
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-places])

Attachments

(1 attachment, 14 obsolete attachments)

10.05 KB, patch
Details | Diff | Splinter Review
Assignee

Description

8 years ago
Posted 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?
Assignee

Updated

8 years ago
Attachment #508835 - Flags: feedback? → feedback?(shorlander)
Assignee

Comment 1

8 years ago
Posted patch patch v1.0 (obsolete) — Splinter Review
Assignee

Comment 2

8 years ago
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
Assignee

Comment 4

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

Comment 5

8 years ago
I meant "to the left of the search bar", not sidebar.
Assignee

Updated

8 years ago
Duplicate of this bug: 631906
(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.
Assignee

Updated

8 years ago
Duplicate of this bug: 632692

Comment 9

8 years ago
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.
Assignee

Comment 10

8 years ago
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

Updated

8 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee

Updated

8 years ago
Summary: Extend glass in the Library when possible → Update the Library design to the browser aero style
Assignee

Updated

8 years ago
Attachment #508835 - Flags: feedback?(shorlander)
Assignee

Updated

8 years ago
Attachment #508836 - Attachment is obsolete: true
Attachment #508836 - Flags: feedback?(dao)
Assignee

Comment 11

8 years ago
Posted image screenshot (obsolete) —
most like browser
Attachment #508835 - Attachment is obsolete: true
Assignee

Comment 12

8 years ago
Posted patch patch v1.0 (obsolete) — Splinter Review
Tried to follow browser styles for the various properties,
Attachment #511523 - Flags: review?(dao)
Assignee

Updated

8 years ago
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?
Assignee

Comment 14

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

Comment 15

8 years ago
Posted patch replace buttons patch v1.0 (obsolete) — Splinter Review
this additional part replaces the buttons
Attachment #512983 - Flags: review?(dao)
Assignee

Updated

8 years ago
Duplicate of this bug: 629220
Posted image Looks broken on XP (obsolete) —
Attachment #512983 - Flags: review?(dao) → review-
Assignee

Comment 18

8 years ago
Posted 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)
Assignee

Comment 19

8 years ago
Posted 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.
Assignee

Comment 22

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

Comment 23

8 years ago
Posted 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)
Assignee

Comment 24

8 years ago
Posted 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)
Assignee

Comment 25

8 years ago
Comment on attachment 527196 [details]
Looks broken on XP

obsoleting, since fixed in recent patches.
Attachment #527196 - Attachment is obsolete: true
Assignee

Comment 26

8 years ago
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.
Assignee

Comment 28

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

Comment 30

8 years ago
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".
Assignee

Comment 32

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

Comment 34

8 years ago
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?
Assignee

Comment 36

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

Comment 38

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

Comment 40

8 years ago
Posted 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)
Assignee

Comment 41

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

Comment 44

8 years ago
Posted 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)
Assignee

Comment 45

8 years ago
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?
Assignee

Comment 47

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

Comment 48

8 years ago
Posted 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.
Assignee

Comment 50

8 years ago
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?
Assignee

Comment 52

8 years ago
Posted 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+
Assignee

Comment 54

8 years ago
> > /* 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.
Assignee

Comment 56

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

Comment 57

8 years ago
Addressed comments.
Attachment #532226 - Attachment is obsolete: true
Attachment #532233 - Attachment is obsolete: true
Assignee

Comment 59

8 years ago
http://hg.mozilla.org/mozilla-central/rev/26e32eea767a
Status: NEW → RESOLVED
Last Resolved: 8 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.