Closed
Bug 960517
Opened 11 years ago
Closed 11 years ago
Adjust Australis's browser theme for Windows 8 and up
Categories
(Firefox :: Theme, defect)
Tracking
()
VERIFIED
FIXED
Firefox 30
People
(Reporter: mikedeboer, Assigned: mikedeboer)
References
()
Details
(Whiteboard: [Australis:P1])
Attachments
(7 files, 36 obsolete files)
(deleted),
patch
|
dao
:
review+
Sylvestre
:
approval-mozilla-aurora+
dao
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dao
:
review+
Sylvestre
:
approval-mozilla-aurora+
dao
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dao
:
review+
Sylvestre
:
approval-mozilla-aurora+
dao
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dao
:
review+
Sylvestre
:
approval-mozilla-aurora+
dao
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dao
:
review+
Sylvestre
:
approval-mozilla-aurora+
dao
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jaws
:
review+
Sylvestre
:
approval-mozilla-aurora+
mikedeboer
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jaws
:
review+
Sylvestre
:
approval-mozilla-aurora+
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.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #8361043 -
Flags: review?(dao)
Assignee | ||
Comment 2•11 years ago
|
||
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...
Comment 3•11 years ago
|
||
(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.
Assignee | ||
Comment 4•11 years ago
|
||
Gosh Tim, you're right!
Attachment #8361043 -
Attachment is obsolete: true
Attachment #8361043 -
Flags: review?(dao)
Attachment #8363156 -
Flags: review?(dao)
Comment 5•11 years ago
|
||
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)
Comment 6•11 years ago
|
||
Also, make sure that the assets you updated include the panorama icon :
check bug 888601 .
Assignee | ||
Comment 7•11 years ago
|
||
I will chop the patch up into byte-size chunks coming Monday. Stay tuned ;)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #8363156 -
Attachment is obsolete: true
Attachment #8363156 -
Flags: review?(dao)
Attachment #8367386 -
Flags: review?(dao)
Comment 9•11 years ago
|
||
(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.
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #8367434 -
Flags: review?(dao)
Assignee | ||
Comment 11•11 years ago
|
||
forgot to hg add a file.
Attachment #8367434 -
Attachment is obsolete: true
Attachment #8367434 -
Flags: review?(dao)
Attachment #8367830 -
Flags: review?(dao)
Assignee | ||
Comment 12•11 years ago
|
||
Unbitrotted.
Attachment #8367386 -
Attachment is obsolete: true
Attachment #8367386 -
Flags: review?(dao)
Attachment #8367835 -
Flags: review?(dao)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8367836 -
Flags: review?(dao)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #8367841 -
Flags: review?(dao)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #8367853 -
Flags: review?(dao)
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #8367856 -
Flags: review?(dao)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8367864 -
Flags: review?(dao)
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #8367830 -
Attachment is obsolete: true
Attachment #8367830 -
Flags: review?(dao)
Attachment #8367866 -
Flags: review?(dao)
Comment 19•11 years ago
|
||
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 20•11 years ago
|
||
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-
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #8367835 -
Attachment is obsolete: true
Attachment #8367835 -
Flags: review?(dao)
Attachment #8368479 -
Flags: review?(dao)
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #8367853 -
Attachment is obsolete: true
Attachment #8368480 -
Flags: review?(dao)
Assignee | ||
Comment 23•11 years ago
|
||
Attachment #8367856 -
Attachment is obsolete: true
Attachment #8367856 -
Flags: review?(dao)
Attachment #8368481 -
Flags: review?(dao)
Assignee | ||
Comment 24•11 years ago
|
||
Attachment #8367864 -
Attachment is obsolete: true
Attachment #8367864 -
Flags: review?(dao)
Attachment #8368482 -
Flags: review?(dao)
Assignee | ||
Comment 25•11 years ago
|
||
Attachment #8367866 -
Attachment is obsolete: true
Attachment #8367866 -
Flags: review?(dao)
Attachment #8368483 -
Flags: review?(dao)
Assignee | ||
Comment 26•11 years ago
|
||
Attachment #8367836 -
Attachment is obsolete: true
Attachment #8367836 -
Flags: review?(dao)
Attachment #8368484 -
Flags: review?(dao)
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #8367841 -
Attachment is obsolete: true
Attachment #8367841 -
Flags: review?(dao)
Attachment #8368485 -
Flags: review?(dao)
Comment 28•11 years ago
|
||
Comment on attachment 8368481 [details] [diff] [review]
Part 6.1: Windows 8 style main window
see comment 19
Attachment #8368481 -
Flags: review?(dao)
Updated•11 years ago
|
Attachment #8368485 -
Flags: review?(dao) → review+
Assignee | ||
Comment 29•11 years ago
|
||
(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 30•11 years ago
|
||
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-
Comment 31•11 years ago
|
||
(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).
Assignee | ||
Comment 32•11 years ago
|
||
Attachment #8368481 -
Attachment is obsolete: true
Attachment #8369385 -
Flags: review?(dao)
Updated•11 years ago
|
Attachment #8369385 -
Flags: review?(dao) → review+
Updated•11 years ago
|
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M?][Australis:P1]
Comment 33•11 years ago
|
||
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 34•11 years ago
|
||
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-
Comment 35•11 years ago
|
||
(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 36•11 years ago
|
||
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-
Assignee | ||
Comment 37•11 years ago
|
||
Attachment #8368479 -
Attachment is obsolete: true
Attachment #8373561 -
Flags: review?(dao)
Assignee | ||
Comment 38•11 years ago
|
||
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)
Comment 39•11 years ago
|
||
(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)
Assignee | ||
Comment 40•11 years ago
|
||
Aha! Thanks Gijs!
/me slaps forehead.
Assignee | ||
Comment 41•11 years ago
|
||
Attachment #8373561 -
Attachment is obsolete: true
Attachment #8373561 -
Flags: review?(dao)
Attachment #8374062 -
Flags: review?(dao)
Assignee | ||
Comment 42•11 years ago
|
||
Attachment #8368480 -
Attachment is obsolete: true
Attachment #8374063 -
Flags: review?(dao)
Assignee | ||
Comment 43•11 years ago
|
||
Attachment #8368483 -
Attachment is obsolete: true
Attachment #8368483 -
Flags: review?(dao)
Attachment #8374072 -
Flags: review?(dao)
Assignee | ||
Comment 44•11 years ago
|
||
Attachment #8368484 -
Attachment is obsolete: true
Attachment #8374103 -
Flags: review?(dao)
Assignee | ||
Updated•11 years ago
|
Attachment #8374103 -
Flags: review?(dao)
Comment 45•11 years ago
|
||
Will this land in Aurora as well?
Comment 46•11 years ago
|
||
Yes.
Assignee | ||
Comment 47•11 years ago
|
||
Attachment #8374103 -
Attachment is obsolete: true
Attachment #8374153 -
Flags: review?(dao)
Assignee | ||
Comment 48•11 years ago
|
||
Attachment #8368482 -
Attachment is obsolete: true
Attachment #8368482 -
Flags: review?(dao)
Attachment #8374190 -
Flags: review?(dao)
Updated•11 years ago
|
Comment 49•11 years ago
|
||
Will track this for now since we want to get Australis right on Windows 8.
Comment 50•11 years ago
|
||
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 51•11 years ago
|
||
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 52•11 years ago
|
||
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-
Comment 53•11 years ago
|
||
(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.
Comment 54•11 years ago
|
||
(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.
Comment 55•11 years ago
|
||
(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)
Comment 56•11 years ago
|
||
Note that only the navbar has the windows 8 icons in the screenshot. The download icon doesn't use the windows 8 icon either.
Updated•11 years ago
|
Attachment #8376752 -
Attachment description: Screenshot of Windows 8 icons in light theme → Screenshot of Windows 8 icons in dark lightweight theme
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(mmaslaney)
Assignee | ||
Comment 57•11 years ago
|
||
Assignee | ||
Comment 58•11 years ago
|
||
Attachment #8376752 -
Attachment is obsolete: true
Comment 59•11 years ago
|
||
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)
Comment 60•11 years ago
|
||
(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)
Comment 61•11 years ago
|
||
(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?
Comment 62•11 years ago
|
||
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.
Comment 63•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(mmaslaney)
Updated•11 years ago
|
Flags: needinfo?(shorlander)
Assignee | ||
Comment 64•11 years ago
|
||
(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?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(dao)
Assignee | ||
Comment 65•11 years ago
|
||
(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?
Assignee | ||
Comment 66•11 years ago
|
||
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)
Comment 67•11 years ago
|
||
(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)
Assignee | ||
Comment 68•11 years ago
|
||
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 69•11 years ago
|
||
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-
Updated•11 years ago
|
Attachment #8374062 -
Attachment is obsolete: true
Attachment #8374062 -
Flags: review?(dao)
Assignee | ||
Comment 70•11 years ago
|
||
(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)
Assignee | ||
Comment 71•11 years ago
|
||
Attachment #8374072 -
Attachment is obsolete: true
Attachment #8378971 -
Flags: review?(dao)
Assignee | ||
Comment 72•11 years ago
|
||
Attachment #8374063 -
Attachment is obsolete: true
Attachment #8378972 -
Flags: review?(dao)
Comment 73•11 years ago
|
||
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)
Comment 74•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(dolske)
Comment 75•11 years ago
|
||
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)
Assignee | ||
Comment 76•11 years ago
|
||
Attachment #8378972 -
Attachment is obsolete: true
Attachment #8379027 -
Flags: review?(dao)
Assignee | ||
Comment 77•11 years ago
|
||
Attachment #8378943 -
Attachment is obsolete: true
Attachment #8379028 -
Flags: review?(dao)
Comment 78•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8379027 -
Flags: review?(dao) → review+
Comment 79•11 years ago
|
||
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?
Assignee | ||
Comment 80•11 years ago
|
||
(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)
Updated•11 years ago
|
Attachment #8379046 -
Flags: review?(dao) → review+
Comment 81•11 years ago
|
||
Comment on attachment 8379027 [details] [diff] [review]
Part 5.4: Windows 8 style tabs
https://hg.mozilla.org/integration/fx-team/rev/0a5a8c27afd7
Attachment #8379027 -
Flags: checkin+
Comment 82•11 years ago
|
||
Comment on attachment 8369385 [details] [diff] [review]
Part 6.2: Windows 8 style main window
https://hg.mozilla.org/integration/fx-team/rev/6c09cd5ce4c9
Attachment #8369385 -
Flags: checkin+
Comment 83•11 years ago
|
||
Comment on attachment 8368485 [details] [diff] [review]
Part 4.1: Windows 8 style Places Organizer
https://hg.mozilla.org/integration/fx-team/rev/eecb30167ca4
Attachment #8368485 -
Flags: checkin+
Comment 84•11 years ago
|
||
Comment on attachment 8378971 [details] [diff] [review]
Part 2.5: Windows 8 style menu-panel buttons
https://hg.mozilla.org/integration/fx-team/rev/88ee67ba1b10
Attachment #8378971 -
Flags: checkin+
Comment 85•11 years ago
|
||
Comment on attachment 8379046 [details] [diff] [review]
Part 1.7: Windows 8 style toolbar icons
https://hg.mozilla.org/integration/fx-team/rev/6fc68b9a7a98
Attachment #8379046 -
Flags: checkin+
Updated•11 years ago
|
Whiteboard: [Australis:M?][Australis:P1] → [Australis:P1][leave open]
Updated•11 years ago
|
Attachment #8377159 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8377161 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8378985 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8374153 -
Flags: review?(dao) → review?(mak77)
Comment 86•11 years ago
|
||
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 87•11 years ago
|
||
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)
Comment 88•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6fc68b9a7a98
https://hg.mozilla.org/mozilla-central/rev/88ee67ba1b10
https://hg.mozilla.org/mozilla-central/rev/eecb30167ca4
https://hg.mozilla.org/mozilla-central/rev/0a5a8c27afd7
https://hg.mozilla.org/mozilla-central/rev/6c09cd5ce4c9
Mike de Boer (mdeboer@mozilla.com)??? Please fix your hg commit information :)
Comment 89•11 years ago
|
||
(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.
Comment 90•11 years ago
|
||
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
Comment 91•11 years ago
|
||
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.
Comment 92•11 years ago
|
||
I also found another issue, the switch to metro button doesn't have the windows 8 style look.
Comment 93•11 years ago
|
||
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.
Comment 94•11 years ago
|
||
(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.
Comment 95•11 years ago
|
||
Any ETA when all this will land on Aurora channel?
Comment 96•11 years ago
|
||
(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.
Assignee | ||
Comment 97•11 years ago
|
||
(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.
Comment 98•11 years ago
|
||
(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.
Assignee | ||
Comment 99•11 years ago
|
||
(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!
Comment 100•11 years ago
|
||
(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).
Updated•11 years ago
|
Assignee | ||
Comment 101•11 years ago
|
||
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?
Assignee | ||
Comment 102•11 years ago
|
||
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?
Assignee | ||
Comment 103•11 years ago
|
||
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?
Assignee | ||
Comment 104•11 years ago
|
||
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?
Assignee | ||
Comment 105•11 years ago
|
||
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?
Updated•11 years ago
|
status-firefox29:
--- → affected
tracking-firefox29:
--- → +
Updated•11 years ago
|
Attachment #8368485 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #8369385 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #8378971 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #8379027 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #8379046 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 106•11 years ago
|
||
Pushed the approved patches to Aurora:
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/d6b7ebc6f4d8
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/03c5b809110b
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/8e41458bd06a
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/bbcfad841808
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/40b8aade9424
Assignee | ||
Comment 107•11 years ago
|
||
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)
Assignee | ||
Comment 108•11 years ago
|
||
I'm pretty sure it's the use of borders. I'm trying to switch to box-shadow.
Comment 109•11 years ago
|
||
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.
Comment 110•11 years ago
|
||
The min/max-restore/close buttons are also non-native.
Assignee | ||
Comment 111•11 years ago
|
||
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)
Comment 112•11 years ago
|
||
(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>
Assignee | ||
Comment 113•11 years ago
|
||
Attachment #8386964 -
Attachment is obsolete: true
Attachment #8386964 -
Flags: ui-review?(shorlander)
Attachment #8387046 -
Flags: review?(dao)
Assignee | ||
Comment 114•11 years ago
|
||
Attachment #8374153 -
Attachment is obsolete: true
Attachment #8387048 -
Flags: review?(mak77)
Comment 115•11 years ago
|
||
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)
Assignee | ||
Comment 116•11 years ago
|
||
(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.
Assignee | ||
Comment 117•11 years ago
|
||
Attachment #8387048 -
Attachment is obsolete: true
Attachment #8387693 -
Flags: review?(mak77)
Comment 118•11 years ago
|
||
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 119•11 years ago
|
||
Comment on attachment 8387693 [details] [diff] [review]
Part 3.5: Windows 8 style downloads panel
...ditto!
Attachment #8387693 -
Flags: review?(mak77) → review?(jaws)
Comment 120•11 years ago
|
||
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 121•11 years ago
|
||
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+
Comment 122•11 years ago
|
||
I landed an encoding fixup (revealed by problems with cross compilation):
https://hg.mozilla.org/integration/mozilla-inbound/rev/b97c09e3de3d
Assignee | ||
Comment 123•11 years ago
|
||
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+
Assignee | ||
Comment 124•11 years ago
|
||
Attachment #8387046 -
Attachment is obsolete: true
Attachment #8388667 -
Flags: review?(jaws)
Assignee | ||
Updated•11 years ago
|
Attachment #8388667 -
Flags: review?(jaws)
Comment 125•11 years ago
|
||
Comment 126•11 years ago
|
||
Comment 127•11 years ago
|
||
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+
Comment 128•11 years ago
|
||
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
Updated•11 years ago
|
Whiteboard: [Australis:P1][leave open] → [Australis:P1][fixed-in-fx-team]
Comment 129•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P1][fixed-in-fx-team] → [Australis:P1]
Target Milestone: --- → Firefox 30
Comment 130•11 years ago
|
||
(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.
Updated•11 years ago
|
Status: REOPENED → ASSIGNED
Whiteboard: [Australis:P1] → [Australis:P1][fixed-in-fx-team]
Target Milestone: Firefox 30 → ---
Assignee | ||
Comment 131•11 years ago
|
||
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?
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #8387693 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 132•11 years ago
|
||
Updated•11 years ago
|
Whiteboard: [Australis:P1][fixed-in-fx-team] → [Australis:P1]
Assignee | ||
Updated•11 years ago
|
Updated•11 years ago
|
Whiteboard: [Australis:P1] → [Australis:P1][fixed-in-fx-team]
Comment 133•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8388667 -
Flags: checkin+
Comment 134•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P1][fixed-in-fx-team] → [Australis:P1]
Target Milestone: --- → Firefox 30
Assignee | ||
Comment 135•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #8388667 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 136•11 years ago
|
||
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!
Updated•11 years ago
|
Depends on: theme-win8
Comment 137•11 years ago
|
||
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
Comment 138•11 years ago
|
||
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.
Comment 139•11 years ago
|
||
(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!
Updated•11 years ago
|
Attachment #8379046 -
Attachment is obsolete: false
You need to log in
before you can comment on or make changes to this bug.
Description
•