376912 - Mac Classic tabbrowser looks bad

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Stefan [:stefanh]]

We're missing a lot of elements from the extra bindings in classic/global/mac/globalBindings on suiterunner. This make the browser tabs looks pretty bad. Icons are stretched and the whole tab content (label, icon) looks misaligned. I was looking at this yesterday and I couldn't make the tabs look right with just styling the current elements. Basically, I think we need the elements that where styled in global/mac/browser.css (now navigator/tabbrowser.css).

We also need different styling of tabbrowser tabs, the styles we get from Pinstripe's tabbox.css are for non-browser tabs and some of them don't fit well (like 3px padding on the tab element).

In a long-time perspective, we shouldn't use native styling on mac tabbrowser tabs. Neither Camino, Safari or Firefox uses native browser tabs. It just don't look right... Also, if we skip the native styling on mac browser tabs, we won't have any problem with people breaking them (the tabs doesn't always behave as expected when you try to style them) and we don't have to think of what happens when bug 231313 is fixed.

Comment 1

[Stefan [:stefanh]]

Attachment: Patch, needs testing on win/nix

Note: this patch assumes a 'mac' and a 'win' dir in themes/classic/navigator.

This makes it look as it used to do. Comments are welcome. There might be some tweaks left to do in tabbrowser.css, but they're minor (new tab button is a bit to ffar at the right etc). I figured we could use the themes/classic/navigator/win/tabbrowser.css file for xpfe builds.

Comment 2

[Stefan [:stefanh]]

Comment on attachment 261008 [details] [diff] [review]
Patch, needs testing on win/nix

Take into account that I forgot 2 endifs in jar.mn...(should work anyway)

Comment 3

[Stefan [:stefanh]]

+        <xul:hbox class="tab-icon-container">
+          <xul:image class="tab-icon" xbl:inherits="validate,src=image"/>
+	</xul:hbox>

...and the "tab-icon-container" should probably go (or I should use it in the mac binding as well).

Comment 4

[Stefan [:stefanh]]

So, I never checked before I mocked up the patch - of course this exists on xpfe as well

Comment 5

[Stefan [:stefanh]]

Hmm, right - I think I just need a mac tabbBrowserBindings.xml and a mac tabbrowser.css in classic/navigator/mac

Comment 6

[Stefan [:stefanh]]

OK, thought a bit of this and I think the best thing to do is probably:

1) copy themes/classic/global/mac/globalBindings.xml to themes/classic/navigator/mac/tabbrowserBindings.xml

2) copy themes/classic/navigator/tabbrowser.css to themes/classic/navigator/mac/tabbrowser.css and to themes/classic/navigator/win/tabbrowser.css

3) Patch tabbrowserBindings.xml and mac/tabbrowser.css

Comment 7

[Stefan [:stefanh]]

Attachment: globalBindings.xml changes

Changes made to the new classic/navigator/mac/tabbrowserBindings.xml (the diff is against globalBindings.xml)

Comment 8

[Stefan [:stefanh]]

Attachment: Example of changes in tabbrowser.css

I'm not finish with this one yet, but these are the changes I found necessary.

Neil et al:

Does this whole approach sounds reasonable?

Comment 9

[Stefan [:stefanh]]

Attachment: Screenshots, before and after patch

Attaching 4 screenshots, comparing xpfe and suiterunner before/after (see attachment #261505 [details] [diff] [review] and attachment #261509 [details] [diff] [review] for changes). The original globalBindings.xml is left untouched in classic/mac/global.

Comment 10

[Stefan [:stefanh]]

Comment on attachment 261571 [details]
Screenshots, before and after patch

Btw, bug 361568 is the reason why some of the icons is cut-off.

Comment 11

[Stefan [:stefanh]]

Neil tested adding the elements from globalBindings.xml in tabbrowser.xml, and it appears that it doesn't break windows. This means that we don't need any bindings in the mac version of tabbrowser.css. Neither do we need tabbrowserBindings.xml

Comment 12

[Stefan [:stefanh]]

Composer uses native-styled bottom tabs. Basically, we have the same problem there - icons are stretched etc. With my patch the tabs looks even worse - pretty terrible, actually. I think it's because we seem to apply some bindings from classic's global.css... those are then styled with toolkit's tabbox.css. Not using the bindings in classic/global/mac/global.css will fix the issues introduced by me. The icons are still stretched, though.

I'm not going to do anything about this atm, since I think that is a separate issue. Most likely the problem is that we need the same content and styling as tabbrowser-tab (tab-image-left, tab-image-middle, tab-icon etc).

Comment 13

[Stefan [:stefanh]]

Attachment: Neil's tabbrowser changes

Those are Neil's tabbrowser changes. The only thing I've done is adding the "overflow: -moz-hidden-unscrollable;" that accidently got lost. This patch will add elements from themes/classic/global/globalBindings.xml and revert the mac style rules in tabbrowser.css

Comment 14

[Stefan [:stefanh]]

Attachment: CVS copy

This is the cvs copy changes - copy tabbrowser.css in classic/navigator to classic/navigator/win/ and classic/navigator/mac.

Comment 15

[Stefan [:stefanh]]

I'm going to play a little bit with my tabbrowser.css changes, but I think this should be solved in two steps:

1) get Neil's changes in and the cvs copying done
2) get the mac tabbrowser.css changes in

