Grippies missing on toolbars for Classic theme in suiterunner

RESOLVED FIXED

Status

defect
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: standard8, Assigned: kairo)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

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

Comment 1

13 years ago
Posted patch override toolkit theming (obsolete) — Splinter Review
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.

Updated

13 years ago
Attachment #249478 - Flags: review?(neil)
(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.
Reporter

Comment 3

13 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.
Assignee

Comment 4

13 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
Assignee

Comment 5

13 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.
Attachment #254202 - Flags: superreview?(neil)
Attachment #254202 - Flags: review?(mnyromyr)
Assignee

Comment 6

13 years ago
Here's the real patch. Note that you additionally need to move the image files like shown in the cvs copies attachment.
Assignee: mnyromyr → kairo
Attachment #249478 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #254203 - Flags: superreview?(neil)
Attachment #254203 - Flags: review?(mnyromyr)
Attachment #249478 - Flags: review?(neil)
Assignee

Comment 7

13 years ago
For easier reviewing, this additional diff shows just the changes I did to the toolbar.css files (toolbarbutton.css move unchanged).
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/
Attachment #254202 - Flags: superreview?(neil) → superreview+
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!
Attachment #254203 - Flags: superreview?(neil) → superreview+
Assignee

Comment 10

13 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.
Assignee

Comment 11

13 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.
Attachment #254202 - Attachment is obsolete: true
Attachment #254203 - Attachment is obsolete: true
Attachment #254204 - Attachment is obsolete: true
Attachment #254361 - Flags: superreview?(neil)
Attachment #254361 - Flags: review?(mnyromyr)
Attachment #254202 - Flags: review?(mnyromyr)
Attachment #254203 - Flags: review?(mnyromyr)

Updated

13 years ago
Attachment #254361 - Flags: superreview?(neil) → superreview+
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.
Attachment #254361 - Flags: review?(mnyromyr) → review-
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!
Assignee

Comment 15

13 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?
> 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>.
Assignee

Comment 17

13 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.

Comment 18

13 years ago
I like the platform dirs in communicator/  ;-)

Comment 19

13 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).
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?! ;-)
Attachment #254361 - Flags: review?(stefanh)
Attachment #254361 - Flags: review-
Attachment #254361 - Flags: review+

Comment 21

13 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).

Comment 22

13 years ago
Posted file Toolbar testcase

Comment 23

13 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.
Attachment #254361 - Flags: review?(stefanh)
Assignee

Updated

13 years ago
Blocks: 370426
Assignee

Comment 24

13 years ago
Filed bug 370426 about the mac toolbar background problem and checked in v2 with Karsten's nits fixed.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.