[OS X] Update Close Buttons Appearance

VERIFIED FIXED in Firefox 4.0b12

Status

()

enhancement
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: shorlander, Assigned: rik)

Tracking

Trunk
Firefox 4.0b12
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 4 obsolete attachments)

35.04 KB, image/png
Details
237.31 KB, image/png
Details
771 bytes, image/png
Details
1.14 KB, image/png
Details
795 bytes, image/png
Details
833 bytes, image/png
Details
19.73 KB, patch
dao
: review+
Dolske
: approval2.0+
Details | Diff | Splinter Review
81.29 KB, image/png
Details
78.70 KB, image/png
Details

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?
(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
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.
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.
Summary: [OS X] Update Tab Close Buttons Appearance → [OS X] Update Close Buttons Appearance

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)
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-
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!
(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
Posted patch Patch (obsolete) — Splinter Review
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)
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
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #510825 - Attachment is obsolete: true
Attachment #511075 - Flags: review?(dao)

Updated

8 years ago
Attachment #511075 - Flags: ui-review?(shorlander)
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
Posted patch Patch v3Splinter Review
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)
(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.
Attachment #511424 - Flags: review?(dao) → review+
Assignee

Comment 29

8 years ago
Thanks!

So what's the next step ?
Attachment #511424 - Flags: approval2.0?
Attachment #510635 - Attachment is obsolete: true
Attachment #510637 - Attachment is obsolete: true
(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+
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
Posted image Before screenshot
Very cool, thanks to everyone for helping me with this.

Some gift screenshots :)
Assignee

Comment 34

8 years ago
Posted image After screenshot

Updated

8 years ago
Depends on: 633847
Blocks: 634272

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.