Comment 16

[Stefan [:stefanh]]

Comment on attachment 261691 [details]
CVS copy

Note that we could keep the windows version as it is and just copy one file from navigator/ to navigator/mac if you like (might be easier and faster). (not going to land anything until everything is settled)

Comment 17

[[e-mail]]

Comment on attachment 261691 [details]
CVS copy

You might want to tell the copier that since the file is already a recent copy it isn't on any branches and we don't care about the dates in the cvs history.

Comment 18

[Stefan [:stefanh]]

Attachment: Take2 - mac changes

Mac tabbrowser.css and jar.mn changes. It will look better with an updated tree (so the patch in bug 349409 is in). Karsten, in order to review this on mac you'll need to apply attachment #261689 [details] [diff] [review] and create a themes/classic/navigator/mac directory. Note that the drag-and drop stuff from windows is included here as well.

Comment 19

[Stefan [:stefanh]]

Attachment: tabbrowser.css diff

Here's a diff between the two tabbrowser.css files so you can see what I changes in the mac file. Note that it isn't perfect, but it'll do for now, I think.

Comment 20

[[e-mail]]

Comment on attachment 261709 [details] [diff] [review]
Take2 - mac changes

sr=me if you don't move the tabbrowser entries in jar.mn - the new entries should be inserted at the same point where you deleted the old one.

Comment 21

[Stefan [:stefanh]]

Comment on attachment 261709 [details] [diff] [review]
Take2 - mac changes

(In reply to comment #20)
> (From update of attachment 261709 [details] [diff] [review])
> sr=me if you don't move the tabbrowser entries in jar.mn - the new entries
> should be inserted at the same point where you deleted the old one.
>

I'll update the patch once Karsten have looked at it.

Comment 22

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 261689 [details] [diff] [review]
Neil's tabbrowser changes

Does it look like the screenshot with this patch?  If so, it's still messed up....

Comment 23

[Stefan [:stefanh]]

(In reply to comment #22)
> (From update of attachment 261689 [details] [diff] [review])
> Does it look like the screenshot with this patch?  If so, it's still messed
> up....

You mean the cut-off icons? See comment #10.

Comment 24

[Stefan [:stefanh]]

Attachment: Neil's tabbrowser changes, version 2

We actually don't need the tabs-left-cap/tabs-right-cap spacers, so I removed them. It might have been more clever to not have 2 reviewers, so just tell me if you think Karsten should do this one too :-) (he will need the patch anyway when looking at Part2).

Comment 25

[Stefan [:stefanh]]

Attachment: New version of take 2 (mac part)

I just realized that I forgot to add the focus rules from classic's tabbox.css. Fixed the jar.mn according to Neil's comment (moving sr+).

Comment 26

[Stefan [:stefanh]]

jftr: I fixed Composer's tabs in bug 378202.

Comment 27

[Stefan [:stefanh]]

Comment on attachment 262035 [details] [diff] [review]
Neil's tabbrowser changes, version 2

I'm going to let the cvs copying be done and then combine the two existing patches to one (easier for everyone).

Comment 28

[Stefan [:stefanh]]

Attachment: Complete patch - fix classic tabbrowser look

OK, here's a new version that combines the previous 2 patches. This one patches the newly cvs copied tabbrowser.css files (bug 379060). Trying jag for the sr.

The only "new" thing here is that I switched from tab {...} to .tabbrowser-tab {...} etc in the css files. Just thought that was more consistent.

Note here that close button and new tab button is not positioned 100% correct. But I'll file a follow-up on that.

Comment 29

[Karsten Düsterloh]

Comment on attachment 263372 [details] [diff] [review]
Complete patch - fix classic tabbrowser look

Looks very decent. :-)
Just two nits to carry over to the close/new tab button bug: 
- both buttons don't give hover feedback, so you don't know they're buttons
- the close buttons lacks a tooltip

Sorry for the delay... :(

Comment 30

[Stefan [:stefanh]]

Checking in suite/browser/navigator.css;
/cvsroot/mozilla/suite/browser/navigator.css,v  <--  navigator.css
new revision: 1.14; previous revision: 1.13
done
Checking in suite/browser/tabbrowser.xml;
/cvsroot/mozilla/suite/browser/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.181; previous revision: 1.180
done
Checking in themes/classic/jar.mn;
/cvsroot/mozilla/themes/classic/jar.mn,v  <--  jar.mn
new revision: 1.163; previous revision: 1.162
done
Checking in themes/classic/navigator/mac/tabbrowser.css;
/cvsroot/mozilla/themes/classic/navigator/mac/tabbrowser.css,v  <--  tabbrowser.css
new revision: 1.15; previous revision: 1.14
done
Checking in themes/classic/navigator/win/tabbrowser.css;
/cvsroot/mozilla/themes/classic/navigator/win/tabbrowser.css,v  <--  tabbrowser.css
new revision: 1.15; previous revision: 1.14
done

Comment 31

[Stefan [:stefanh]]

I'll remove the obsolete themes/classic/navigator/tabbrowser.css file once it has cycled.

Comment 32

[Stefan [:stefanh]]

(In reply to comment #31)
> I'll remove the obsolete themes/classic/navigator/tabbrowser.css file once it
> has cycled.
> 

File is now removed. Filed bug 381915 for the New/close Tab issue.