851001 - Update global/icons/close[@2x].png for Australis

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

1) There is a discrepancy between mockups for the close button on Windows (see bug 738491 comment 117)
2) OS X mockups don't include @2x versions
3) OS X close buttons aren't a drop-in replacement for global/icons/close.png because of different spacing.  Can we get a new version that uses the same spacing as the current version to avoid breaking addons?
4) Linux currently picks up the gtk image (moz-icon://stock/gtk-close?size=menu). Are we going to switch to our own image?

Comment 1

[Frank Yan (:fryn)]

(In reply to Matthew N. [:MattN] from comment #0)
> 3) OS X close buttons aren't a drop-in replacement for
> global/icons/close.png because of different spacing.  Can we get a new
> version that uses the same spacing as the current version to avoid breaking
> addons?

I think it's okay if they have different spacing if we add them as a new file instead of replacing global/icons/close.png, i.e.:
.tab-close-button {
  list-style-image: url(some/path/that/is/not/global/icons/close.png);
}

Comment 2

[Michael Monreal [:micmon]]

(In reply to Matthew N. [:MattN] from comment #0)
> 4) Linux currently picks up the gtk image
> (moz-icon://stock/gtk-close?size=menu). Are we going to switch to our own
> image?

I really hope the gtk-close will NOT be used. Even GNOME apps do not use this icon anymore (they use a very simple mono color "x" which changes from gray to black on mouse over instead of showing a button outline).

Comment 3

[Stephen Horlander [:shorlander]]

(In reply to Matthew N. [:MattN] from comment #0)
> 1) There is a discrepancy between mockups for the close button on Windows
> (see bug 738491 comment 117)

Yes, I can update them all to be the same.

> 2) OS X mockups don't include @2x versions
> 3) OS X close buttons aren't a drop-in replacement for
> global/icons/close.png because of different spacing.  Can we get a new
> version that uses the same spacing as the current version to avoid breaking
> addons?

I don't think anything needs to change here. I will double check but I don't think the spacing will need to change either.

> 4) Linux currently picks up the gtk image
> (moz-icon://stock/gtk-close?size=menu). Are we going to switch to our own
> image?

Yes we need a custom close icon for Linux

Comment 4

[Matthew N. [:MattN]]

(In reply to Stephen Horlander from comment #3)
> (In reply to Matthew N. [:MattN] from comment #0)
> > 1) There is a discrepancy between mockups for the close button on Windows
> > (see bug 738491 comment 117)
> 
> Yes, I can update them all to be the same.

Have you decided which one of the two versions you would like to use?

> > 2) OS X mockups don't include @2x versions
> > 3) OS X close buttons aren't a drop-in replacement for
> > global/icons/close.png because of different spacing.  Can we get a new
> > version that uses the same spacing as the current version to avoid breaking
> > addons?
> 
> I don't think anything needs to change here. I will double check but I don't
> think the spacing will need to change either.

I'm not sure what you mean.  Are you saying that the current (pre-Australis) OS X close buttons are fine? If we are changing them, could you provide a 2x version?

