Mark Banner (:standard8) Reporter
	Description • 18 years ago

We're missing the grippies for the classic theme on suiterunner this is a regression that would be good to get fixed.

	Karsten Düsterloh
	Comment 1 • 18 years ago

This brings back grippies for Classic by moving our special rules from global into communicator (thus we get those new toolkit rules for things "missing" in XPFE). I wrote the patch under Mac OS X and made analogous changes for the other OSs, but those need to be tested. On Mac at least, the changes don't hurt normal trunk builds.

	Bruno 'Aqualon' Escherl
	Comment 2 • 18 years ago

(In reply to comment #1)
> Created an attachment (id=249478) [edit]
> override toolkit theming

I tested the patch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/2006122214 SeaMonkey/1.5a (suiterunner build) and the grippies work as expected.

	Mark Banner (:standard8) Reporter
	Comment 3 • 18 years ago

The raised toolbar problem seen on windows and linux appears to come down to the difference between

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/themes/classic/global/win/toolbar.css&rev=1.31&mark=57-61#57

and

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/themes/gnomestripe/global/toolbar.css&rev=1.6&mark=53-63#53

Due to the toolkit definition we get an extra "!-moz-appearance = toolbar" and "min-height=20px" in the css definition of the toolbar (as shown in domi). Deleting the "!-moz-appearance = toolbar" and resizing the main window resets the toolbars to their normal flat display.

	Robert Kaiser Assignee
	Comment 4 • 18 years ago

hmm, it would basically be nice if we could use -moz-appearance:toolbar so that our toolbars get styled just like it's usual on the OS - but for now, I think we can go with -moz-appearance:none

I'm currently working on making this work based on Karsten's patch from here, but moving everything, including the files, to communicator/ so that it still works after bug 349409

	Robert Kaiser Assignee
	Comment 5 • 18 years ago

I've done a new patch based on Karsten's earlier patch here, but moving the files out of global/
This files shows the moves we need.

	Robert Kaiser Assignee
	Comment 6 • 18 years ago

Here's the real patch. Note that you additionally need to move the image files like shown in the cvs copies attachment.

	Robert Kaiser Assignee
	Comment 7 • 18 years ago

For easier reviewing, this additional diff shows just the changes I did to the toolbar.css files (toolbarbutton.css move unchanged).

	neil@parkwaycc.co.uk
	Comment 8 • 18 years ago

Comment on attachment 254202 [details]
cvs copies from global/ to communicator/

>mozilla/themes/classic/global/toolbar/tbgrip-arrow.gif mozilla/themes/classic/toolbar/tbgrip-arrow.gif
>mozilla/themes/classic/global/toolbar/tbgrip-arrow-clps.gif mozilla/themes/classic/toolbar/tbgrip-arrow-clps.gif
>mozilla/themes/classic/global/toolbar/tbgrip-texture.gif mozilla/themes/classic/toolbar/tbgrip-texture.gif
As correctly referenced in attachment 254203 [details] [diff] [review], these need to be in mozilla/themes/classic/communicator/toolbar/

	neil@parkwaycc.co.uk
	Comment 9 • 18 years ago

Comment on attachment 254203 [details] [diff] [review]
patch v1: use definitions from communicator/ instead of global/

You can't remove the old global/toolbar(button).css just yet!

	Robert Kaiser Assignee
	Comment 10 • 18 years ago

(In reply to comment #8)
> As correctly referenced in attachment 254203 [details] [diff] [review], these need to be in
> mozilla/themes/classic/communicator/toolbar/

Argh, sure, c&p error. I'll post an updated cvs copies file.

	Robert Kaiser Assignee
	Comment 11 • 18 years ago

OK, I have decided to rework this so that we don't redefine what toolkit already defines (and they put quite some work into "looking right" on all OSes).
This patch also has shrunk to a size where I think we can go without cvs copies, which makes cvs admins happier...

I only left those styles in our files that we need for grippies and none that are already in toolkit. I also tried to leave the -moz-appearance:toolbar in for toolbar/menubar elements, and killed all additional borders, leaving only the one between the grippy and the toolbar-holder in. We'll need to see how this looks in different native themes, but it looks to me like an elegant solution.
I could only test this on Linux though, so I don't know yet if this looks fine on win/mac/os2.

	Karsten Düsterloh
	Comment 12 • 18 years ago

Comment on attachment 254361 [details] [diff] [review]
patch v2: only add our specific styles to toolkit's

This does not quite work for Mac yet. After copying over the tbgrip* icons, in XPFE the grippybox gets plain blue and the SR toolbars lose their stripes - screenshot upcoming.

	Karsten Düsterloh
	Comment 14 • 18 years ago

Okay, the blue colour was actually my fault, but the rest still holds:
- in XPFE, the collapsed-tray-holder shows no stripes (which is even a bug without your patch), it's just a missing
    .collapsed-tray-holder {-moz-appearance: toolbar;}
- in SuiteRunner, the collapsed-tray-holder is correctly styled by toolkit - but only because toolkit breaks the toolbar's stripes by setting a -moz-appearance:toolbar!important; onto the toolbox!

	Robert Kaiser Assignee
	Comment 15 • 18 years ago

Karsten:
so, actually, if xpfe works ok but is not more broken than without the patch, we shouldn't care much, IMHO - we won't release based on xpfe anyways, this is just so that trunk stays usable for testing while we get ready to make suiterunner the default.

I don't understand the second part completely yet though: Is toolkit breaking all toolbars anywhere (including FF/TB) because of that toolbox rule, or just ours, ot just the collapsed-tray-holder? What can we (easily) do to fix that?

	Karsten Düsterloh
	Comment 16 • 18 years ago

> so, actually, if xpfe works ok but is not more broken than without the patch,
> we shouldn't care much, IMHO - we won't release based on xpfe anyways

Okay, true.

> I don't understand the second part completely yet though: Is toolkit breaking
> all toolbars anywhere (including FF/TB) because of that toolbox rule

Yes, so it seems (I didn't look into FF/TB yet, though; I'll do this tonight): see also <http://mxr-test.landfill.bugzilla.org/seamonkey/source/toolkit/themes/pinstripe/global/toolbar.css#48>.

	Robert Kaiser Assignee
	Comment 17 • 18 years ago

Actually, this looks to me like it was intentional, see bug 354644 - so I start to think it 's probably better to check in what we have at the moment, and then try to figure out what mac toolbar background color is really correct. I personally don't know relevant Apple docs that describe how it should look to follow "native" style for OSX.

	Stefan [:stefanh]
	Comment 18 • 18 years ago

I like the platform dirs in communicator/  ;-)

	Stefan [:stefanh]
	Comment 19 • 18 years ago

(In reply to comment #17)
> Actually, this looks to me like it was intentional, see bug 354644 - so I start
> to think it 's probably better to check in what we have at the moment, and then
> try to figure out what mac toolbar background color is really correct.

Fwiw, I agree (haven't tested the patch, though). It shouldn't be too hard to add some styling later if it feels totally wrong (note that mac classic is pretty wrong in certain cases so this could also be an improvement).

	Karsten Düsterloh
	Comment 20 • 18 years ago

Comment on attachment 254361 [details] [diff] [review]
patch v2: only add our specific styles to toolkit's

Adding 

toolbar
{
  background: none;
}

to the Mac toolbar.css makes the the toolbars be striped again for SR.
r=me with that (and the following two other minor nits), supposing Stefan agrees:

>Index: mozilla/themes/classic/jar.mn
>===================================================================
>+#ifdef XP_MACOSX
>+  skin/classic/communicator/toolbar.css                                 (communicator/mac/toolbar.css)
>+#else
>+#ifdef XP_OS2

#elif XP_OS2 etc. should work (and does indeed on Mac).

>+ * The Original Code is Mozilla Communicator client code, released
>+ * March 31, 1998.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998-2001
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Joe Hewitt (hewitt@netscape.com)

Are You Sure?! ;-)

	Stefan [:stefanh]
	Comment 21 • 18 years ago

(In reply to comment #20)
> (From update of attachment 254361 [details] [diff] [review])
> Adding 
> 
> toolbar
> {
>   background: none;
> }
> 
>

I don't have a working sr tree/build atm so I can't look at this until tomorrow evening (CET). But I wonder how your change will affect a toolbar without a containing toolbox (I imagine that the stripes you see comes from the toolbox).

	Stefan [:stefanh]
	Comment 23 • 18 years ago

Comment on attachment 254361 [details] [diff] [review]
patch v2: only add our specific styles to toolkit's

I don't think this patch made toolbars lose their stripes. I didn't had any stripes in the toolbar before the patch either. 
Anyway, I think the style is much better with Mnyromyr's proposal, but I don't see any stripes in the testcase. Whatever you do (I'm fine with either way) this will need a follow-up bug so we get the toolbar styling right for mac.

	Robert Kaiser Assignee
	Comment 24 • 18 years ago

Filed bug 370426 about the mac toolbar background problem and checked in v2 with Karsten's nits fixed.