Closed Bug 960517 Opened 6 years ago Closed 6 years ago

Adjust Australis's browser theme for Windows 8 and up

Categories

(Firefox :: Theme, defect)

29 Branch
All
Windows 8
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 + verified
firefox30 + verified

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

()

Details

(Whiteboard: [Australis:P1])

Attachments

(7 files, 36 obsolete files)

989 bytes, patch
dao
: review+
dao
: checkin+
Details | Diff | Splinter Review
2.28 KB, patch
dao
: review+
dao
: checkin+
Details | Diff | Splinter Review
7.27 KB, patch
dao
: review+
dao
: checkin+
Details | Diff | Splinter Review
1.04 KB, patch
dao
: review+
dao
: checkin+
Details | Diff | Splinter Review
18.59 KB, patch
dao
: review+
dao
: checkin+
Details | Diff | Splinter Review
5.14 KB, patch
jaws
: review+
mikedeboer
: checkin+
Details | Diff | Splinter Review
21.15 KB, patch
jaws
: review+
jaws
: checkin+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #859751 +++

While Australis looks terrific on Windows 7 and Vista, it looks totally out of place on Windows 8. It just doesn't fit in almost every way:
1. Color scheme - gray and white instead of blue.
2. Buttons and text fields - flat instead of bulky.
3. Hover effects - they're mixed because some are hard coded and some are not.

IMPORTANT: this bug is about the browser theme, NOT the toolkit theme work.
Attachment #8361043 - Flags: review?(dao)
Depends on: 961727
Dão, could you maybe throw in a first-pass review to maybe catch a few of the obvious mistakes/ omissions/ oversights I might've put in the patch?
Since I've been staring at the code for a while, I think there might be a couple of those...
(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Dão, could you maybe throw in a first-pass review to maybe catch a few of
> the obvious mistakes/ omissions/ oversights I might've put in the patch?
> Since I've been staring at the code for a while, I think there might be a
> couple of those...

You uploaded the wrong patch I think.
Gosh Tim, you're right!
Attachment #8361043 - Attachment is obsolete: true
Attachment #8361043 - Flags: review?(dao)
Attachment #8363156 - Flags: review?(dao)
Some suggestions :
- Restyle .panel-multiview-anchor
- Use the second row of icons as hover effect and "opened" effect (the third row of icons, the blue ones, is actually useless with shorlander's current mockup)
Also, make sure that the assets you updated include the panorama icon :
check bug 888601 .
I will chop the patch up into byte-size chunks coming Monday. Stay tuned ;)
Attachment #8363156 - Attachment is obsolete: true
Attachment #8363156 - Flags: review?(dao)
Attachment #8367386 - Flags: review?(dao)
(In reply to Mike de Boer [:mikedeboer] from comment #7)
> I will chop the patch up into byte-size chunks coming Monday. Stay tuned ;)

That's great, thanks.
Attachment #8367434 - Flags: review?(dao)
forgot to hg add a file.
Attachment #8367434 - Attachment is obsolete: true
Attachment #8367434 - Flags: review?(dao)
Attachment #8367830 - Flags: review?(dao)
Unbitrotted.
Attachment #8367386 - Attachment is obsolete: true
Attachment #8367386 - Flags: review?(dao)
Attachment #8367835 - Flags: review?(dao)
Attachment #8367836 - Flags: review?(dao)
Attachment #8367841 - Flags: review?(dao)
Attached patch Part 5: Windows 8 style tabs (obsolete) — Splinter Review
Attachment #8367853 - Flags: review?(dao)
Attachment #8367856 - Flags: review?(dao)
Attached patch Part 7: Windows 8 style toolbars (obsolete) — Splinter Review
Attachment #8367864 - Flags: review?(dao)
Attachment #8367830 - Attachment is obsolete: true
Attachment #8367830 - Flags: review?(dao)
Attachment #8367866 - Flags: review?(dao)
Comment on attachment 8367856 [details] [diff] [review]
Part 6: Windows 8 style main window

>@@ -189,17 +188,28 @@
>   }
> 
>   #appcontent:not(:-moz-lwtheme) {
>     background-color: -moz-dialog;
>   }
> 
>   #main-menubar:not(:-moz-lwtheme):not(:-moz-window-inactive) {
>     background-color: rgba(255,255,255,.5);
>-    border-radius: 4px;
>+  }
>+
>+  @media (-moz-os-version: windows-vista),
>+         (-moz-os-version: windows-win7) {
...

If you added showfunc = 1 to your diff options in ~/.hgrc, I could easily tell what media block you're modifying here.

> @media not all and (-moz-windows-compositor) {
>   @media (-moz-windows-default-theme) {
>-    #main-window {
>-      background-color: rgb(185,209,234);
>-    }
>-    #main-window:-moz-window-inactive {
>-      background-color: rgb(215,228,242);
>+    @media (-moz-os-version: windows-vista),
>+           (-moz-os-version: windows-win7) {
>+      #main-window {
>+        background-color: rgb(185,209,234);
>+      }
>+      #main-window:-moz-window-inactive {
>+        background-color: rgb(215,228,242);
>+      }
>     }

Is it possible to disable the compositor on Windows 8 by enabling something like Aero Basic on Windows 7? If possible on Windows 8, I have no idea what difference this makes visually and whether not setting the above background-colors is the right thing to do.
Comment on attachment 8367853 [details] [diff] [review]
Part 5: Windows 8 style tabs

