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

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Theme
--
minor
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: MattN, Assigned: mconley)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
Firefox 28
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M6], URL)

Attachments

(2 attachments, 5 obsolete attachments)

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

4 years ago
(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);
}
(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).
(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
Whiteboard: [Australis:M4]
(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.
Flags: needinfo?(shorlander)
Whiteboard: [Australis:M4] → [Australis:M5]
Created attachment 752460 [details] [diff] [review]
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.
Flags: needinfo?(shorlander)
(Assignee)

Comment 6

4 years ago
Are we OK changing these icons across all XUL apps, or should this be Firefox specific?
Flags: needinfo?(shorlander)
(Assignee)

Comment 7

4 years ago
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.
Flags: needinfo?(shorlander)
(Assignee)

Comment 8

4 years ago
Created attachment 752936 [details] [diff] [review]
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.
Assignee: shorlander → mconley
(Assignee)

Comment 9

4 years ago
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.
Attachment #752936 - Attachment description: WIP Patch 1 → Patch v1.0
Attachment #752936 - Flags: review?(dao)
> > 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.
(Assignee)

Comment 11

4 years ago
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.
Attachment #752936 - Flags: review?(dao)
(Assignee)

Comment 12

4 years ago
Created attachment 752981 [details] [diff] [review]
WIP Patch 2

Using a new close-icon class so that we don't have to repeat the luna-blue/olive/silver block everywhere.
Attachment #752936 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 752984 [details] [diff] [review]
WIP Patch 3
Attachment #752981 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
So far this patch only does Windows. I'll get it to do OSX tomorrow.
Duplicate of this bug: 854794
(Assignee)

Updated

4 years ago
Whiteboard: [Australis:M5] → [Australis:M6]
(Assignee)

Comment 16

4 years ago
Created attachment 753404 [details] [diff] [review]
Patch v1

I *think* I got them all. Going to do a self-review before requesting review.
Attachment #752984 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
Created attachment 753422 [details] [diff] [review]
Patch v1.1

Forgot some OSX theme stuff under toolkit. Did a final sweep for icons/close.png and close@2x.png, 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.
Attachment #753404 - Attachment is obsolete: true
Attachment #753422 - Flags: review?(dao)
(Assignee)

Comment 18

4 years ago
Gentle review ping.
(Assignee)

Comment 19

4 years ago
Slightly more forceful review ping.
(Assignee)

Updated

4 years ago
Attachment #753422 - Flags: review?(mnoorenberghe+bmo)
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.
Attachment #753422 - Flags: review?(mnoorenberghe+bmo)
Attachment #753422 - Flags: review?(dao)
Attachment #753422 - Flags: review+
(Assignee)

Updated

4 years ago
Blocks: 879921
(Assignee)

Comment 21

4 years ago
Created attachment 758721 [details] [diff] [review]
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.
Attachment #753422 - Attachment is obsolete: true
Attachment #758721 - Flags: review+

Comment 22

4 years ago
Pushed: https://hg.mozilla.org/projects/ux/rev/546ccc10a18f
Whiteboard: [Australis:M6] → [Australis:M6][fixed-in-ux]
This may have caused regression on bug 649216.

Comment 24

4 years ago
(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? :-(
(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).
(Assignee)

Comment 26

4 years ago
(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
Done. Filed bug 880277.

Comment 28

4 years ago
https://hg.mozilla.org/mozilla-central/rev/546ccc10a18f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
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".'
Keywords: addon-compat

Updated

3 years ago
Depends on: 1000051
You need to log in before you can comment on or make changes to this bug.