467091 - SeaMonkey's tabbrowser tabs shouldn't be native-styled

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Stefan [:stefanh]]

After the landing of bug 231313, SeaMonkey's tabbrowser tabs has a ... well, "really native", not "10.2-native" style. That's not what we want, because a "really native" tab is completely wrong to have as a browser tab.

Comment 1

[Stefan [:stefanh]]

I think this should block the a2 release.

Comment 2

[Stefan [:stefanh]]

Attachment: screenshot

Here's a screenshot for those that doesn't have a mac.

Comment 3

[Robert Kaiser]

I agree with Stefan that we shouldn't ship an Alpha with this.

Comment 4

[Markus Stange [:mstange]]

Attachment: screenshot v1

Comment 5

[Markus Stange [:mstange]]

Attachment: wip patch v1

Comment 6

[Markus Stange [:mstange]]

Attachment: wip patch v2: with a line before the first tab

The patch is not RTL compatible yet.

Comment 7

[Stefan [:stefanh]]

Comment on attachment 350496 [details] [diff] [review]
wip patch v2: with a line before the first tab

I'll have look at this tomorrow.

Comment 8

[Stefan [:stefanh]]

Comment on attachment 350496 [details] [diff] [review]
wip patch v2: with a line before the first tab

First of all: Thanks for doing this Markus :-)

I'm commenting on the patch instead, since the ui differs slightly from the screenshot. I don't feel I should comment on the code, though - it's a wip.

I like this, I think it fits well with the current look. ui-wise, I have only 2 concerns:

1) When we talked yesterday, Markus wondered if the tabs were too large. Actually, I think they are a bit too large. For the labels, instead of 13px I think we want 12px font-size. That means using .tab-text { font: icon; } (or whatever that sounds semantically best and gives us kThemeViewsFont). It looks to me that just switching to a smaller font is enough here, since the tab height will shrink a bit.

2) When you select a tab, both text and icon move a bit downwards. Using a smaller font, only the icon moves downwards. (in this case it's the padding change on .tab-image-middle when the tab is selected that causes this, fwiw).

Comment 9

[Markus Stange [:mstange]]

Attachment: wip patch v3

Comment 10

[Stefan [:stefanh]]

Comment on attachment 350654 [details] [diff] [review]
wip patch v3

Yeah, size is OK now. But label and icon still moves when a tab is selected.

Comment 11

[Markus Stange [:mstange]]

Ah, now I understand. I thought your concern (2) was that the text doesn't move downwards when using the smaller font.

Comment 12

[Stefan [:stefanh]]

(In reply to comment #11)
> Ah, now I understand. I thought your concern (2) was that the text doesn't move
> downwards when using the smaller font.

Ah, heh.

Comment 13

[Markus Stange [:mstange]]

Attachment: v4

Comment 14

[Stefan [:stefanh]]

Comment on attachment 350785 [details] [diff] [review]
v4

This looks good to me. Thanks :-) We've discussed the focusring (irc) and I think we can tweak it like this:

  outline: 2px solid -moz-mac-focusring;
  outline-offset: -2px;
  -moz-outline-radius:  5px;

r=me with that fixed.

Comment 15

[Markus Stange [:mstange]]

Attachment: v4.1

Comment 16

[[e-mail]]

Comment on attachment 350817 [details] [diff] [review]
v4.1

>-.tab-image-left, .tab-image-right, .tab-image-middle {
We can probably follow this up in a new bug by removing the tabbrowser tab.

>+.tabbrowser-tab:focus {
>+  outline: 2px solid -moz-mac-focusring;
>+  outline-offset: -2px;
>+  -moz-outline-radius: 5px;
>+}
>+
>+
Nit: double blank line.

>+.tabbrowser-tab[selected=true] > .tab-image-middle {
.tab-image-middle inherits selected, so .tab-image-middle[selected="true"] should work (it might even avoid the :not on the :hover rule earlier).

>+.tabbrowser-tab[beforeselected=true] {
>+  -moz-border-right-colors: transparent transparent;
> }
Nit: this doesn't look RTL safe. Also, "true" needs quotes.

>+.tabbrowser-tab[busy] {
>+  list-style-image: url("chrome://global/skin/icons/loading_16.png");
>+}
Because you reordered the style rules this doesn't look like an edit...

>-.tabbrowser-tab[busy] {
>-  list-style-image: url("chrome://communicator/skin/icons/loading.gif");
>-}
... an edit which we don't want anyway. Please put this one back and try and keep the rules in the original order so that it's obvious what's changing!

>+.tabs-closebutton:hover {
>+  list-style-image: url("chrome://global/skin/icons/closetab-hover.png"); 
>+}
>+
>+.tabs-closebutton:active:hover {
>+  list-style-image: url("chrome://global/skin/icons/closetab-active.png"); 
>+}
Nit: trailing space after ;

sr=me with the nits fixed including fixing the style rule order.

Comment 17

[Stefan [:stefanh]]

(In reply to comment #16)

> >+.tabbrowser-tab[busy] {
> >+  list-style-image: url("chrome://global/skin/icons/loading_16.png");
> >+}
> Because you reordered the style rules this doesn't look like an edit...
> 
> >-.tabbrowser-tab[busy] {
> >-  list-style-image: url("chrome://communicator/skin/icons/loading.gif");
> >-}
> ... an edit which we don't want anyway. Please put this one back and try and
> keep the rules in the original order so that it's obvious what's changing!

I strongly disagree. The loading_16.png icon *is* a well-known, standardized mac symbol for something that's loading and it's used in all mac apps. I think using the old icon will look really funny - mac users will wonder why the tabs etc look normal and not the loading icon.

Comment 18

[[e-mail]]

OK, so it turns out that I clicked on winstripe's loading_16.png by mistake.

/me mumbles something about having different images with too similar names...

Comment 19

[[e-mail]]

To clarify, the diff should look like this after all:

> .tabbrowser-tab[busy] {
>-  list-style-image: url("chrome://communicator/skin/icons/loading.gif");
>+  list-style-image: url("chrome://global/skin/icons/loading_16.png");
> }

Comment 20

[Markus Stange [:mstange]]

Attachment: v5

Comment 21

[Markus Stange [:mstange]]

pushed: http://hg.mozilla.org/comm-central/rev/bc3ad517b210