>+%else
>+/* Background tab separators (3px wide) for XP */
>+#tabbrowser-tabs[movingtab] > .tabbrowser-tab[beforeselected]:not([last-visible-tab])::after,
>+.tabbrowser-tab:not([selected]):not([afterselected-visible]):not([afterhovered]):not([first-visible-tab]):not(:hover)::before,
>+#tabbrowser-tabs:not([overflow]) > .tabbrowser-tab[last-visible-tab]:not([selected]):not([beforehovered]):not(:hover)::after {
>+  background-image: url(chrome://browser/skin/tabbrowser/tab-separator-XP.png);
>+}
>+
> /* Use lighter colors of buttons and text in the titlebar on luna-blue */
> @media (-moz-windows-theme: luna-blue) {
>   #main-window[tabsintitlebar]:not([inFullscreen]) .tabbrowser-arrowscrollbox > .scrollbutton-up,
>   #main-window[tabsintitlebar]:not([inFullscreen]) .tabbrowser-arrowscrollbox > .scrollbutton-down {
>     list-style-image: url(chrome://browser/skin/tabbrowser/tab-arrow-left-inverted.png);
>   }
> 
>   #main-window[tabsintitlebar]:not([inFullscreen]) .tabs-newtab-button,
>diff --git a/browser/themes/windows/jar.mn b/browser/themes/windows/jar.mn
>--- a/browser/themes/windows/jar.mn
>+++ b/browser/themes/windows/jar.mn
>@@ -165,17 +165,17 @@ browser.jar:
> #       Makefile.in with a non-default marker of "%" and the result of that gets packaged.
>         skin/classic/browser/tabbrowser/tab-selected-end.svg         (tab-selected-end.svg)
>         skin/classic/browser/tabbrowser/tab-selected-start.svg       (tab-selected-start.svg)
> 
>         skin/classic/browser/tabbrowser/tab-stroke-end.png           (tabbrowser/tab-stroke-end.png)
>         skin/classic/browser/tabbrowser/tab-stroke-start.png         (tabbrowser/tab-stroke-start.png)
>         skin/classic/browser/tabbrowser/tabDragIndicator.png         (tabbrowser/tabDragIndicator.png)
>         skin/classic/browser/tabbrowser/tab-separator-luna-blue.png  (tabbrowser/tab-separator-luna-blue.png)
>-        skin/classic/browser/tabbrowser/tab-separator.png            (tabbrowser/tab-separator.png)
>+        skin/classic/browser/tabbrowser/tab-separator-XP.png         (tabbrowser/tab-separator-XP.png)

I see no reason why this should be packaged as tab-separator-XP.png rather than tab-separator.png. Going with the latter name will allow browser/themes/shared/tabs.inc.css to automatically use that image, removing the need for the above CSS rule.
Attachment #8367853 - Flags: review?(dao) → review-
Attachment #8367835 - Attachment is obsolete: true
Attachment #8367835 - Flags: review?(dao)
Attachment #8368479 - Flags: review?(dao)
Attached patch Part 5.1: Windows 8 style tabs (obsolete) — Splinter Review
Attachment #8367853 - Attachment is obsolete: true
Attachment #8368480 - Flags: review?(dao)
Attachment #8367856 - Attachment is obsolete: true
Attachment #8367856 - Flags: review?(dao)
Attachment #8368481 - Flags: review?(dao)
Attachment #8367864 - Attachment is obsolete: true
Attachment #8367864 - Flags: review?(dao)
Attachment #8368482 - Flags: review?(dao)
Attachment #8367866 - Attachment is obsolete: true
Attachment #8367866 - Flags: review?(dao)
Attachment #8368483 - Flags: review?(dao)
Attachment #8367836 - Attachment is obsolete: true
Attachment #8367836 - Flags: review?(dao)
Attachment #8368484 - Flags: review?(dao)
Attachment #8367841 - Attachment is obsolete: true
Attachment #8367841 - Flags: review?(dao)
Attachment #8368485 - Flags: review?(dao)
Comment on attachment 8368481 [details] [diff] [review]
Part 6.1: Windows 8 style main window

see comment 19
Attachment #8368481 - Flags: review?(dao)
Attachment #8368485 - Flags: review?(dao) → review+
(In reply to Dão Gottwald [:dao] from comment #19)
> Is it possible to disable the compositor on Windows 8 by enabling something
> like Aero Basic on Windows 7? If possible on Windows 8, I have no idea what
> difference this makes visually and whether not setting the above
> background-colors is the right thing to do.

It's apparently not possible to disable the compositor (DWM) on Windows 8 - http://msdn.microsoft.com/en-us/library/hh848042%28v=vs.85%29.aspx.
Sometimes a program can be displayed with the 'old' style as if composition were disabled, but it is in fact not. http://stackoverflow.com/questions/13831997/is-dwmiscompositionenabled-still-of-use-in-windows-8
Comment on attachment 8368481 [details] [diff] [review]
Part 6.1: Windows 8 style main window

> @media not all and (-moz-windows-compositor) {
>   @media (-moz-windows-default-theme) {
>-    #main-window {
>-      background-color: rgb(185,209,234);
>-    }
>-    #main-window:-moz-window-inactive {
>-      background-color: rgb(215,228,242);
>+    @media (-moz-os-version: windows-vista),
>+           (-moz-os-version: windows-win7) {
>+      #main-window {
>+        background-color: rgb(185,209,234);
>+      }
>+      #main-window:-moz-window-inactive {
>+        background-color: rgb(215,228,242);
>+      }
>     }

So this change appears to be unnecessary.
Attachment #8368481 - Flags: review-
(In reply to Mike de Boer [:mikedeboer] from comment #25)
> Created attachment 8368483 [details] [diff] [review]
> Part 2.3: Windows 8 style menu-panel buttons

I'd like to see panel subview anchor styled after bug 882807 is landed.

(In reply to Mike de Boer [:mikedeboer] from comment #21)
> Created attachment 8368479 [details] [diff] [review]
> Part 1.2: Windows 8 style toolbar icons

This should use latest icons from the new email link icon bug (in case it hasn't).
Attachment #8368481 - Attachment is obsolete: true
Attachment #8369385 - Flags: review?(dao)
Attachment #8369385 - Flags: review?(dao) → review+
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M?][Australis:P1]
Comment on attachment 8368484 [details] [diff] [review]
Part 3.1: Windows 8 style downloads panel

>+%ifdef WINDOWS_AERO
>+/* Win8 and beyond. */
>+@media not all and (-moz-os-version: windows-vista) {
>+  @media not all and (-moz-os-version: windows-win7) {
>+    #downloadsHistory {
>+      transition-duration: 150ms;
>+      transition-property: background-color;
>+      color: ButtonText;
>+    }
>+
>+    #downloadsHistory:hover {
>+      background-color: hsla(210,4%,10%,.05);
>+    }
>+
>+    #downloadsHistory:hover:active {
>+      background-color: hsla(210,4%,10%,.1);
>+      box-shadow: 0 2px 0 0 hsla(210,4%,10%,.1) inset;
>+    }
>+  }
>+}
>+%endif

Should this really be Win8-specific? If so, what about classic themes on Windows 8?

> @media (-moz-windows-default-theme) {
>   #downloadsPanel[hasdownloads] > #downloadsFooter {
>+    background-color: hsla(210,4%,10%,.04);
>+    box-shadow: 0 1px 0 hsla(210,4%,10%,.08) inset;
>+  }
> %ifdef WINDOWS_AERO
>-    background-color: #f1f5fb;
>+  @media (-moz-os-version: windows-vista),
>+         (-moz-os-version: windows-win7) {
>+    #downloadsPanel[hasdownloads] > #downloadsFooter {
>+      background-color: #f1f5fb;
> %else
>+  #downloadsPanel[hasdownloads] > #downloadsFooter {
>     background-color: hsla(216,45%,88%,.98);
> %endif
>     box-shadow: 0px 1px 2px rgb(204,214,234) inset;
>     border-bottom-left-radius: 3px;
>     border-bottom-right-radius: 3px;
>   }
>+%ifdef WINDOWS_AERO
>+}
>+%endif
> }

And this? (Note that this does exclude classic.)

Actually marking r- because I find the above block extremely hard to read... Can you somehow unwind this, even if this means duplicating lines?
Attachment #8368484 - Flags: review?(dao) → review-
Comment on attachment 8368479 [details] [diff] [review]
Part 1.2: Windows 8 style toolbar icons

>-        skin/classic/aero/browser/Toolbar.png                        (Toolbar-aero.png)
>+        skin/classic/aero/browser/Toolbar.png
>         skin/classic/aero/browser/Toolbar-inverted.png
>+        skin/classic/aero/browser/Toolbar-aero.png

This doesn't look right. Toolbar-aero.png should be packaged as Toolbar.png, and then browser/themes/shared/toolbarbuttons.inc.css will take care of it automatically (so you can drop the rules referring to Toolbar-aero.png).

The new Win8-specific Toolbar.png contains unused states. I think we should drop them from the image until we use them. It seems debatable whether we'll ever want to use them... Seem like we could use opacity for the non-hover/hover distinction and achieve a similar effect with a much lower maintenance burden.

