Mac Classic tabbrowser looks bad

RESOLVED FIXED

Status

defect
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: stefanh, Assigned: stefanh)

Tracking

({regression})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

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.
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 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)
Attachment #261008 - Attachment description: Patch, need testing on win/nix → Patch, needs testing on win/nix
+        <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).
So, I never checked before I mocked up the patch - of course this exists on xpfe as well
Summary: [suiterunner] Mac Classic tabbrowser looks bad → Mac Classic tabbrowser looks bad
Attachment #261008 - Attachment is obsolete: true
Hmm, right - I think I just need a mac tabbBrowserBindings.xml and a mac tabbrowser.css in classic/navigator/mac
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

Posted patch globalBindings.xml changes (obsolete) — Splinter Review
Changes made to the new classic/navigator/mac/tabbrowserBindings.xml (the diff is against globalBindings.xml)
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?
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 on attachment 261571 [details]
Screenshots, before and after patch

Btw, bug 361568 is the reason why some of the icons is cut-off.
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
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).
Posted patch Neil's tabbrowser changes (obsolete) — Splinter Review
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
Posted file CVS copy
This is the cvs copy changes - copy tabbrowser.css in classic/navigator to classic/navigator/win/ and classic/navigator/mac.
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
Attachment #261689 - Flags: review?(cst)
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)
Attachment #261691 - Flags: superreview?(neil)
Attachment #261691 - Flags: review?(neil)
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.
Attachment #261691 - Flags: superreview?(neil)
Attachment #261691 - Flags: superreview+
Attachment #261691 - Flags: review?(neil)
Attachment #261691 - Flags: review+
Posted patch Take2 - mac changes (obsolete) — Splinter Review
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.
Attachment #261505 - Attachment is obsolete: true
Attachment #261509 - Attachment is obsolete: true
Attachment #261709 - Flags: superreview?(neil)
Attachment #261709 - Flags: review?(mnyromyr)
Posted patch tabbrowser.css diff (obsolete) — Splinter Review
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.
Status: NEW → ASSIGNED
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.
Attachment #261709 - Flags: superreview?(neil) → superreview+
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 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....
(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.
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).
Attachment #261689 - Attachment is obsolete: true
Attachment #262035 - Flags: review?(cst)
Attachment #261689 - Flags: review?(cst)
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+).
Attachment #261709 - Attachment is obsolete: true
Attachment #261710 - Attachment is obsolete: true
Attachment #262292 - Flags: superreview+
Attachment #262292 - Flags: review?(mnyromyr)
Attachment #261709 - Flags: review?(mnyromyr)
jftr: I fixed Composer's tabs in bug 378202.
Attachment #262035 - Flags: review?(cst) → review?(mnyromyr)
Keywords: regression
Depends on: 379060
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).
Attachment #262035 - Attachment is obsolete: true
Attachment #262035 - Flags: review?(mnyromyr)
Attachment #262292 - Attachment is obsolete: true
Attachment #262292 - Flags: review?(mnyromyr)
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.
Attachment #263372 - Flags: superreview?(jag)
Attachment #263372 - Flags: review?(mnyromyr)
Attachment #263372 - Flags: superreview?(jag) → superreview+
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... :(
Attachment #263372 - Flags: review?(mnyromyr) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I'll remove the obsolete themes/classic/navigator/tabbrowser.css file once it has cycled.
(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.
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.