Closed Bug 467091 Opened 14 years ago Closed 14 years ago

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

Categories

(SeaMonkey :: Themes, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a2

People

(Reporter: stefanh, Assigned: mstange)

References

Details

(Keywords: classic)

Attachments

(2 files, 6 obsolete files)

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.
I think this should block the a2 release.
Flags: blocking-seamonkey2.0a2?
Attached image screenshot
Here's a screenshot for those that doesn't have a mac.
I agree with Stefan that we shouldn't ship an Alpha with this.
Flags: blocking-seamonkey2.0a2? → blocking-seamonkey2.0a2+
Attached image screenshot v1 (obsolete) —
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attachment #350492 - Flags: ui-review?(stefanh)
Attached patch wip patch v1 (obsolete) — Splinter Review
The patch is not RTL compatible yet.
Attachment #350494 - Attachment is obsolete: true
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 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).
Attached patch wip patch v3 (obsolete) — Splinter Review
Attachment #350492 - Attachment is obsolete: true
Attachment #350496 - Attachment is obsolete: true
Attachment #350654 - Flags: ui-review?(stefanh)
Attachment #350492 - Flags: ui-review?(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.
Ah, now I understand. I thought your concern (2) was that the text doesn't move downwards when using the smaller font.
(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.
Attached patch v4 (obsolete) — Splinter Review
Attachment #350654 - Attachment is obsolete: true
Attachment #350785 - Flags: ui-review?(stefanh)
Attachment #350785 - Flags: review?(stefanh)
Attachment #350654 - Flags: ui-review?(stefanh)
Attachment #350785 - Attachment is patch: true
Attachment #350785 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #350785 - Flags: ui-review?(stefanh)
Attachment #350785 - Flags: ui-review+
Attachment #350785 - Flags: review?(stefanh)
Attachment #350785 - Flags: review+
Attached patch v4.1 (obsolete) — Splinter Review
Attachment #350785 - Attachment is obsolete: true
Attachment #350817 - Flags: superreview?(neil)
Keywords: classic
Attachment #350817 - Flags: superreview?(neil) → superreview+
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.
(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.
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...
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");
> }
Attached patch v5Splinter Review
Attachment #350817 - Attachment is obsolete: true
Attachment #350987 - Flags: superreview?(neil)
Attachment #350987 - Flags: superreview?(neil)
pushed: http://hg.mozilla.org/comm-central/rev/bc3ad517b210
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0a2
Blocks: 467618
You need to log in before you can comment on or make changes to this bug.