589219 - [OS X] Update Close Buttons Appearance

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement)

Description

[Stephen Horlander [:shorlander]]

Attachment: New Tab Close Buttons Style

Update Mac tab close icons.

Comment 1

[Anthony Ricaud (:rik)]

I'd like to try to provide my first Firefox patch with this. Can we have the PSD files used for the screenshot ?

Comment 2

[Markus Stange [:mstange]]

I found one instance of the new close button in http://svn.mozilla.org/design/projects/newtheme/firelight/i06/Firefox-4-Mockup-i06-(OSX)-(TabsBottom)-(TabOverflow).psd but that only has the hover state.

Anthony, when you fix this, can you please put all states into the same image and use -moz-image-region to switch between them? The current buttons use separate images, so they flash on first mouseover / mousedown, and it annoys me to no end...

Stephen, what should happen to the close buttons of doorhangers, notification bars and the find bar? Should they keep using the old image?

Comment 3

[Stephen Horlander [:shorlander]]

(In reply to comment #2)
> Stephen, what should happen to the close buttons of doorhangers, notification
> bars and the find bar? Should they keep using the old image?

We should use the same style for all the close buttons. Also we use the close icon in Panorama and for sidebars. 

Anthony: give me a bit and I will provide images. Not sure if we can use the same image for all these elements though.

Comment 4

[Dominic Spitaler]

Attachment: Tab Close Button v1 – 12x16px (no CSS)

Did that one some months ago. I made two versions; one that changes the CSS to move the new button-image to the correct position and one that reduces the width of the png to do the same.

Comment 5

[Dominic Spitaler]

Attachment: Tab Close Button v1 – 16x16px (with CSS)

Comment 6

[Anthony Ricaud (:rik)]

So I have a working version here (with images by Maurice Svay). It's really cool :)

I've tried to use it on the add-on bar and it feels weird to me. We may need a fourth state. Light as the first state but with a border like the hover and active states.

Comment 7

[Frank Yan (:fryn)]

Comment on attachment 510635 [details] [diff] [review]
Tab Close Button v1 – 12x16px (no CSS)

Shorlander, could you ui-review one of these two patches, if we want the updated close buttons?

Comment 8

[Stephen Horlander [:shorlander]]

Comment on attachment 510635 [details] [diff] [review]
Tab Close Button v1 – 12x16px (no CSS)

As per comment 3 this bug needs to morph a bit into updating all of the places we use this icon at the same time.  Will followup with a screenshot and image files for the various contexts.

Comment 9

[Stephen Horlander [:shorlander]]

Attachment: Close icons in various locations

All the instances where we use the closetab icon should be updated to be use consistent icons with contextually specific styling:

- Grey chrome: tabs, toolbars, etc
- Blue sidebars
- HUD style panels
- Panorama (much like bug 596412 for Windows)

Attaching sprites for each. Tweaked Dominic's icons for this. Thanks Dominic!

Comment 10

[Stephen Horlander [:shorlander]]

Attachment: Close Icon: Tabs/Toolbars

Comment 11

[Stephen Horlander [:shorlander]]

Attachment: Close Icon: Sidebars

Comment 12

[Stephen Horlander [:shorlander]]

Attachment: Close Icon: Panels

Comment 13

[Stephen Horlander [:shorlander]]

Attachment: Close Icon: Panorama

Comment 14

[Stephen Horlander [:shorlander]]

(In reply to comment #6)
> I've tried to use it on the add-on bar and it feels weird to me. We may need a
> fourth state. Light as the first state but with a border like the hover and
> active states.

It doesn't seem that strange to me. It seems a little strange with the findbar active as well but that isn't unique to changing the icons.

Comment 15

[Anthony Ricaud (:rik)]

Attachment: Patch

Let's go for the first patch.

Everything works as expected to me, except the active state for the panorama close buttons.

Comment 16

[Stephen Horlander [:shorlander]]

Comment on attachment 510825 [details] [diff] [review]
Patch

(In reply to comment #15)
> Everything works as expected to me, except the active state for the panorama
> close buttons.

Looks great! Thanks for doing this.

The Panorama active state thing is a known issue with the way it does click handling. Should be a separate bug for that. Which I forgot to file :)

ui-r: me

Requesting review from Dão.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 510825 [details] [diff] [review]
Patch

> /* Tabstrip close button */
> .tabs-closebutton {
>   -moz-padding-end: 4px;
>-  list-style-image: url("chrome://global/skin/icons/closetab.png");
>+  list-style-image: url("chrome://global/skin/icons/close-sidebar.png");
>   border: none;
>+  -moz-image-region: rect(0, 16px, 16px, 0);
> }

You need to use #sidebar-header > .tabs-closebutton here, as .tabs-closebutton is used in different contexts.

> toolkit/themes/pinstripe/global/icons/close-panel.png

Should be:
toolkit/themes/pinstripe/global/notification/close.png

> toolkit/themes/pinstripe/global/icons/close-panorama.png

Should be:
browser/themes/pinstripe/browser/tabview/close.png

> toolkit/themes/pinstripe/global/icons/close-tab-toolbar.png

You can call this one just close.png.

Comment 18

[Anthony Ricaud (:rik)]

(In reply to comment #17)
> Comment on attachment 510825 [details] [diff] [review]
> Patch
> 
> > /* Tabstrip close button */
> > .tabs-closebutton {
> >   -moz-padding-end: 4px;
> >-  list-style-image: url("chrome://global/skin/icons/closetab.png");
> >+  list-style-image: url("chrome://global/skin/icons/close-sidebar.png");
> >   border: none;
> >+  -moz-image-region: rect(0, 16px, 16px, 0);
> > }
> 
> You need to use #sidebar-header > .tabs-closebutton here, as .tabs-closebutton
> is used in different contexts.

Should I just change the selector or add another rule just for the images?

Comment 19

[Dão Gottwald [:dao]]

.tabs-closebutton targets various different close buttons, so you need to add another rule.

Comment 20

[Anthony Ricaud (:rik)]

OK so I should also not remove the closetab.png from jar.mn since other close buttons will still use it, right?

Comment 21

[Anthony Ricaud (:rik)]

Attachment: Patch v2

Comment 22

[Frank Yan (:fryn)]

Comment on attachment 511075 [details] [diff] [review]
Patch v2

This looks good to me. Dão, could you give this a quick look? It's straight-forward list-style-image and -moz-image-region code.

It does increase the number of close buttons images we use, but each image is  visually tailored for its surrounding elements, so I think it's very much worth it.

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #20)
> OK so I should also not remove the closetab.png from jar.mn since other close
> buttons will still use it, right?

You should probably use chrome://global/skin/icons/close.png for .tabs-closebutton, chrome://global/skin/icons/close-sidebar.png for #sidebar-header > .tabs-closebutton, and remove closetab.png.

As of <http://hg.mozilla.org/mozilla-central/rev/fc119d5000e5>, the tabview/ stuff isn't needed anymore.

Comment 24

[Anthony Ricaud (:rik)]

I don't understand your comment about tabview/. The patch in <http://hg.mozilla.org/mozilla-central/rev/fc119d5000e5> is Windows only.

Comment 25

[Dão Gottwald [:dao]]

Um, right, I thought it covered all platforms.

Comment 26

[Anthony Ricaud (:rik)]

Attachment: Patch v3

BTW, I have no example of .tabs-closebutton that is not in #sidebar-header so I can't really test this.

Comment 27

[Frank Yan (:fryn)]

(In reply to comment #26)
> Created attachment 511424 [details] [diff] [review]
> Patch v3
> 
> BTW, I have no example of .tabs-closebutton that is not in #sidebar-header so I
> can't really test this.

We use it in an obscure, non-default configuration:
Go to about:config in the url bar
Type in browser.tabs.closeButtons
Change the value to 3
See the lone close button at the right end of the tab bar.
That is an example of .tabs-closebutton outside of #sidebar-header

Comment 28

[Anthony Ricaud (:rik)]

Ah thanks.

So it looks ok for this button too. Even better since it now has a hover state.

Comment 29

[Anthony Ricaud (:rik)]

Thanks!

So what's the next step ?

Comment 30

[Frank Yan (:fryn)]

(In reply to comment #29)
> Thanks!
> 
> So what's the next step ?

Wait for a driver to a+ (approval+) it, then it'll be ready to land.
Thanks for making the patch. :)

Comment 31

[Justin Dolske [:Dolske]]

Comment on attachment 511424 [details] [diff] [review]
Patch v3

Huh, no more round close buttons? Isn't that some kind of platform convention thing? Anyway, a+!

Comment 32

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/8665df09de8b

Comment 33

[Anthony Ricaud (:rik)]

Attachment: Before screenshot

Very cool, thanks to everyone for helping me with this.

Some gift screenshots :)

Comment 34

[Anthony Ricaud (:rik)]

Attachment: After screenshot

Comment 35

[Teodosia Pop]

Verified using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110220 Firefox/4.0b12pre.