Do we really need the spit between (the new, Win8-specific) Toolbar-inverted.png and Toolbar-inverted-XPVista7.png? Note that Toolbar-inverted.png has much weaker outlines around the icons which I don't think will work well enough on noisy lightweight themes. It contains unused states as well.
Attachment #8368479 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #34)
> Comment on attachment 8368479 [details] [diff] [review]
> Part 1.2: Windows 8 style toolbar icons
> 
> >-        skin/classic/aero/browser/Toolbar.png                        (Toolbar-aero.png)
> >+        skin/classic/aero/browser/Toolbar.png
> >         skin/classic/aero/browser/Toolbar-inverted.png
> >+        skin/classic/aero/browser/Toolbar-aero.png
> 
> This doesn't look right. Toolbar-aero.png should be packaged as Toolbar.png,
> and then browser/themes/shared/toolbarbuttons.inc.css will take care of it
> automatically (so you can drop the rules referring to Toolbar-aero.png).

Ah, but then you'd need a different file name and CSS rule for the Win8-specific image.

Please use these kind of overrides to handle this:
http://hg.mozilla.org/mozilla-central/annotate/262e73a6b7cd/browser/themes/osx/jar.mn#l419
This will basically cause bug 702558 on Windows. It's ugly, but third-party themes can work around it. At this point I think this is the lesser evil compared to the CSS getting out of hand here.
Comment on attachment 8368480 [details] [diff] [review]
Part 5.1: Windows 8 style tabs

>+@media (-moz-os-version: windows-vista),
>+       (-moz-os-version: windows-win7) {
>+  /* Background tab separators (3px wide) for Vista and 7 */
>+  #tabbrowser-tabs[movingtab] > .tabbrowser-tab[beforeselected]:not([last-visible-tab])::after,
>+  .tabbrowser-tab:not([selected]):not([afterselected-visible]):not([afterhovered]):not([first-visible-tab]):not(:hover)::before,
>+  #tabbrowser-tabs:not([overflow]) > .tabbrowser-tab[last-visible-tab]:not([selected]):not([beforehovered]):not(:hover)::after {
>+    background-image: url(chrome://browser/skin/tabbrowser/tab-separator-aero.png);
>+  }
>+}

See comment 35.
Attachment #8368480 - Flags: review?(dao) → review-
Attachment #8368479 - Attachment is obsolete: true
Attachment #8373561 - Flags: review?(dao)
Dão, the `override` statements do not work with a condition like `os=WINNT osversion<8`, `os=WINNT osversion>5 osversion<8`, `os=WINNT osversion<=7` (these I tested).

Is this supposed to work at all on Windows? Do I need to flip a switch somewhere to get this working?
Flags: needinfo?(dao)
(In reply to Mike de Boer [:mikedeboer] from comment #38)
> Dão, the `override` statements do not work with a condition like `os=WINNT
> osversion<8`, `os=WINNT osversion>5 osversion<8`, `os=WINNT osversion<=7`
> (these I tested).
> 
> Is this supposed to work at all on Windows? Do I need to flip a switch
> somewhere to get this working?

You should use the win nt version, so 5.1 for XP, 6.2 for 8, etc.
Flags: needinfo?(dao)
Aha! Thanks Gijs!

/me slaps forehead.
Attachment #8373561 - Attachment is obsolete: true
Attachment #8373561 - Flags: review?(dao)
Attachment #8374062 - Flags: review?(dao)
Attached patch Part 5.2: Windows 8 style tabs (obsolete) — Splinter Review
Attachment #8368480 - Attachment is obsolete: true
Attachment #8374063 - Flags: review?(dao)
Attachment #8368483 - Attachment is obsolete: true
Attachment #8368483 - Flags: review?(dao)
Attachment #8374072 - Flags: review?(dao)
Attachment #8368484 - Attachment is obsolete: true
Attachment #8374103 - Flags: review?(dao)
Attachment #8374103 - Flags: review?(dao)
Will this land in Aurora as well?
Attachment #8374103 - Attachment is obsolete: true
Attachment #8374153 - Flags: review?(dao)
Attachment #8368482 - Attachment is obsolete: true
Attachment #8368482 - Flags: review?(dao)
Attachment #8374190 - Flags: review?(dao)
Will track this for now since we want to get Australis right on Windows 8.
Comment on attachment 8374062 [details] [diff] [review]
Part 1.4: Windows 8 style toolbar icons

The new Toolbar-inverted.png still seems too faint to work well on noisy lightweight theme backgrounds.
Attachment #8374062 - Flags: review?(dao) → review-
Comment on attachment 8374063 [details] [diff] [review]
Part 5.2: Windows 8 style tabs

>-        skin/classic/browser/tabbrowser/tab-separator.png            (tabbrowser/tab-separator.png)
>+        skin/classic/browser/tabbrowser/tab-separator-XPVista7.png   (tabbrowser/tab-separator-XP.png)

>-        skin/classic/aero/browser/tabbrowser/tab-separator.png       (tabbrowser/tab-separator-aero.png)
>+        skin/classic/aero/browser/tabbrowser/tab-separator.png       (tabbrowser/tab-separator.png)
>+        skin/classic/aero/browser/tabbrowser/tab-separator-XPVista7.png  (tabbrowser/tab-separator-aero.png)

>+% override chrome://browser/skin/tabbrowser/tab-separator.png    chrome://browser/skin/tabbrowser/tab-separator-XPVista7.png    os=WINNT osversion<6.2

This is pretty confusing. Can you please:

- package tab-separator-XP.png as tab-separator.png
- package tab-separator-aero.png as tab-separator-aero.png
- make the override not apply to XP
Attachment #8374063 - Flags: review?(dao) → review-
Comment on attachment 8374072 [details] [diff] [review]
Part 2.4: Windows 8 style menu-panel buttons

>+    #edit-controls@inAnyPanel@,
>+    #zoom-controls@inAnyPanel@ {
>+      border-radius: 0;
>+      height: 36px;

Why do you need to specify a height here?

>+    #edit-controls@inAnyPanel@ > toolbarbutton,
>+    #zoom-controls@inAnyPanel@ > toolbarbutton {
>+      min-width: calc(@menuPanelWidth@ / 3);
>+      max-width: calc(@menuPanelWidth@ / 3);
>+    }
>+
>+    #edit-controls@inAnyPanel@ > #copy-button,
>+    #zoom-controls@inAnyPanel@ > #zoom-reset-button {
>+      /* reduce the width with 2px for this button to compensate for two separators
>+         of 1px. */
>+      min-width: calc(@menuPanelWidth@ / 3 - 2px);
>+      max-width: calc(@menuPanelWidth@ / 3 - 2px);
>+    }

This seems out of sync with panelUIOverlay.inc.css, which uses @menuPanelButtonWidth@. I'm also concerned about this code rotting again when somebody touches panelUIOverlay.inc.css. Can the separator styling be refactored to avoid the need for this?
Attachment #8374072 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #50)
> Comment on attachment 8374062 [details] [diff] [review]
> Part 1.4: Windows 8 style toolbar icons
> 
> The new Toolbar-inverted.png still seems too faint to work well on noisy
> lightweight theme backgrounds.

