462967 - Update full screen window controls on windows theme

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

The window controls (minimize, restore down, close) are currently just using black and white gif images.  We are having new artwork created to refresh these icons on XP and Vista for 3.1.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Window controls for XP

Let's place this file in /source/toolkit/themes/winstripe/global/icons/

Note that we need to continue to package the previous gif images in case any extensions are using them.

For XP the minimize and restore down images should still be placed on buttons, however the close images should not (since it designed to look like it is itself a button).

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Forgot to note that the rows are:

normal
hover
hit

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Window controls for Vista

In this case all of the images should appear on their own (as opposed to being placed on buttons) since they draw the button appearance.

Also note that the close button column image region is 16x17, while for xp all of the icons were just 16x16.

The images also need to be placed so that they appear in a direct row without any padding between them, and pushed against the top of the window.

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Comment 5

[Dão Gottwald [:dao]]

So why should these images go into toolkit? The whole fullscreen UI seems to be implemented in and controlled by browser code.

Comment 6

[Dão Gottwald [:dao]]

I tested this on XP and Vista, and I don't think this is the styling that Alex had in mind. So you should probably get this ui-reviewed.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Dao: do you have screenshots that you can show us and explain why you think it's not what Alex expects?

Comment 8

[Dão Gottwald [:dao]]

There's a tryserver build:

https://build.mozilla.org/tryserver-builds/[e-mail]-try-9d19d149d88/try-9d19d149d88-win32.zip

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: yeah, this is wrong

Oh, I see.

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

>So why should these images go into toolkit? The whole fullscreen UI seems to be
>implemented in and controlled by browser code.

Similar to the print preview icons I don't really have a strong opinion, but generic window controls seemed very toolkit-ish, also aren't the current window controls in toolkit?

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

>Created an attachment (id=380295) [details]
>yeah, this is wrong

Yeah, so on Vista we just want to use the images to give the presentation of buttons.  Also the top of the images should touch the top of the screen.

On XP this only applies to the stop button.  We couldn't provide a button appearance for the minimize and restore down images since we don't know if they should be tan, silver, olive, etc.

Do we want to use the old style gifs when not using the default windows theme?  A related issue is bug 429571 dealing with high contrast mode.

Comment 12

[Dão Gottwald [:dao]]

OK, let's keep them in toolkit. I can see how an app that's not firefox or a firefox derivation could want to use these icons, even if there's no full-screen UI tool in toolkit. SeaMonkey, for instance, but there could even be non-browser apps.

