suiterunner help should use the same back/forward/home/print icons as the rest of SeaMonkey, we need to leverage our new helpOverlay to do that (at least in Classic).

This patch makes suiterunner help fit with the rest of SeaMonkey in the default ("Classic") theme, using communicatoricons.png for the help viewer. The first rule in the added CSS is needed for the menubuttons to pick up the list-style-image, as toolkit help.css has a "toolbar toolbarbutton" rule there that also propagates to toolbarbutton children of a toolbar button. My new rule guarantees that child picks up the setting from its parent. The jar.mn includes a patch I've had in my tree for a long time now and never attached anywhere: It excludes the old global Classic rules from suiterunner, so that we remove redundancy of them and *stripe rules from toolkit.

> The jar.mn includes a patch I've had in my tree for a long time now and never > attached anywhere: It excludes the old global Classic rules from suiterunner, > so that we remove redundancy of them and *stripe rules from toolkit. That would be the patch in bug 349409 :P

(In reply to comment #2) > That would be the patch in bug 349409 :P Oops... that's waiting long enough that I forgot it's not even mine ;-) In this case, close out this part in reviews here, it's a different bug, sorry.

Mano made a pig's ear of help.css in bug 353673, in particular the style rule #help-forward-button:not([disabled="true"]):active is wrong for two reasons.

Comment on attachment 245102 [details] [diff] [review] classic patch: restyle button icons, exclude old classic global from suiterunner >+#help-forward-button { >+ list-style-image: url("chrome://communicator/skin/icons/communicatoricons.png"); >+ -moz-image-region: rect(90px 29px 119px 0); >+} >+ >+#help-forward-button:not([disabled="true"]):hover { >+ -moz-image-region: rect(90px 59px 119px 30px); >+} >+ >+#help-forward-button:not([disabled="true"]):hover:active { >+ -moz-image-region: rect(90px 89px 119px 60px); >+} >+ >+#help-forward-button[disabled="true"] { >+ -moz-image-region: rect(90px 119px 119px 90px) !important; >+} Some of your problems arise from toolkit help.css badness but: 1. You shouldn't need to repeat the list-style-image for each button 2. You shouldn't need to use :not([disabled="true"]) 3. You shouldn't need to use [disabled="true"] { !important; } Also, won't you be overriding pinstripe on the Mac in which case you'll need separate review for that.

Filed bug 361096 on comment #4 and changed the rest locally, I also have a patch for modern locally in my tree, will attach this all soon. Who can/should do the mac review?

Here's a new patch for both Classic and Modern. For Classic, I did what Neil requested, but it now only works correctly after applying the patch of bug 361096 as the currently used additional :not() pseudo class make the :hover and :active rules in the original [w|p]instripe help.css more specific than the ones in the overlay and therefore overriding ours. For Modern, I basically added a new help component to themes/modern/, copying the help.css from extensions/help/resources/skin/modern/ with slight modifications (see the modern help.css diff I'll attach shortly for easier review). Note that you also need to copy the help*.gif files from that dir over to themes/modern/help/ for the patch to work correctly. While I was there, I also made Modern pick up some findBar.css file in global/ as it might need it (I don't think that needs really modern-specific styling currently).

Here's the diff of the two Modern theme help.css files, so that it's easier to see what my changes are during the review process. Basically, it's just making it fit the toolkit help dialog, no other changes.

Comment on attachment 245893 [details] diff of new and old Modern help.css, for easier review >+#context-copy[disabled="true"] { >+ visibility: collapse; > } Side note: visibility: collapse; is not a good idea for menuitems.

(In reply to comment #9) > (From update of attachment 245893 [details] [edit]) > >+#context-copy[disabled="true"] { > >+ visibility: collapse; > > } > Side note: visibility: collapse; is not a good idea for menuitems. I copied that from winstripe help.css - what would be better to do there (might be a good idea to do it in bug 361096 as well then)?

This patch is updated to match recent changes in my patch for bug 361096 - we're using the :not() stuff as well for the overrides, and modern uses display:none on #context-copy (which is also the only change to the modern file comapred to the last patch)

Comment on attachment 246927 [details] [diff] [review] patch v2.1: classic+modern patch, updated to match new bug 361096 patch >+ skin/modern/help/help.css (help/help.css) >+ skin/modern/help/helpFileLayout.css (/toolkit/themes/winstripe/help/helpFileLayout.css) >+ skin/modern/help/home.gif (help/home.gif) >+ skin/modern/help/home-act.gif (help/home-act.gif) >+ skin/modern/help/home-dis.gif (help/home-dis.gif) >+ skin/modern/help/home-hov.gif (help/home-hov.gif) Were you going to CVS copy these or should these really point to /extensions/help/resources/skin/modern ? >+/* make this look and act like a primary toolbar anywhere else */ Unfortunately acting like a primary toolbar includes observing the pictures and text preference. You'll need the navigator.css overrides to fix that. >+ >+ >+ Nit.

Comment on attachment 246927 [details] [diff] [review] patch v2.1: classic+modern patch, updated to match new bug 361096 patch Classic looks fine; you mentioned you wanted to get grippies to work but I don't see them in the browser either for some reason... Modern has the problem with the "text only" toolbar style.

(In reply to comment #12) > Were you going to CVS copy these or should these really point to > /extensions/help/resources/skin/modern ? I actually believe a direct copy should be OK there, the home images don't need much history preserved, we maybe could discuss help.css though... > >+/* make this look and act like a primary toolbar anywhere else */ > Unfortunately acting like a primary toolbar includes observing the pictures and > text preference. You'll need the navigator.css overrides to fix that. Unfortunately, we don't have all the labels set, so we can't even follow the text-only pref correctly. I fixed it to always show icon-only in the new patch (following in a minute).

This new patch "fixes" modern with "text only" toolbar style setting by forcing icon-only style (always display icon). I also fixed the nit and reworded comments to tell what's actually happening.

Comment on attachment 251395 [details] [diff] [review] patch v2.2: classic+modern, including fix for text-only in modern >+ skin/modern/communicator/helpOverlay.css (communicator/helpOverlay.css) Nit: file missing from patch (presumed same as previous patch) >+ skin/modern/global/findBar.css (/toolkit/themes/winstripe/global/findBar.css) Sorry I didn't spot this before, but I think Modern will need its own version of this; at least, the find bar looked really ugly. Separate bug if you like. >+#HelpToolbar .toolbarbutton-text { >+ display: none; >+} >+#HelpToolbar .toolbarbutton-icon { >+ display: -moz-box; >+} Nit: missing blank line between the two blocks >+/* -moz-box-orient: horizontal !important; >+ min-width: 0px; */ >+ list-style-image: url("chrome://navigator/skin/icons/btn1.gif"); >+/* margin: 0 !important; >+ padding: 4px 5px !important; */ Any reason for the comments? Otherwise r+sr=me with the nits fixed.

checked in with nits fixed, filed bug 367272 for a real modern findBar.