Closed
Bug 467091
Opened 14 years ago
Closed 14 years ago
SeaMonkey's tabbrowser tabs shouldn't be native-styled
Categories
(SeaMonkey :: Themes, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.0a2
People
(Reporter: stefanh, Assigned: mstange)
References
Details
(Keywords: classic)
Attachments
(2 files, 6 obsolete files)
27.40 KB,
image/png
|
Details | |
4.10 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•14 years ago
|
||
I think this should block the a2 release.
Flags: blocking-seamonkey2.0a2?
Reporter | ||
Comment 2•14 years ago
|
||
Here's a screenshot for those that doesn't have a mac.
![]() |
||
Comment 3•14 years ago
|
||
I agree with Stefan that we shouldn't ship an Alpha with this.
Flags: blocking-seamonkey2.0a2? → blocking-seamonkey2.0a2+
Assignee | ||
Comment 4•14 years ago
|
||
Assignee | ||
Comment 5•14 years ago
|
||
Assignee | ||
Comment 6•14 years ago
|
||
The patch is not RTL compatible yet.
Attachment #350494 -
Attachment is obsolete: true
Reporter | ||
Comment 7•14 years ago
|
||
Comment on attachment 350496 [details] [diff] [review] wip patch v2: with a line before the first tab I'll have look at this tomorrow.
Reporter | ||
Comment 8•14 years ago
|
||
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).
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #350492 -
Attachment is obsolete: true
Attachment #350496 -
Attachment is obsolete: true
Attachment #350654 -
Flags: ui-review?(stefanh)
Attachment #350492 -
Flags: ui-review?(stefanh)
Reporter | ||
Comment 10•14 years ago
|
||
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.
Assignee | ||
Comment 11•14 years ago
|
||
Ah, now I understand. I thought your concern (2) was that the text doesn't move downwards when using the smaller font.
Reporter | ||
Comment 12•14 years ago
|
||
(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.
Assignee | ||
Comment 13•14 years ago
|
||
Attachment #350654 -
Attachment is obsolete: true
Attachment #350785 -
Flags: ui-review?(stefanh)
Attachment #350785 -
Flags: review?(stefanh)
Attachment #350654 -
Flags: ui-review?(stefanh)
Assignee | ||
Updated•14 years ago
|
Attachment #350785 -
Attachment is patch: true
Attachment #350785 -
Attachment mime type: application/octet-stream → text/plain
Reporter | ||
Comment 14•14 years ago
|
||
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+
Assignee | ||
Comment 15•14 years ago
|
||
Attachment #350785 -
Attachment is obsolete: true
Attachment #350817 -
Flags: superreview?(neil)
Updated•14 years ago
|
Attachment #350817 -
Flags: superreview?(neil) → superreview+
Comment 16•14 years ago
|
||
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.
Reporter | ||
Comment 17•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
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");
> }
Assignee | ||
Comment 20•14 years ago
|
||
Attachment #350817 -
Attachment is obsolete: true
Attachment #350987 -
Flags: superreview?(neil)
Assignee | ||
Updated•14 years ago
|
Attachment #350987 -
Flags: superreview?(neil)
Assignee | ||
Comment 21•14 years ago
|
||
pushed: http://hg.mozilla.org/comm-central/rev/bc3ad517b210
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0a2
You need to log in
before you can comment on or make changes to this bug.
Description
•