(In reply to comment #11)
> Do we want to use the old style gifs when not using the default windows theme? 
> A related issue is bug 429571 dealing with high contrast mode.

Given bug 429571, I'm pretty sure the answer is "no". The new icons seem to work better in that regard.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 380168 [details] [diff] [review]
Patch (v1)

>+#minimize-button, #restore-button, #close-button {

nit: line break after comma

Comment 14

[(no longer active)]

Attachment: Patch (v2)

OK, needed to set -moz-appearance to none.  Also addressed the nit in comment 13.

Comment 15

[Dão Gottwald [:dao]]

This doesn't implement the very last aspect of comment 3, does it?

Comment 16

[(no longer active)]

(In reply to comment #15)
> This doesn't implement the very last aspect of comment 3, does it?

You mean images appearing without any space in between them and pushed against the top?  It does implement that.

Comment 17

[Dão Gottwald [:dao]]

Yeah. Looks strange on XP though, probably because of the top toolbar border. The border for #window-controls also seems wrong, especially on Vista.

Comment 18

[Dão Gottwald [:dao]]

Tryserver build: https://build.mozilla.org/tryserver-builds/[e-mail]-try-9599a112e4d/try-9599a112e4d-win32.zip

Comment 19

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 380447 [details] [diff] [review]
Patch (v2)

overall better than what we currently have, so I don't want to hold up landing.  However if you do have time, there are two small issues, I'll attach screenshots.

Comment 20

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Two minor issues with the v2

-the tan color we selected for XP works well with the default themes (blue, silver, olive), but looks a bit disabled with classic.  consider using the gif images in that case
-for XP default, the close button should line up with restore down and minimize

Comment 21

[(no longer active)]

Attachment: Patch (v3)

Addressed both points in comment 20.

I'll attach a screenshot of the behavior of the patch on 4 configurations shortly.

Comment 22

[(no longer active)]

Attachment: Screenshot of the new patch

This shows a screenshot of Vista Classic, Vista Aero, XP Classic and XP Luna with this patch.

Comment 23

[Alex Faaborg [:faaborg] (Firefox UX)]

looks great

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 380712 [details] [diff] [review]
Patch (v3)

windows-classic isn't the right tool for this (uses the icons that assume a luna background for third-party themes, and maintains bug 429571 for classic), and unfortunately windows-default-theme isn't either (maintains bug 429571 for classic and third-party themes). This is also really unnecessary for Vista, where the new icons work on all backgrounds. So I think we should fix the new XP icons to better work on different backgrounds as well.

Comment 25

[Alex Faaborg [:faaborg] (Firefox UX)]

We can solve bug bug 429571 the same way we solved all of the others, by adding some white outlines to the legacy GIF images.  That plus using windows default for the icons that are designed for the default windows themes should have us covered.

>unnecessary for Vista,where the new icons work on all backgrounds.

Aero style window controls in classic mode is usable, but it will look like a bug (since the window controls use simple black glyphs everywhere else in the OS and app).

Comment 26

[Dão Gottwald [:dao]]

It will be somewhat consistent, at least. Attachment 380713 [details] looks frankensteinesque with the slightly smaller and rounded close button next to the black glyphs.

Comment 27

[Dão Gottwald [:dao]]

Attachment: alternative windowControls.png

This should work better on different backgrounds. It won't look identical to attachment 380125 [details] with Luna, because I obviously haven't spent a lot of time on it, although I think it will look equally good.

Comment 28

[Dão Gottwald [:dao]]

And I still think the border left to the controls doesn't fit, at least on Vista.

Comment 29

[Alex Faaborg [:faaborg] (Firefox UX)]

We can clean up the border in a follow up bug

Comment 30

[Dão Gottwald [:dao]]

It's one simple style rule. And it's quite likely that we won't have time for follow-ups on 1.9.1.

Comment 31

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm more worried that we aren't going to be able to update the fullscreen icons for our aero and luna themes at all if we don't get this checked in today, but if you want to change the line that's fine as well, it will improve the appearance.

Comment 32

[Dão Gottwald [:dao]]

Alex, any problem with attachment 380759 [details]?

Ehsan, is there a way for you to finish this today?

Comment 33

[Alex Faaborg [:faaborg] (Firefox UX)]

well, it was designed for general purpose use, which isn't the goal with our luna specific icons.  Also, it doesn't quiet fit with classic, which uses solid black glyphs.

Comment 34

[Dão Gottwald [:dao]]

It's designed to fit with Luna, really, but so that the glyphs don't fade away with different themes.

I actually can't recall a place where we have icons purely for Luna, and different ones for Classic.

Comment 35

[Dão Gottwald [:dao]]

Attachment: patch v4

this is what I think we should do

Comment 36

[Alex Faaborg [:faaborg] (Firefox UX)]

Let's get this checked in and follow up in firefox.next if we get better os theme detection.

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

I honestly can't tell what's going on in this bug. Attachment 380713 [details] looked great to me, while I understand Dao's concerns with third-party themes (not because I think we need to be beholden to them, but because I think that we declared ourselves compat-frozen at b3 and switching this on them now at the last minute is kind of a bad thing for us to do) but haven't seen a screenshot of what attachment 380926 [details] [diff] [review] looks like.

Very doubtful I'll approve anything here for 1.9.1 without seeing that, first.

Comment 38

[Dão Gottwald [:dao]]

Attachment: screenshot

tryserver build: https://build.mozilla.org/tryserver-builds/[e-mail]-try-f7afd035fcf/try-f7afd035fcf-win32.zip

Comment 39

[Dão Gottwald [:dao]]

Attachment: more screenshots

this is attachment 380713 [details] in comparison

Comment 40

[Dão Gottwald [:dao]]

Attachment: patch v4.1

This is the patch that the tryserver build was actually built with, and that I used for the screenshots. v4 had minor alignment issues on XP.

Comment 41

[Dão Gottwald [:dao]]

interdiff from v4 to v4.1:
https://bugzilla.mozilla.org/attachment.cgi?oldid=380926&action=interdiff&newid=381027&headers=1

not going to ask for another review for this

Comment 42

[Dão Gottwald [:dao]]

Attachment: windowControls.png source file

don't want to keep this on my desktop, so attaching it here

Comment 43

[(no longer active)]

Dão is practically driving this, sorry I couldn't keep up the pace.

Comment 44

[u88484]

Why do the tooltips look different between the before and after screenshots?  Are the before screenshots from 3.0.* and the afer from a latest branch/trunk build and not from the latest branch/trunk with and without the patch applied ( or images replaced)?

Comment 45

[Dão Gottwald [:dao]]

Are you referring to attachment 381026 [details]?
The left side is from Ehsan and the right side is from me. So these are different machines and different Windows installations. That said, the tooltips look the same to me. In any case, no patch in this bug touched them.

Comment 46

[u88484]

(In reply to comment #45)
> Are you referring to attachment 381026 [details]?
> The left side is from Ehsan and the right side is from me. So these are
> different machines and different Windows installations. That said, the tooltips
> look the same to me. In any case, no patch in this bug touched them.
Yeah, that attachment.  The border looks a little thicker around some of the tooltips and the font just a little bolder and/or not as sharp on some of them.  Just wanted to make sure nothing screwy was happening here with a patch.

Comment 47

[Alex Faaborg [:faaborg] (Firefox UX)]

beltzner: screen shots in attachment 381026 [details], the left side is patch v3, the right side is patch v4.  Both are better than what we currently have.

Comment 48

[Mike Beltzner [:beltzner, not reading bugmail]]

Told Dao that we should try this on trunk, first. a=beltzner for mozilla-central only.

Comment 49

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/ca5376dae3ca

Comment 50

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 381027 [details] [diff] [review]
patch v4.1

a191=beltzner

Comment 51

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2175a36f4456

Comment 52

[Asher]

Attachment: Button hover effect is too large

It looks like the button is larger than the image in Aero mode, or at least the hover effect is too large?  Looks visually awkward.  Screenshot attached.

Comment 53

[Dão Gottwald [:dao]]

WTF... I have no idea why hg applied the patch this way without notifying me.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4f750271194b