Closed Bug 851001 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Theme, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: MattN, Assigned: mconley)

References

()

Details

(Keywords: addon-compat, Whiteboard: [Australis:M6])

Attachments

(2 files, 5 obsolete files)

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?
(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]
Attached patch Close Icons - 01Splinter Review
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)
Are we OK changing these icons across all XUL apps, or should this be Firefox specific?
Flags: needinfo?(shorlander)
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)
Attached patch Patch v1.0 (obsolete) — Splinter Review
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
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.
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)
Attached patch WIP Patch 2 (obsolete) — Splinter Review
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
Attached patch WIP Patch 3 (obsolete) — Splinter Review
Attachment #752981 - Attachment is obsolete: true
So far this patch only does Windows. I'll get it to do OSX tomorrow.
Duplicate of this bug: 854794
Whiteboard: [Australis:M5] → [Australis:M6]
Attached patch Patch v1 (obsolete) — Splinter Review
I *think* I got them all. Going to do a self-review before requesting review.
Attachment #752984 - Attachment is obsolete: true
Attached patch Patch v1.1 (obsolete) — Splinter Review
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)
Slightly more forceful review ping.
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+
(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+
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.
(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).
(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.
https://hg.mozilla.org/mozilla-central/rev/546ccc10a18f
Status: ASSIGNED → RESOLVED
Closed: 6 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
Depends on: 1000051
You need to log in before you can comment on or make changes to this bug.