[OS X] Update Close Buttons Appearance

VERIFIED FIXED in Firefox 4.0b12

Status

()

Firefox
Theme
--
enhancement
VERIFIED FIXED
8 years ago
5 years ago

People

(Reporter: shorlander, Assigned: rik)

Tracking

Trunk
Firefox 4.0b12
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 467817 [details]
New Tab Close Buttons Style

Update Mac tab close icons.

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

8 years ago
I'd like to try to provide my first Firefox patch with this. Can we have the PSD files used for the screenshot ?
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?
(Reporter)

Updated

8 years ago
Assignee: shorlander → nobody
(Reporter)

Comment 3

8 years ago
(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

8 years ago
Created attachment 510635 [details] [diff] [review]
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

8 years ago
Created attachment 510637 [details] [diff] [review]
Tab Close Button v1 – 16x16px (with CSS)
(Assignee)

Comment 6

8 years ago
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.
(Reporter)

Updated

8 years ago
Summary: [OS X] Update Tab Close Buttons Appearance → [OS X] Update Close Buttons Appearance
(Reporter)

Updated

8 years ago
Status: ASSIGNED → NEW

Comment 7

8 years ago
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?
Attachment #510635 - Flags: ui-review?(shorlander)
(Reporter)

Comment 8

8 years ago
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.
Attachment #510635 - Flags: ui-review?(shorlander) → ui-review-
(Reporter)

Comment 9

8 years ago
Created attachment 510721 [details]
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!
(Reporter)

Comment 10

8 years ago
Created attachment 510722 [details]
Close Icon: Tabs/Toolbars
(Reporter)

Comment 11

8 years ago
Created attachment 510723 [details]
Close Icon: Sidebars
(Reporter)

Comment 12

8 years ago
Created attachment 510724 [details]
Close Icon: Panels
(Reporter)

Comment 13

8 years ago
Created attachment 510725 [details]
Close Icon: Panorama
(Reporter)

Comment 14

8 years ago
(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.
(Assignee)

Comment 15

8 years ago
Created attachment 510825 [details] [diff] [review]
Patch

Let's go for the first patch.

Everything works as expected to me, except the active state for the panorama close buttons.
Assignee: nobody → anthony
Status: NEW → ASSIGNED
Version: unspecified → Trunk
(Assignee)

Updated

8 years ago
Attachment #510825 - Flags: review?(shorlander)
(Reporter)

Comment 16

8 years ago
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.
Attachment #510825 - Flags: ui-review+
Attachment #510825 - Flags: review?(shorlander)
Attachment #510825 - Flags: review?(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.
Attachment #510825 - Flags: review?(dao) → review-
(Assignee)

Comment 18

8 years ago
(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?
.tabs-closebutton targets various different close buttons, so you need to add another rule.
(Assignee)

Comment 20

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

Comment 21

8 years ago
Created attachment 511075 [details] [diff] [review]
Patch v2
Attachment #510825 - Attachment is obsolete: true
Attachment #511075 - Flags: review?(dao)

Updated

8 years ago
Attachment #511075 - Flags: ui-review?(shorlander)
(Reporter)

Updated

8 years ago
Attachment #511075 - Flags: ui-review?(shorlander) → ui-review+

Comment 22

8 years ago
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.
Attachment #511075 - Flags: feedback+
(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.
(Assignee)

Comment 24

8 years ago
I don't understand your comment about tabview/. The patch in <http://hg.mozilla.org/mozilla-central/rev/fc119d5000e5> is Windows only.
Um, right, I thought it covered all platforms.
(Assignee)

Comment 26

8 years ago
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.
Attachment #511075 - Attachment is obsolete: true
Attachment #511424 - Flags: review?(dao)
Attachment #511075 - Flags: review?(dao)

Comment 27

8 years ago
(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
(Assignee)

Comment 28

8 years ago
Ah thanks.

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

Updated

8 years ago
Attachment #511424 - Flags: review?(dao) → review+
(Assignee)

Comment 29

8 years ago
Thanks!

So what's the next step ?

Updated

8 years ago
Attachment #511424 - Flags: approval2.0?

Updated

8 years ago
Attachment #510635 - Attachment is obsolete: true

Updated

8 years ago
Attachment #510637 - Attachment is obsolete: true

Comment 30

8 years ago
(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 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+!
Attachment #511424 - Flags: approval2.0? → approval2.0+

Updated

8 years ago
Keywords: checkin-needed

Comment 32

8 years ago
http://hg.mozilla.org/mozilla-central/rev/8665df09de8b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Firefox 4.0 → Firefox 4.0b12
(Assignee)

Comment 33

8 years ago
Created attachment 511683 [details]
Before screenshot

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

Some gift screenshots :)
(Assignee)

Comment 34

8 years ago
Created attachment 511684 [details]
After screenshot

Updated

8 years ago
Depends on: 633847

Comment 35

8 years ago
Verified using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110220 Firefox/4.0b12pre.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.