> > 4) Linux currently picks up the gtk image
> > (moz-icon://stock/gtk-close?size=menu). Are we going to switch to our own
> > image?
> 
> Yes we need a custom close icon for Linux

Have we figured out what we're doing for Linux? Are we going with the SVG from the spec? If so, could you add the other state(s) to the image?

Thanks.

Comment 5

[Stephen Horlander [:shorlander]]

Attachment: Close Icons - 01

This patch just adds the new and updates images.


(In reply to Matthew N. [:MattN] from comment #4)
> Have you decided which one of the two versions you would like to use?

Putting them in this patch.


> I'm not sure what you mean.  Are you saying that the current (pre-Australis)
> OS X close buttons are fine? If we are changing them, could you provide a 2x
> version?

Made a slight tweak to the OS X image. The current @2x is fine.


> Have we figured out what we're doing for Linux? Are we going with the SVG
> from the spec? If so, could you add the other state(s) to the image?

I am ok trying the SVG. I will create the rest of the states and add them later.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Are we OK changing these icons across all XUL apps, or should this be Firefox specific?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Just talked to shorlander in IRC - yes this is toolkit-wide, and intentionally so. We're updating the toolkit close icons to be in sync with our theme changes.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.0

This adds the new assets and rules for Aero, and Luna (Blue, Olive Silver).

I'll do OSX next and then request review.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 752936 [details] [diff] [review]
Patch v1.0

The OSX graphic is just a drop-in replacement, so I didn't need to do anything with the CSS. Seems to look alright.

Comment 10

[Matthew N. [:MattN]]

> > Have we figured out what we're doing for Linux? Are we going with the SVG
> > from the spec? If so, could you add the other state(s) to the image?
> 
> I am ok trying the SVG. I will create the rest of the states and add them
> later.

mconley: note that Linux will require CSS changes because the icon will likely be smaller than the gtk one.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 752936 [details] [diff] [review]
Patch v1.0

We also want to use these icons in a few other places - not just in tabs.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 2

Using a new close-icon class so that we don't have to repeat the luna-blue/olive/silver block everywhere.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 3

Comment 14

[Mike Conley (:mconley) (:⚙️)]

So far this patch only does Windows. I'll get it to do OSX tomorrow.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

I *think* I got them all. Going to do a self-review before requesting review.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1

Forgot some OSX theme stuff under toolkit. Did a final sweep for icons/close.png and [e-mail], and I think I got them all.

The only exception seems to be in toolkit/themes/*/plugins/pluginProblems.css. In here, we've got a closeIcon class that doesn't conform to the list-style-image + -moz-image-region approach that we're using with close-icon, so I've left it. Let me know if that's not OK.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Gentle review ping.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Slightly more forceful review ping.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 753422 [details] [diff] [review]
Patch v1.1

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

The pluginProblem.css close icon is specially designed for the click-to-play overlay, so it is good to leave that as is.

I don't see any linux icon changes in this patch. Let's move that to a follow-up, as I don't see the SVG close-icon in http://people.mozilla.com/~shorlander/files/australis-linux-svg-test/australis-liveDemo-linux.html.

::: browser/themes/windows/browser.css
@@ -1643,1 @@
>  .tab-close-button:hover[selected="true"] {

Should these be left here or removed? I don't know why we'd remove the rule for background tabs but leave the one for selected tabs.

::: toolkit/themes/osx/global/global.css
@@ +340,5 @@
> +/* :::::: Close button icons ::::: */
> +
> +.close-icon {
> +  -moz-image-region: rect(0, 16px, 16px, 0);
> +  list-style-image: url("chrome://global/skin/icons/close.png");

nit: swap the order here, putting the list-style-image rule first.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.2 (r+'d by jaws)

(In reply to Jared Wein [:jaws] from comment #20)
> Comment on attachment 753422 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 753422 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The pluginProblem.css close icon is specially designed for the click-to-play
> overlay, so it is good to leave that as is.
> 
> I don't see any linux icon changes in this patch. Let's move that to a
> follow-up, as I don't see the SVG close-icon in
> http://people.mozilla.com/~shorlander/files/australis-linux-svg-test/
> australis-liveDemo-linux.html.

Yep, good call. I've filed follow-up bug 879921.

> 
> ::: browser/themes/windows/browser.css
> @@ -1643,1 @@
> >  .tab-close-button:hover[selected="true"] {
> 
> Should these be left here or removed? I don't know why we'd remove the rule
> for background tabs but leave the one for selected tabs.
> 

Removed! Good eyes.

> ::: toolkit/themes/osx/global/global.css
> @@ +340,5 @@
> > +/* :::::: Close button icons ::::: */
> > +
> > +.close-icon {
> > +  -moz-image-region: rect(0, 16px, 16px, 0);
> > +  list-style-image: url("chrome://global/skin/icons/close.png");
> 
> nit: swap the order here, putting the list-style-image rule first.

Done - did it for Windows too.

Comment 22

[:Gijs (he/him)]

Pushed: https://hg.mozilla.org/projects/ux/rev/546ccc10a18f

Comment 23

[u428464]

This may have caused regression on bug 649216.

Comment 24

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #23)
> This may have caused regression on bug 649216.

Are you sure it's this and not the massive merge changeset right before it which seems to have broken all kinds of other things? :-(

Comment 25

[u428464]

(In reply to :Gijs Kruitbosch from comment #24)
> (In reply to Guillaume C. [:ge3k0s] from comment #23)
> > This may have caused regression on bug 649216.
> 
> Are you sure it's this and not the massive merge changeset right before it
> which seems to have broken all kinds of other things? :-(

You may be right, but it's UX branch specific though (the behavior is still correct in nightly).

Comment 26

[Mike Conley (:mconley) (:⚙️)]

(In reply to Guillaume C. [:ge3k0s] from comment #25)
> (In reply to :Gijs Kruitbosch from comment #24)
> > (In reply to Guillaume C. [:ge3k0s] from comment #23)
> > > This may have caused regression on bug 649216.
> > 
> > Are you sure it's this and not the massive merge changeset right before it
> > which seems to have broken all kinds of other things? :-(
> 
> You may be right, but it's UX branch specific though (the behavior is still
> correct in nightly).

Guillaume:

Can you please file a separate bug for this issue, blocking australis-tabs, with [Australis:M?] in the whiteboard? And please include detailed steps to reproduce.

Thanks,

-Mike

Comment 27

[u428464]

Done. Filed bug 880277.

Comment 28

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/546ccc10a18f

Comment 29

[Matthew N. [:MattN]]

The class attribute change broke Tree Style Tabs so I've added it to https://developer.mozilla.org/en-US/Firefox/australis-add-on-compat-draft

  'The class attribute on tab close buttons has been changed. Extensions shouldn't be relying on the class attribute value since it is a list of tokens and should instead look for the anonid attribute with value "close-button".'