He could target the second row of icons instead of the first row either by cropping the image, either by using -moz-image-region.
(In reply to Tim Nguyen [:ntim] from comment #53)
> (In reply to Dão Gottwald [:dao] from comment #50)
> > Comment on attachment 8374062 [details] [diff] [review]
> > Part 1.4: Windows 8 style toolbar icons
> > 
> > The new Toolbar-inverted.png still seems too faint to work well on noisy
> > lightweight theme backgrounds.
> 
> He could target the second row of icons instead of the first row either by
> cropping the image, either by using -moz-image-region.

That's still not good enough, as the icons have a much weaker outline than in the current Toolbar-inverted.png. I don't think we need this to be Win8-specific at all.
(In reply to Dão Gottwald [:dao] from comment #54)
> (In reply to Tim Nguyen [:ntim] from comment #53)
> > (In reply to Dão Gottwald [:dao] from comment #50)
> > > Comment on attachment 8374062 [details] [diff] [review]
> > > Part 1.4: Windows 8 style toolbar icons
> > > 
> > > The new Toolbar-inverted.png still seems too faint to work well on noisy
> > > lightweight theme backgrounds.
> > 
> > He could target the second row of icons instead of the first row either by
> > cropping the image, either by using -moz-image-region.
> 
> That's still not good enough, as the icons have a much weaker outline than
> in the current Toolbar-inverted.png. I don't think we need this to be
> Win8-specific at all.

I think it looks fine with the second row of icons in a lightweight theme.
I don't really see the problem. You might want to ask Stephen about this. I'll attach a screen right now.
Flags: needinfo?(shorlander)
Note that only the navbar has the windows 8 icons in the screenshot. The download icon doesn't use the windows 8 icon either.
Attachment #8376752 - Attachment description: Screenshot of Windows 8 icons in light theme → Screenshot of Windows 8 icons in dark lightweight theme
Flags: needinfo?(mmaslaney)
Attachment #8376752 - Attachment is obsolete: true
Looking at the Windows 8 version and comparing it with OS X with the same themes, I don't see that much of a difference in legibility. Some themes are just horrible in that respect, but that doesn't appear to be platform specific.

Dão, do you have a particular theme in mind where the Win 8 version is much worse than others?
Flags: needinfo?(dao)
(In reply to Philipp Sackl [:phlsa] from comment #59)
> Looking at the Windows 8 version and comparing it with OS X with the same
> themes, I don't see that much of a difference in legibility. Some themes are
> just horrible in that respect, but that doesn't appear to be platform
> specific.
> 
> Dão, do you have a particular theme in mind where the Win 8 version is much
> worse than others?

No particular theme, just generally themes with a dark base color (so they get inverted icons) but elements that aren't dark. Attachment 8377161 [details] is actually an example for this: The star icon seems to fade away in from of the flowers. I'm pretty sure the current Toolbar-inverted.png does a better job in such cases.
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #52)
> Comment on attachment 8374072 [details] [diff] [review]
> Part 2.4: Windows 8 style menu-panel buttons
> 
> >+    #edit-controls@inAnyPanel@ > #copy-button,
> >+    #zoom-controls@inAnyPanel@ > #zoom-reset-button {
> >+      /* reduce the width with 2px for this button to compensate for two separators
> >+         of 1px. */
> >+      min-width: calc(@menuPanelWidth@ / 3 - 2px);
> >+      max-width: calc(@menuPanelWidth@ / 3 - 2px);
> >+    }
> 
> This seems out of sync with panelUIOverlay.inc.css, which uses
> @menuPanelButtonWidth@. I'm also concerned about this code rotting again
> when somebody touches panelUIOverlay.inc.css. Can the separator styling be
> refactored to avoid the need for this?

Alternatively, since the separators for Customize / Help / Exit already use a similar style, should this change just be made across platforms in panelUIOverlay.inc.css?
If we're down to tweaking icon/color contrast levels, can we just address that in a followup?

This is a pretty big 7-part patch, a P1 for one of our primary platforms. I'm quite keen to get this landed ASAP, so that we have time to find&fix any other issues that might be found from wider testing.
Keywords: verifyme
(In reply to Justin Dolske [:Dolske] from comment #62)
> If we're down to tweaking icon/color contrast levels, can we just address
> that in a followup?

Landing what's good to go and moving the problematic parts to followup bugs such that we don't land regressions makes more sense to me.
Flags: needinfo?(mmaslaney)
Flags: needinfo?(shorlander)
(In reply to Dão Gottwald [:dao] from comment #51)
> This is pretty confusing. Can you please:
> 
> - package tab-separator-XP.png as tab-separator.png
> - package tab-separator-aero.png as tab-separator-aero.png
> - make the override not apply to XP

Dão, I tried overrides with the following conditions:
```
os=WINNT osversion>=6 osversion<6.2
os=WINNT osversion>5.3 osversion<6.2
```

and both do NOT work. Any suggestions as to how I can make this work?
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #61)
> Alternatively, since the separators for Customize / Help / Exit already use
> a similar style, should this change just be made across platforms in
> panelUIOverlay.inc.css?

Yes! But I'm not particularly fond of doing that in this bug, but instead in a follow-up bug immediately after this one. Don't you agree?
Comment on attachment 8374062 [details] [diff] [review]
Part 1.4: Windows 8 style toolbar icons

As per comment 62 and comment 64
Attachment #8374062 - Flags: review- → review?(dao)
(In reply to Mike de Boer [:mikedeboer] from comment #64)
> (In reply to Dão Gottwald [:dao] from comment #51)
> > This is pretty confusing. Can you please:
> > 
> > - package tab-separator-XP.png as tab-separator.png
> > - package tab-separator-aero.png as tab-separator-aero.png
> > - make the override not apply to XP
> 
> Dão, I tried overrides with the following conditions:
> ```
> os=WINNT osversion>=6 osversion<6.2
> os=WINNT osversion>5.3 osversion<6.2
> ```
> 
> and both do NOT work. Any suggestions as to how I can make this work?

If combinding osversion conditions doesn't work (which we should probably file a bug on), can use two overrides, one with osversion=6.0 and one with osversion=6.1?

(In reply to Mike de Boer [:mikedeboer] from comment #65)
> (In reply to Dão Gottwald [:dao] from comment #61)
> > Alternatively, since the separators for Customize / Help / Exit already use
> > a similar style, should this change just be made across platforms in
> > panelUIOverlay.inc.css?
> 
> Yes! But I'm not particularly fond of doing that in this bug, but instead in
> a follow-up bug immediately after this one. Don't you agree?

No, as letting Win8-specific CSS override the legacy base styling and alter the layout is more complicated than just changing the base styling.
Flags: needinfo?(dao)
I did not obsolete the previous patch, because I find the clarity of the unfolded overrides highly debatable. I leave it up to you to pick one of the two.
Attachment #8378943 - Flags: review?(dao)
Comment on attachment 8378943 [details] [diff] [review]
Part 1.5: Windows 8 style toolbar icons

This still seems to mess with Toolbar-inverted.png. Please drop this change and file a followup bug on introducing Win8-specific inverted icons for dark lightweight themes (although I'll say it's not clear to me what should be Win8-specific about them, given that we happily use the same inverted icons for dark lightweight themes across Windows XP, Vista and 7).
Attachment #8378943 - Flags: review?(dao) → review-
Attachment #8374062 - Attachment is obsolete: true
Attachment #8374062 - Flags: review?(dao)
(In reply to Dão Gottwald [:dao] from comment #69)
> Comment on attachment 8378943 [details] [diff] [review]
> Part 1.5: Windows 8 style toolbar icons
> 
> This still seems to mess with Toolbar-inverted.png. Please drop this change
> and file a followup bug on introducing Win8-specific inverted icons for dark
> lightweight themes (although I'll say it's not clear to me what should be
> Win8-specific about them, given that we happily use the same inverted icons
> for dark lightweight themes across Windows XP, Vista and 7).

Well, I'm just following UX's instructions, not blindly of course, and this seems to truly be what they (Stephen, Michael, Philipp) want, all things considered.
I don't feel the need to overrule their judgement, neither do I feel the need to fight you over this issue. I think this is what Justin meant with leaving possible improvements to the contrast of inverted/ non-inverted icons for a follow-up with lower priority than P1.

Needinfo-ing the people I mentioned here to get some closure here and definitive steps forward.
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
Flags: needinfo?(mmaslaney)
Flags: needinfo?(dolske)
Attachment #8374072 - Attachment is obsolete: true
Attachment #8378971 - Flags: review?(dao)
Attached patch Part 5.3: Windows 8 style tabs (obsolete) — Splinter Review
Attachment #8374063 - Attachment is obsolete: true
Attachment #8378972 - Flags: review?(dao)
Attached image Comparison of old and new white icons (obsolete) —
I could swear I commented on this bug a few hours ago, but my comment isn't here… weird.

Anyway: I looked at the issue again. Next to each other, the legibility of the old icons is slightly better. So as long as the normal icons stay Win8-specific, using the old icons for dark themes for the time being seems to be the better choice.
Flags: needinfo?(philipp)
For now can we just keep using the same general Windows Toolbar-inverted.png (https://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/Toolbar-inverted.png) and file a follow-up for investigating Windows 8 specific icons.

Thanks!
Flags: needinfo?(shorlander)
Flags: needinfo?(mmaslaney)
Flags: needinfo?(dolske)
Comment on attachment 8378972 [details] [diff] [review]
Part 5.3: Windows 8 style tabs

This looks good code-wise, but it seems that the new tab-separator.png won't work with dark high-contrast settings or dark lightweight themes, as it consists only of black pixels at different opacity levels. All other tab-separator images have a light outline to deal with different backgrounds. I'd suggest making tab-separator.png a copy of tab-separator-aero.png for now and filing a bug on replacing it with a proper Win8-specific image.
Attachment #8378972 - Flags: review?(dao)
Attachment #8378972 - Attachment is obsolete: true
Attachment #8379027 - Flags: review?(dao)
Attachment #8378943 - Attachment is obsolete: true
Attachment #8379028 - Flags: review?(dao)
Comment on attachment 8378971 [details] [diff] [review]
Part 2.5: Windows 8 style menu-panel buttons

>--- a/browser/themes/shared/customizableui/panelUIOverlay.inc.css
>+++ b/browser/themes/shared/customizableui/panelUIOverlay.inc.css

>-#edit-controls > separator,
>-#zoom-controls > separator {
>+.toolbaritem-combined-buttons > separator {
>   -moz-appearance: none;
>   width: 3px;
>   -moz-box-align: stretch;
>   background-image: linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 40%, hsla(0,0%,100%,.3) 60%, hsla(0,0%,100%,0)),
>                     linear-gradient(to bottom, hsla(210,54%,20%,0), hsla(210,54%,20%,.15) 40%, hsla(210,54%,20%,.15) 60%, hsla(210,54%,20%,0)),
>                     linear-gradient(to bottom, hsla(0,0%,100%,0), hsla(0,0%,100%,.3) 40%, hsla(0,0%,100%,.3) 60%, hsla(0,0%,100%,0));
>   background-size: 1px, 1px, 1px;
>   background-position: 0 0, 1px 0, 2px 0;
>   background-repeat: no-repeat;
> }

This looks like it belongs in browser.css...
Attachment #8378971 - Flags: review?(dao) → review+
Attachment #8379027 - Flags: review?(dao) → review+
Comment on attachment 8379028 [details] [diff] [review]
Part 1.6: Windows 8 style toolbar icons

> @media (-moz-windows-theme: luna-silver) {
>   :-moz-any(@primaryToolbarButtons@),
>+  #home-button.bookmark-item,
>   #bookmarks-menu-button.toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {
>     list-style-image: url("chrome://browser/skin/Toolbar-lunaSilver.png");
>   }

Does http://hg.mozilla.org/mozilla-central/annotate/cf9666cc3f36/browser/themes/windows/browser.css#l785 not override this?
(In reply to Dão Gottwald [:dao] from comment #79)
> Does
> http://hg.mozilla.org/mozilla-central/annotate/cf9666cc3f36/browser/themes/
> windows/browser.css#l785 not override this?

Yes, it does! I fixed it in this patch.
Attachment #8379028 - Attachment is obsolete: true
Attachment #8379028 - Flags: review?(dao)
Attachment #8379046 - Flags: review?(dao)
Attachment #8379046 - Flags: review?(dao) → review+
Whiteboard: [Australis:M?][Australis:P1] → [Australis:P1][leave open]
Attachment #8377159 - Attachment is obsolete: true
Attachment #8377161 - Attachment is obsolete: true
Attachment #8378985 - Attachment is obsolete: true
Attachment #8374153 - Flags: review?(dao) → review?(mak77)
Comment on attachment 8374190 [details] [diff] [review]
Part 7.2: Windows 8 style toolbars

>-@media (-moz-windows-default-theme) {
>+@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
>+       (-moz-windows-default-theme) and (-moz-os-version: windows-win8) {
>   #navigator-toolbox > toolbar:not(:-moz-lwtheme),
>   #browser-bottombox:not(:-moz-lwtheme),
>   .browserContainer > findbar {
>     background-color: @customToolbarColor@;
>   }

windows-win8 should be windows-win7, except that this was already part of another patch, so it should just go away here.

>+/* Win8 and beyond. */
>+@media not all and (-moz-os-version: windows-vista) {
>+  @media not all and (-moz-os-version: windows-win7) {
>+    #urlbar:not(:-moz-lwtheme),
>+    .searchbar-textbox:not(:-moz-lwtheme) {
>+      background-color: hsla(0,0%,100%,.9);
>+      border: 1px solid transparent;
>+      border-color: hsla(210,54%,20%,.25) hsla(210,54%,20%,.27) hsla(210,54%,20%,.3);
>+      box-shadow: 0 1px 0 hsla(0,0%,0%,.01) inset,
>+                  0 1px 0 hsla(0,0%,100%,.1);
>+      transition-property: margin-left, border-color, background-color;
>+      transition-duration: 200ms;
>+    }
>+
>+    #urlbar:not(:-moz-lwtheme)[focused],
>+    .searchbar-textbox:not(:-moz-lwtheme)[focused] {
>+      background-color: hsla(0,0%,100%,1);
>+      border-color: #4595e5;
>+    }
>+
>+    /* Introducing an additional hover state for the Bookmark button */
>+    #nav-bar .toolbarbutton-1[buttonover] > .toolbarbutton-menubutton-button:hover > .toolbarbutton-icon {
>+      background-color: hsla(210,4%,10%,.08);
>+      -moz-border-end: 1px solid;
>+      -moz-padding-end: 5px;
>+      border-color: hsla(210,4%,10%,.1);
>+    }
>+
>+    /* Introducing a panel opened state */
>+    #nav-bar .toolbarbutton-1[open]:not([disabled]) > .toolbarbutton-icon,
>+    #nav-bar .toolbarbutton-1[open]:not([disabled]) > .toolbarbutton-text,
>+    #nav-bar .toolbarbutton-1[open]:not([disabled]) > .toolbarbutton-badge-container,
>+    #nav-bar .toolbarbutton-1[open] > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon {
>+      background-color: #177ee5;
>+      border-color: hsla(210,80%,20%,.1);
>+      box-shadow: 0 1px 0 0 hsla(210,80%,20%,.1) inset;
>+    }
>+
>+    #nav-bar :-moz-any(@primaryToolbarButtons@)[open]:not([disabled]),
>+    #nav-bar #bookmarks-menu-button[open] > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon {
>+      list-style-image: url(chrome://browser/skin/Toolbar-inverted.png);
>+    }

Are you sure this doesn't inadequately overload Toolbar-inverted.png? This is quite a different context from where we're otherwise using those icons.

Should this whole block really apply regardless of the theme, e.g. also for high-contrast and classic settings? Not sure how well those are hidden under Windows 8, but they're still available, right?

Forking the button styling for Windows 8 (whereas e.g. XP and 7 manage to share the same styling) makes me sad from a maintenance point of view. Any chance that we can unify this stuff again?
Attachment #8374190 - Flags: review?(dao) → review-
Comment on attachment 8374153 [details] [diff] [review]
Part 3.3: Windows 8 style downloads panel

Review of attachment 8374153 [details] [diff] [review]:
-----------------------------------------------------------------

Well, it's mostly questions at the moment, I'm trying to figure which decisions were taken for the theme.

I applied the patch, the result looks pointing towards the mockup, but it's very different.
The padding/margin of the richlistitems is totally off, the icons are still the old ones (with the rounded outline), there is no separator between the entry and the button in the right as well as they are a single entity.
What's the expected result here, is the plan to just update the selection style?
I figure there is a lot to change to match the mockup, though I can't evaluate if we reached the expected goal without knowing what that is. From my point of view we should at least fix the selection style (as you almost did here) and the margin/padding. The internal details that are more complex since they require deep changes to the binding can be left apart, as well as the icons that need new graphic assets.

Moving to what is here, the selection style looks visually correct, though the mockup has a transition on mousedown, what I see is an immediate color change... Could you please get in touch with shorlander and clarify what's expected?
When I mouseover entries, the upper border of the second item is blurry while in the mockup all of the borders are very sharp, looks like the selection border fights with the richlistitems separator. it was an issue even with the previous style, though since the selection was light blue it was barely noticeable, while here is jumping to my eyes.

::: browser/themes/windows/downloads/allDownloadsViewOverlay-aero.css
@@ +6,5 @@
>  %include allDownloadsViewOverlay.css
>  %undef WINDOWS_AERO
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {

Just by comparing the Library and Explorer, the selection style doesn't seem to fit, it looks like windows XP selection. (fwiw, the existing selection is also different than the win8 style, not sure if something regressed it, we used to have a better style there, iirc.)
And it is totally different from any other view in the Library...

I don't think we should touch the Library view in this patch, there should be patch to make all of it consistent at a maximum, not just changing a part of it, let's keep it coherent.
I'd limit this patch to the panel, for which we have a mockup that seems to fit in the OS style, so please revert the changes to this overlay style.

::: browser/themes/windows/downloads/downloads.css
@@ +37,5 @@
> +
> +%ifdef WINDOWS_AERO
> +/* < Win8 */
> +@media (-moz-os-version: windows-vista),
> +       (-moz-os-version: windows-win7) {

I don't think the comment is adding anything useful to the self-documenting media query... you may remove all of those < win8 comments, imo.

@@ +51,5 @@
> +}
> +
> +#downloadsHistory:hover:active {
> +  box-shadow: none;
> +}

why not just moving these rules to downloads-aero.css then and set them only for win8?
since any other platform should be fine as it was, looks like we should just need overriding rules in the -aero.css file for win8 that will change transition-duration, hover and hover:active styles

@@ +229,5 @@
> +/* < Win8 */
> +@media (-moz-os-version: windows-vista),
> +       (-moz-os-version: windows-win7) {
> +%endif
> +#downloadsPanel:not([keyfocus]) > #downloadsListBox > richlistitem[type="download"][state="1"][exists]:hover {

why are we making other OS versions overwrite win8, rather than the opposite, as I suggested above? Is there some specific decision taken for themes to do so?
Attachment #8374153 - Flags: review?(mak77)
(In reply to Dão Gottwald [:dao] from comment #86)
> Should this whole block really apply regardless of the theme, e.g. also for
> high-contrast and classic settings? Not sure how well those are hidden under
> Windows 8, but they're still available, right?
High contrast themes are accessible within Customization windows, just like Windows 7 has it. Classic style, however, is gone. If there is option to enable it, it's hidden and common user won't find that easily, if at all.
Should I fill follow-ups for those things or should it be done here ?
- Restyle .panel-multiview-anchor (as shown in the mockup)
- Darken icons on hover to match the mockup
- Windows 8 style tab scrollbox, new tab, list all tabs icon (ux-consistency)
- Lighter toolbar color to match the mockup (#F8F8F8)
- Windows 8 style menu button separator (solid separator according to mockup)
- Remove border-radius for customization mode buttons
Note for darkening icons :
if you don't want to include more graphics or don't want to set specific image regions for darker graphics, you might want to use SVG filters.
I also found another issue, the switch to metro button doesn't have the windows 8 style look.
Tim, this is great feedback. With the amount of patches in this bug and the fact that some still haven't landed, I think you should wait until this bug is closed and then file a new bug for these other issues.
(In reply to Tim Nguyen [:ntim] from comment #90)
> - Windows 8 style menu button separator (solid separator according to mockup)

We consciously decided against this for all platforms because it looks bad when you enable the bookmarks toolbar.
Blocks: 978003
Any ETA when all this will land on Aurora channel?
(In reply to Aris from comment #95)
> Any ETA when all this will land on Aurora channel?

Probably when all the patches land on Nightly. There are still 2 patches left still in review.
(In reply to Marco Bonardo [:mak] from comment #87)
> why are we making other OS versions overwrite win8, rather than the
> opposite, as I suggested above? Is there some specific decision taken for
> themes to do so?

This is the way we've been implementing the design since bug 859751. Since what Stephen shows in his interactive mockups is what Fx will look like from Windows 8 onward, it makes sense to implement the design as the new default (bottom-up, if you will), contrary to implementing it as exceptions to the past (top-down). This way it'll be easier to keep updating the windows theme. I don't know to what degree we support user styles, but this method also makes it easier for theme builders to change things.
(In reply to Mike de Boer [:mikedeboer] from comment #97)
> This is the way we've been implementing the design since bug 859751. Since
> what Stephen shows in his interactive mockups is what Fx will look like from
> Windows 8 onward, it makes sense to implement the design as the new default
> (bottom-up, if you will), contrary to implementing it as exceptions to the
> past (top-down). This way it'll be easier to keep updating the windows
> theme. I don't know to what degree we support user styles, but this method
> also makes it easier for theme builders to change things.

So, how should we use -aero.css files, just to overwrite rules for Vista/7 glass? Are we pointing towards their removal?
Btw, please fix the other applicable comments, then I will try to review it again with that decision in mind.
(In reply to Marco Bonardo [:mak] from comment #98)
> So, how should we use -aero.css files, just to overwrite rules for Vista/7
> glass?

Well, yes, indeed! Don't you think that it's rather difficult to keep thinking of Win XP as the default and considering newer versions as exceptions? Treating newer Windows versions as exceptions, style-wise, sounds rather negative to me, whereas they are generally perceived as better looking, are they not?

> Are we pointing towards their removal?

I don't know if it's reasonable to turn that ship around, but I'd definitely vote aye.

> Btw, please fix the other applicable comments, then I will try to review it
> again with that decision in mind.

I will asap, thanks for those!
(In reply to Mike de Boer [:mikedeboer] from comment #97)
> This is the way we've been implementing the design since bug 859751. Since
> what Stephen shows in his interactive mockups is what Fx will look like from
> Windows 8 onward

I really think the download panel that is shown in the mockup is the one to be used on every platform (the same panel is shown on the OSX mockup FWIW).
Depends on: 978767
No longer blocks: 978003
Depends on: 978003
Comment on attachment 8368485 [details] [diff] [review]
Part 4.1: Windows 8 style Places Organizer

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Library window not looking like Windows 8 native.
Testing completed (on m-c, etc.): baked on m-c for a couple of days.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a
Attachment #8368485 - Flags: approval-mozilla-aurora?
Comment on attachment 8369385 [details] [diff] [review]
Part 6.2: Windows 8 style main window

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Firefox main window not looking like Windows 8 native.
Testing completed (on m-c, etc.): baked on m-c for a couple of days.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a
Attachment #8369385 - Flags: approval-mozilla-aurora?
Comment on attachment 8378971 [details] [diff] [review]
Part 2.5: Windows 8 style menu-panel buttons

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Menu panel buttons without square edged buttons on Windows 8
Testing completed (on m-c, etc.): baked on m-c for a couple of days.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a
Attachment #8378971 - Flags: approval-mozilla-aurora?
Comment on attachment 8379027 [details] [diff] [review]
Part 5.4: Windows 8 style tabs

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Australis tabs not blending with the main window correctly on Windows 8
Testing completed (on m-c, etc.): baked on m-c for a couple of days.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a
Attachment #8379027 - Flags: approval-mozilla-aurora?
Comment on attachment 8379046 [details] [diff] [review]
Part 1.7: Windows 8 style toolbar icons

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: No nice Windows 8 style icons!
Testing completed (on m-c, etc.): baked on m-c for a couple of days.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a
Attachment #8379046 - Flags: approval-mozilla-aurora?
Attachment #8368485 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8369385 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8378971 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8379027 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8379046 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Dão, I removed the blue opened state, after I discussed it with Stephen.

Now I have a different problem though: the unified back/ forward button clip paths do not apply nicely anymore. I *think* this has to do with the fact that I use borders now instead of ONLY box-shadows, but that didn't lead me to a fix.
The down state does use a box-shadow on top.

Do you have any ideas how I can fix this?
Attachment #8379046 - Attachment is obsolete: true
Attachment #8386378 - Flags: feedback?(dao)
I'm pretty sure it's the use of borders. I'm trying to switch to box-shadow.
A few oddities:

In customize mode, the bottom border of the content area sticks to the bottom of the window instead of shrinking back.

The shadow behind the tabs disappears in customize mode and re-appears when out of it.  The shadow isn't necessary / shouldn't be there for either.
The min/max-restore/close buttons are also non-native.
Stephen, can you take a look at the SVG clip path for the forward-button?
Attachment #8374190 - Attachment is obsolete: true
Attachment #8386378 - Attachment is obsolete: true
Attachment #8386378 - Flags: feedback?(dao)
Attachment #8386964 - Flags: ui-review?(shorlander)
(In reply to Mike de Boer [:mikedeboer] from comment #111)
> Created attachment 8386964 [details] [diff] [review]
> Part 7.3: Windows 8 style toolbars
> 
> Stephen, can you take a look at the SVG clip path for the forward-button?

Here is the path I used to fix the gap:

<svg:clipPath id="windows-keyhole-forward-clip-path" clipPathUnits="objectBoundingBox">
  <svg:path d="m 0, 0 c .3, .25 .3, .75, 0, 1 l 1 0, 0 -1  z"/>
</svg:clipPath>
Attachment #8386964 - Attachment is obsolete: true
Attachment #8386964 - Flags: ui-review?(shorlander)
Attachment #8387046 - Flags: review?(dao)
Attachment #8374153 - Attachment is obsolete: true
Attachment #8387048 - Flags: review?(mak77)
Comment on attachment 8387048 [details] [diff] [review]
Part 3.4: Windows 8 style downloads panel

Review of attachment 8387048 [details] [diff] [review]:
-----------------------------------------------------------------

issues found while testing this:
- when the panel is empty there is only the Show all downloads part. While before this was looking like a link, now there is a brief hover effect, but it's so subtle that I can barely tell something is happening. Likely causethe background is white, not grey. We could apply an opaque background in this case, though the arrow will stay white and that may look bad. May you figure with Stephen what's expected in this case?
- for whatever reason I don't see any hover styling on download items :/

apart these the patch looks ok, but I'm not sure why I can't see the hover off-hand

::: browser/themes/windows/downloads/downloads.css
@@ +21,5 @@
>  #downloadsHistory {
> +  background-color: transparent;
> +  transition-duration: 150ms;
> +  transition-property: background-color;
> +  color: ButtonText;

is this needed? doesn't look like there's something else setting a color, and the background is not a button bgcolor

@@ +237,5 @@
>  }
>  
> +#downloadsPanel:not([keyfocus]) > #downloadsListBox > richlistitem[type="download"][state="1"][exists]:hover:active {
> +  background-color: Highlight;
> +  box-shadow: none;

should you also set outline: 0 here?
Attachment #8387048 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] from comment #115)
> Comment on attachment 8387048 [details] [diff] [review]
> Part 3.4: Windows 8 style downloads panel
> 
> Review of attachment 8387048 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> issues found while testing this:
> - when the panel is empty there is only the Show all downloads part. While
> before this was looking like a link, now there is a brief hover effect, but
> it's so subtle that I can barely tell something is happening. Likely
> causethe background is white, not grey. We could apply an opaque background
> in this case, though the arrow will stay white and that may look bad. May
> you figure with Stephen what's expected in this case?

I will!

> - for whatever reason I don't see any hover styling on download items :/

I'm not going to introduce them here. The downloads panel is slated for a UX overhaul anyways, so I'm leaving things be as much as possible here.
Attachment #8387048 - Attachment is obsolete: true
Attachment #8387693 - Flags: review?(mak77)
Comment on attachment 8387046 [details] [diff] [review]
Part 7.4: Windows 8 style toolbars

Over to jaws so he and Mike can bang this out today.
Attachment #8387046 - Flags: review?(dao) → review?(jaws)
Comment on attachment 8387693 [details] [diff] [review]
Part 3.5: Windows 8 style downloads panel

...ditto!
Attachment #8387693 - Flags: review?(mak77) → review?(jaws)
Comment on attachment 8387046 [details] [diff] [review]
Part 7.4: Windows 8 style toolbars

Review of attachment 8387046 [details] [diff] [review]:
-----------------------------------------------------------------

this would have been r+ but the vista/7 part wasn't tested due to the syntax error. we need to test the vista/7 changes before this can land.

::: browser/themes/windows/browser-aero.css
@@ +327,5 @@
> +      border: 1px solid transparent;
> +      border-color: hsla(210,54%,20%,.25) hsla(210,54%,20%,.27) hsla(210,54%,20%,.3);
> +      box-shadow: 0 1px 0 hsla(0,0%,0%,.01) inset,
> +                  0 1px 0 hsla(0,0%,100%,.1);
> +      transition-property: margin-left, border-color, background-color;

need to remove the "margin-left, "

@@ +342,5 @@
> +    #nav-bar .toolbarbutton-1[buttonover] > .toolbarbutton-menubutton-button:hover > .toolbarbutton-icon {
> +      background-color: hsla(210,4%,10%,.08);
> +      -moz-border-end: 1px solid;
> +      -moz-padding-end: 5px;
> +      border-color: hsla(210,4%,10%,.1);

there is something wrong with the border color at the beginning of this transition. it appears near black (or -moz-use-text-color?). 

i looked in to this and it wasn't obvious as to why this was creeping in. we can fix this in a follow-up so we don't hold up the rest of this patch for it.

::: browser/themes/windows/browser.css
@@ +551,5 @@
> +  #nav-bar .toolbarbutton-1 > .toolbarbutton-icon,
> +  #nav-bar .toolbarbutton-1 > .toolbarbutton-text,
> +  #nav-bar .toolbarbutton-1 > .toolbarbutton-badge-container,
> +  #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> +    background-color: hsla(210,32%,93%,0);

it looks like you need a curly bracket here :)
Attachment #8387046 - Flags: review?(jaws) → review-
Comment on attachment 8387693 [details] [diff] [review]
Part 3.5: Windows 8 style downloads panel

Review of attachment 8387693 [details] [diff] [review]:
-----------------------------------------------------------------

this looks good, nice.
Attachment #8387693 - Flags: review?(jaws) → review+
I landed an encoding fixup (revealed by problems with cross compilation):

https://hg.mozilla.org/integration/mozilla-inbound/rev/b97c09e3de3d
Comment on attachment 8387693 [details] [diff] [review]
Part 3.5: Windows 8 style downloads panel

Checked-in as: https://hg.mozilla.org/integration/fx-team/rev/a18c5fa023cb
Attachment #8387693 - Flags: checkin+
Attachment #8387046 - Attachment is obsolete: true
Attachment #8388667 - Flags: review?(jaws)
Attachment #8388667 - Flags: review?(jaws)
Comment on attachment 8388667 [details] [diff] [review]
Part 7.5: Windows 8 style toolbars

Review of attachment 8388667 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.xul
@@ +1165,5 @@
>  #include tab-shape.inc.svg
>  
>  #ifndef XP_UNIX
>      <svg:clipPath id="windows-keyhole-forward-clip-path" clipPathUnits="objectBoundingBox">
> +      <svg:path d="m 0, 0 c .3, .25 .3, .75, 0, 1 l 1 0, 0 -1  z"/>

nit please remove spaces between x,y coordinates.
m 0,0 c .3,.25 .3,.75 0,1 l 1,0 0,-l z

::: browser/themes/windows/browser-aero.css
@@ +342,5 @@
> +    #nav-bar .toolbarbutton-1[buttonover] > .toolbarbutton-menubutton-button:hover > .toolbarbutton-icon {
> +      background-color: hsla(210,4%,10%,.08);
> +      -moz-border-end: 1px solid;
> +      -moz-padding-end: 5px;
> +      border-color: hsla(210,4%,10%,.1);

this still has the issue about a dark border color being shown at the start of the transition. please file a follow-up bug about it, we should fix that but i don't want to hold up the rest of this patch for that one issue.

::: browser/themes/windows/browser.css
@@ +544,4 @@
>    border: 1px solid;
> +  border-color: transparent;
> +  transition-property: background-color, border-color;
> +  transition-duration: 250ms;

why are we changing the transition to 250ms from 150ms? we use 150ms on linux now too, and 150ms on <Win8. please change this back to 150ms.
Attachment #8388667 - Flags: review?(jaws) → review+
I landed your patch for you on fx-team so that it has a chance of making tomorrow's Nightly build.

https://hg.mozilla.org/integration/fx-team/rev/ece9cdfb15d7
Whiteboard: [Australis:P1][leave open] → [Australis:P1][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a18c5fa023cb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P1][fixed-in-fx-team] → [Australis:P1]
Target Milestone: --- → Firefox 30
(In reply to :Gijs Kruitbosch from comment #129)
> https://hg.mozilla.org/mozilla-central/rev/a18c5fa023cb

Oh blegh, this cset was here already, but the bug wasn't resolved because the *other* cset hasn't yet made it to m-c. Sigh.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Whiteboard: [Australis:P1] → [Australis:P1][fixed-in-fx-team]
Target Milestone: Firefox 30 → ---
Comment on attachment 8387693 [details] [diff] [review]
Part 3.5: Windows 8 style downloads panel

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: The downloads panel will look a bit alien to Windows 8 users.
Testing completed (on m-c, etc.): baked on m-c for a day.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a
Attachment #8387693 - Flags: approval-mozilla-aurora?
Attachment #8387693 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [Australis:P1][fixed-in-fx-team] → [Australis:P1]
Whiteboard: [Australis:P1] → [Australis:P1][fixed-in-fx-team]
Comment on attachment 8388667 [details] [diff] [review]
Part 7.5: Windows 8 style toolbars

*trying to fix the screwiness that bugzilla was yesterday, this should have r=jaws*
Attachment #8388667 - Flags: review?(jaws)
https://hg.mozilla.org/mozilla-central/rev/ece9cdfb15d7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P1][fixed-in-fx-team] → [Australis:P1]
Target Milestone: --- → Firefox 30
Comment on attachment 8388667 [details] [diff] [review]
Part 7.5: Windows 8 style toolbars

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: The look of buttons on the nav-bar currently have the Windows Vista/ 7 look. This patch changes that for Windows 8 users.
Testing completed (on m-c, etc.): baked on m-c for a day.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a
Attachment #8388667 - Flags: approval-mozilla-aurora?
Attachment #8388667 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to Aurora as: https://hg.mozilla.org/releases/mozilla-aurora/rev/c43a49abc397

Thanks to all for your tremendous amount of help on this bug!
Depends on: theme-win8
Depends on: 989449
Marking issue verified as all of it's dependencies are verified fixed except for the issues described in the meta bug 982644.

Tested on Windows 8 64bit and on a MS Pro 2 device running Windows 8.1 64bit using:
- latest Aurora, build ID: 20140416004008
- Fx 29 beta 8, build ID: 20140414143035.
Status: RESOLVED → VERIFIED
Keywords: verifyme
All the traditional apps (non metro non ribbon) buttons in windows 8 use a blue background and border on hover, and have a more saturated state on click. The current patch (1) no color, (2) border turns lighter on click (3) extra inset shadow on click instead. 1 is arguably for consistency with australis but 2 and 3 are not found in australis nor Windows itself. Could these be fine tuned? It's a bit jarring with all due respect. On nightly 21 Apr.
(In reply to henryfhchan from comment #138)
> All the traditional apps (non metro non ribbon) buttons in windows 8 use a
> blue background and border on hover, and have a more saturated state on
> click. The current patch (1) no color, (2) border turns lighter on click (3)
> extra inset shadow on click instead. 1 is arguably for consistency with
> australis but 2 and 3 are not found in australis nor Windows itself. Could
> these be fine tuned? It's a bit jarring with all due respect. On nightly 21
> Apr.

Please file a new bug for this, ideally with some illustrative screenshots. Thank you!
Depends on: 998157
Attachment #8379046 - Attachment is obsolete: false
Depends on: 1100435
No longer depends on: 1100435
Depends on: 1121782
You need to log in before you can comment on or make changes to this bug.