Here is an email from Neil to me: {{ Serge GAUTHERIE wrote: > Neil Rashbrook wrote: > >> Serge GAUTHERIE wrote: >> >>> Neil Rashbrook wrote: >>> >>>> should make it possible for the [buttondown] and [buttonover] rules for some buttons of the themes to work using :active and :hover rules. I think it would be great if we could switch them all to :active and :hover. >>> >>> >> I don't have bug or a list, but I do have an example: >> http://lxr.mozilla.org/seamonkey/source/themes/modern/editor/editorPrimaryToolbar.css#146 >> I think it should be possible to use :hover and :active like all the other buttons. >> I know it didn't work before, but some of the bugs only got fixed recently. > > {{ > 146 #printButton[buttonover="true"] { > 147 -moz-image-region: rect(34px 99px 67px 50px); > 148 } > > 162 #linkButton:hover { > 163 -moz-image-region: rect(102px 99px 135px 50px); > 164 } > }} > > What should I do ? > Replace the line by > 146 #printButton:hover { > and test some(/all) of the involved buttons !? Yes please. }}

I am not sure that my lists are 100% correct, but they should do it, isn't it ? <http://lxr.mozilla.org/mozilla/search?string=printButton> To fix: {{ /composer/base/skin/editorPrimaryToolbar.css, line 234 -- #printButton[buttonover="true"] { /composer/base/skin/editorPrimaryToolbar.css, line 238 -- #printButton[buttondown="true"] { /themes/classic/editor/editorPrimaryToolbar.css, line 132 -- #printButton[buttonover="true"] { /themes/classic/editor/editorPrimaryToolbar.css, line 136 -- #printButton[buttondown="true"] { /themes/modern/editor/editorPrimaryToolbar.css, line 146 -- #printButton[buttonover="true"] { /themes/modern/editor/editorPrimaryToolbar.css, line 150 -- #printButton[buttondown="true"] { }} To test: {{ /editor/ui/composer/content/TextEditorAppShell.xul, line 146 -- <toolbarbutton id="printButton"/> /editor/ui/composer/content/editor.xul, line 203 -- <toolbarbutton id="printButton"/> /editor/ui/composer/content/editorOverlay.xul, line 738 -- <toolbarbutton id="printButton" type="menu-button" class="toolbarbutton-1" /composer/base/content/editor.xul, line 200 -- <toolbarbutton id="printButton"/> /composer/base/content/editorOverlay.xul, line 749 -- <toolbarbutton id="printButton" type="menu-button" class="toolbarbutton-1" /mail/base/content/mailWindowOverlay.xul, line 1794 -- <toolbarbutton id="button-print" type="menu-button" class="toolbarbutton-1" label="&printButton.label;" /mailnews/base/prefs/resources/content/pref-mailnews.xul, line 119 -- <checkbox id="printButton" /mailnews/base/resources/content/mailWindowOverlay.xul, line 1775 -- <toolbarbutton id="button-print" type="menu-button" class="toolbarbutton-1" label="&printButton.label;" /toolkit/components/printing/content/printdialog.xul, line 135 -- <data style="display:none;" id="printButton" label="&printButton.label;"/> /xpfe/components/prefwindow/resources/content/pref-navigator.xul, line 148 -- <checkbox id="printButton" /xpfe/global/resources/content/printdialog.xul, line 140 -- <data style="display:none;" id="printButton" label="&printButton.label;"/> }}

For this bug I am only interested in fixing the themes/ files that use the buttondown and buttonover attributes. In particular the initial Composer test only changes the themes/*/editor/editorPrimaryToolbar.css files. Once this bug is fixed other bugs would have to be opened for composer/ etc.

Agreed: I'll leave the composer/ file alone for this bug. May be my test list is too long: Does this affect only (html) editor, and perhaps compose(r) !? _How do I proceed to check if the change works or not ?_ I'm not familiar with |-moz-image-region| :-<

Right, those files are only used by the HTML web page editor (Composer). I'll call the three images normal, hover and activated. If you hover the print button, you should see the image change to hover. When you move the mouse away, it should change back to normal. Move the mouse back over the button and it will change back to the hover image. Then, try holding the mouse down on the print button, and it should have the activated image. Moving the mouse away should again change back to the normal image, and moving the mouse back will change back to the activated image. It should be possible to use :hover to make the image respond to the hover state and :hover:active to make the image respond to the activated state.

Both themes always work the same way: no theme issue. I checked your four cases interacting with the image (left) part of the button: same behaviour, no regression :-) I also checked interacting with the dropdown (right) part of the button: without the patch, the image part always stays normal (only its focus activates); with the patch, the image state dynamically responds to the same state as the dropdown. I let you decide if this is a regression or an enhancement !

Comment on attachment 159322 [details] [diff] [review] (Av1) <editorPrimaryToolbar.css> [Check in: See comment 14] See my previous comment about a possible regression/enhancement ;->

For the record, I tested with [Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a3) Gecko/20040817] (release) (W98SE)

(In reply to comment #6) >(From update of attachment 159322 [details] [diff] [review]) >See my previous comment about a possible regression/enhancement ;-> Bugfix, according to mconnor@steelgryphon.com :-)

Comment on attachment 159322 [details] [diff] [review] (Av1) <editorPrimaryToolbar.css> [Check in: See comment 14] don't forget to kill the code setting these attributes, r=varga

(In reply to comment #9) > (From update of attachment 159322 [details] [diff] [review]) > don't forget to kill the code setting these attributes, r=varga Jan / Neil: Can you point me to an example of such code to be removed ?

(In reply to comment #10) >(In reply to comment #9) >>(From update of attachment 159322 [details] [diff] [review]) >>don't forget to kill the code setting these attributes, r=varga >Can you point me to an example of such code to be removed ? Basically, just lxr for buttondown and buttonover - it's all got to go ;-)

What about the 'buttonover' and 'buttondown' attributes of toolbarbutton-menubutton? See: themes/modern/global/toolbarbutton.css I don't think that these can be removed, and changing these will have impact on all existing themes out there.

Files that need converting: /extensions/help/resources/skin/classic/help.css /extensions/help/resources/skin/modern/help.css /themes/classic/communicator/button.css /themes/classic/global/mac/toolbarbutton.css /themes/classic/global/unix/toolbarbutton.css /themes/classic/global/win/toolbarbutton.css /themes/classic/navigator/navigator.css /themes/modern/global/globalBindings.xml /themes/modern/navigator/navigator.css These I can give r+sr= for. /themes/classic/messenger/primaryToolbar.css /themes/classic/messenger/messengercompose/messengercompose.css /themes/classic/messenger/smime/msgCompSMIMEOverlay.css /themes/modern/messenger/primaryToolbar.css /themes/modern/messenger/messengercompose/messengercompose.css /themes/modern/messenger/smime/msgCompSMIMEOverlay.css These I can only give r= for, you'll need sr=bienvenu or someone. /xpfe/global/resources/content/bindings/button.xml I think we should go for r=varga on this one, and I can give sr=.

Comment on attachment 159322 [details] [diff] [review] (Av1) <editorPrimaryToolbar.css> [Check in: See comment 14] Check in: { 2004-09-21 17:01 neil%parkwaycc.co.uk } Note that Neil applied the same change to |#formButton| (in Classic theme). Jan and Neil: Can you set your r/sr, for the record ? Neil: Would this patch, and the upcoming ones, work in aviary branch ?

Comment on attachment 159322 [details] [diff] [review] (Av1) <editorPrimaryToolbar.css> [Check in: See comment 14] Well, not aviary, unless you mean /toolkit (r?mconnor@steelgryphon.com) and /mail (r?mscott@mozilla.org)

To still discuss this in this bug: I'm not clear how this can be done in toolbarbutton.css (in classic mode). My theme wants to do something similar as Classic: when hovering above the dualbutton: outline (outset) both the internal button itself, as well as the dropmarker, and highlight (background-color: #CCCCFF) the actually hovered item (button or the dropmarker). One would the following to work: toolbarbutton:hover > .toolbarbutton-menubutton-dropmarker, toolbarbutton:hover > .toolbarbutton-menubutton-button { border: 1px outset threedface; } .toolbarbutton-menubutton-dropmarker:hover, .toolbarbutton-menubutton-button:hover { background-color: #CCCCFF; } First style rule styles both items when the whole button is hovered, second style rule only the specific item. This works, except when the hovering is about the dropmarker: the background color should change, but it doesn't. (note, this is using the default binding, as also used in the Classic theme.)

Ah, so you need to style the dropmarker based on whether the button portion is hovered? Note that if Classic is trying to do this, then it's been broken ever since heirarchical hover was implemented, which was ages ago.

Indeed, this is true for all my themes: LittleMozilla, MicroMozilla, Walnut, Nautipolis (and others). One of the design principles to also show what items are 'active' by providing hover feedback. Classic works as intended, but doesn't provide hover feedback on the dropdown marker itself. Only the icon of the button itself responds on hovering on the button. Could it be that the dropmarker needs 'allowevents' to let the :hover thing work?

Taking, sent to me from Neil (Serge if you are still working on this feel free to take back, just please dont wait too long ;-) )

I'm preparing comment 13 "Neil-Neil" part... I'm wondering about comment 16 to 18: Should I change anything from how it was done in part A ?

I don't know what/how needs to be changed in /themes/modern/global/globalBindings.xml : helpwanted !

Justin: helpwanted I don't know how to change /xpfe/global/resources/content/bindings/button.xml, maybe you can take this one !?

Comments 16-18 just mean we have to leave button.xml for now. We can't use allowevents because that's what makes the button independent of the menu.

I checked {{ #button-getmsg #button-print #button-attach #button-save #button-security }} which work as expected. But I don't know what {{ #button-mark }} is, and how to test it...

Oh, I originally said I was only filing this for editor... so much for that ;-) You have to be reading news to see the Mark button.

Comment on attachment 164778 [details] [diff] [review] (Cv1) "Neil-David" [Checked in: Comment 29] I think you forgot the send button. The rest looks fine though.

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a4) Gecko/20040927] (release) (W98SE) Part C) {{ #button-mark }} works fine too. The send button code is already as wanted, isn't it: <http://lxr.mozilla.org/mozilla/search?string=button-send> :-> Part B) /themes/modern/global/globalBindings.xml : helpwanted !

Comment on attachment 164778 [details] [diff] [review] (Cv1) "Neil-David" [Checked in: Comment 29] Silly, that button-send was for a patch that's so old I've lost the bug number :-/

Comment on attachment 164778 [details] [diff] [review] (Cv1) "Neil-David" [Checked in: Comment 29] Check in: { 2004-11-06 12:52 neil%parkwaycc.co.uk }

(In reply to comment #13) Neil: [Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a4) Gecko/20040927] (release) (W98SE) > /extensions/help/resources/skin/classic/help.css > /extensions/help/resources/skin/modern/help.css Tested successfully :-) > /themes/classic/communicator/button.css > /themes/classic/global/mac/toolbarbutton.css > /themes/classic/global/unix/toolbarbutton.css > /themes/classic/global/win/toolbarbutton.css These, I'm not too sure what they mean and how to test them properly :-| > /themes/classic/navigator/navigator.css > /themes/modern/navigator/navigator.css Tested successfully :-)

Bv1, tested part, with Neil's email comment on <mozilla/themes/modern/navigator/navigator.css>: {{ That looks quite nice, except I don't like :hover:active[toolbarmode="small"] - I didn't even think it would work. Can you put the :hover:active after the [] bits because that's how we do it everywhere else. }} I changed 2*2 previously existing occurences of same syntax too. I tested again Back & Forward & Reload buttons; I assume Stop button still works too.

Comment on attachment 168514 [details] [diff] [review] (Bv1a) "Neil-Neil" [Checked in: Comment 36] Could you (super-)review/check in this patch ? Thanks.

Bv1, UNtested part, as I did.

Dv1a, UNtested, with Neil's email comment: {{ I'd have to compare that patch with a version that moves the :hover (and :active) to the .toolbarbutton-menubutton-button (or dropmarker) }} I did this, plus: *Applying it to line 103 of <button.css> too: I'm not sure about that one ... Tell me ! *Adding a comma at line 93 of <*/toolbarbutton.css>: I believe it missed, did it not ?

Comment on attachment 168519 [details] [diff] [review] (Dv1b) "Neil-Neil" (for comparison) Neil: Compare/Test Dv1a and Dv1b; then, I'll prepare Dv1c with your next (email) suggested step.

Comment on attachment 168514 [details] [diff] [review] (Bv1a) "Neil-Neil" [Checked in: Comment 36] Check in: { 2004-12-12 01:21 neil%parkwaycc.co.uk }

(In reply to comment #2) > Once this bug is fixed other bugs would have to be opened for composer/ etc. I filled bug 274277 for Composer.

"Same" as patch Bv1a; plus *pinstripe and qute: Adding a comma at line "110": I believe it missed, did it not ? *qute: Adding |:hover| at line 92: I believe it missed, did it not ?

Comment on attachment 168547 [details] [diff] [review] (Ev1) "toolkit" <help.css> [Checked in: Comment 59] Neil: (I'll CC ben@bengoodger.com) I don't use these themes: Could you test/"(super-)review"/check in this patch ? Thanks.

Ben: Could you have a look at comment 38 and 39 ? Thanks.

I'm a little concerned about the use of :hover:active here. Why not use just :active? It seems to me like people using keyboard navigation won't trigger :hover:active rules if they navigate to a button and press it using just keys. Or will they?

(In reply to comment #41) >I'm a little concerned about the use of :hover:active here. >Why not use just :active? Because the buttondown attribute code emulates :hover:active [this dates back to before we had real heirarchical :hover:active] >It seems to me like people using keyboard navigation a) keyboard navigation never updated the buttondown attribute b) toolbarbuttons generally don't accept keyboard focus anyway

Addition to Bv1a: not a bug, simple code merging.

Comment on attachment 168552 [details] [diff] [review] (Fv1) <navigator.css> additional (untested, but obvious) Could you test/(super-)review/check in this patch ? Thanks.

(In reply to comment #2) > Once this bug is fixed other bugs would have to be opened for composer/ etc. I filled bug 274306 for ThunderBird.

Comment on attachment 168519 [details] [diff] [review] (Dv1b) "Neil-Neil" (for comparison) I finally remembered why this doesn't work... anonymous button content never goes in to the hover or active state.

Comment on attachment 168516 [details] [diff] [review] (Dv1a) "Neil-Neil" [Checked in: Comment 47] Check in: { 2004-12-12 15:18 neil%parkwaycc.co.uk }

Neil: ('Dv1a' related) I believe |border: 2px solid;| could be removed from either http://lxr.mozilla.org/mozilla/source/themes/classic/communicator/button.css#92 or http://lxr.mozilla.org/mozilla/source/themes/classic/communicator/button.css#115 but I don't know which one ?

Addition to Dv1a: not a bug, simple code duplication removal. Neill answered me: {{ I think removing the second one makes more sense - the three styles are there to override the following styles which don't specify a border width either. }} (untested, but obvious) Could you test/(super-)review/check in this patch ? Thanks.

Comment on attachment 168734 [details] [diff] [review] (Gv1) <button.css> additional [Checked in: Comment 50] Check in: { 2004-12-15 01:40 neil%parkwaycc.co.uk mozilla/ themes/ classic/ communicator/ button.css 1.11 }

"Same" as patch Dv1a; plus *pinstripe: Adding a comma at line "63": I believe it missed, did it not ?

Comment on attachment 168800 [details] [diff] [review] (Hv1) "toolkit" <toolbarbutton.css> I don't use these themes: Could you test/"(super-)review"/check in this patch ? Thanks.

"Same" as patch Dv1a and Gv1; plus *pinstripe: Adding a comma at line "63": I believe it missed, did it not ?

"Same" as patch Dv1a and Gv1; plus *pinstripe: Adding a comma at line "63": I believe it missed, did it not ? *<tabbox.css>: "reordering". I don't use these themes: Could you test/"(super-)review"/check in this patch ? Thanks.

Not a bug, simple code "reordering". (untested, but obvious) Could you test/(super-)review/check in this patch ? Thanks.

Comment on attachment 168817 [details] [diff] [review] (Iv1) <tabbox.css> ++ [Checked in: Comment 56] Check in: { 2005-01-04 06:24 neil%parkwaycc.co.uk }

Neil, jwalden+fxhelp: Any hope to get the 2+1 remaining patches in v1.8b ?

Comment on attachment 168815 [details] [diff] [review] (Hv1b) "toolkit" <*button.css> ++ I'm not sure why I'm being pinged to review this, as it doesn't appear to touch Help and I'm not a theme person anyways. Passing review to someone more qualified...

Comment on attachment 168547 [details] [diff] [review] (Ev1) "toolkit" <help.css> [Checked in: Comment 59] ...this, on the other hand, I should be able to handle. (I missed this when I was scanning the patch list earlier for patches requesting review from me.) It looks okay to me, and it seems to be in the vein of the other attachments here. I'll check this in sometime later today, probably. r=jwalden

Asaf: May be you could help getting a review for this patch ? {{ (Hv1b) "toolkit" <*button.css> ++ patch 2004-12-15 16:21 PST 8.16 KB jwalden+bmo: review? (webmail) }}

No; Poke kmgerich (same email address), he probably missed your request.

Hv1b, updated to current Trunk, plus fix port from bug 285971. I don't use these themes: Could you test/review/check in this patch ? Thanks.

Fv1, with Neil's email suggestion(s) {{ I'm not convinced... it might be useful to remind theme authors that they could use different images if they wish. }} Merging the rules, and adding a comment, is more explicit than having distinct rules which could let think that the styles are different, if not looking closely...

Comment on attachment 177556 [details] [diff] [review] (Fv1a) <navigator.css> additional [Checked in: Comment 64] Check in: { 2005-03-17 15:57 neil%parkwaycc.co.uk }

Hv1c, plus fix (port from bug 285971) to bug 248579 and more. I don't use these themes: Could you test/(super-)review/check in this patch ? Thanks. No review from <webmail@kmgerich.com> since "2005-01-26" :-( Trying mconnor + bryner, as in bug 248579.

Comment on attachment 177813 [details] [diff] [review] (Hv1d) "toolkit" <*button.css> ++ I don't have time to verify that this doesn't regress anything.

Hv1d, updated to current Trunk. I don't use these themes: Could you test/review/check in this patch ? (Or who could help testing ?) Thanks.

Comment on attachment 221321 [details] [diff] [review] (Hv1e) "toolkit" <*button.css> ++ this is almost certainly bitrotted with all of the changes in the interim. Please request reviews from rflint@mozilla.com if you create a new version of the patch.

Hv1e, updated to current Trunk, with some more "white space" cleanup. It's untested, and I don't know if +toolbarbutton[type="menu-button"][disabled="true"]:hover:active > .toolbarbutton-menubutton-dropmarker { is still useful or not. (I'm not going to redo all the checks/searches I did 3.5 years ago :-<) There might even be more to do, but I hope this (at least) is wanted.

(untested, but obvious) NB: Not sure where to find out who to ask review from...

Comment on attachment 343505 [details] [diff] [review] (Jv1) </calendar/*/calendar-daypicker.css> [Checkin: Comment 73] Moving review over to Fallen.

Comment on attachment 343505 [details] [diff] [review] (Jv1) </calendar/*/calendar-daypicker.css> [Checkin: Comment 73] Looks good, r=philipp. No further review or approval needed for calendar.

Comment on attachment 343505 [details] [diff] [review] (Jv1) </calendar/*/calendar-daypicker.css> [Checkin: Comment 73] http://hg.mozilla.org/comm-central/rev/5a17ea83a93b

This would seem obvious as (now) unused, but double-check.

Comment on attachment 344087 [details] [diff] [review] (Kv1) </suite/themes/modern/global/globalBindings.xml> [Checkin: Comment 76] rs=me

Comment on attachment 344087 [details] [diff] [review] (Kv1) </suite/themes/modern/global/globalBindings.xml> [Checkin: Comment 76] http://hg.mozilla.org/comm-central/rev/1ff4df81f9af

Comment on attachment 343502 [details] [diff] [review] (Hv1f) "toolkit" <*button.css> ++ rflint: looking for r.

Comment on attachment 343502 [details] [diff] [review] (Hv1f) "toolkit" <*button.css> ++ >-toolbarbutton:hover:active[buttonstyle="text"], >-toolbarbutton[open="true"][buttonstyle="text"] { >+toolbarbutton[buttonstyle="text"]:hover:active, >+toolbarbutton[buttonstyle="text"][open="true"] { What's the point of this change? >-#help-back-button:hover .toolbarbutton-menubutton-dropmarker, >-#help-back-button[buttonover="true"] > .toolbarbutton-menubutton-dropmarker { >+#help-back-button:hover .toolbarbutton-menubutton-dropmarker, >+#help-back-button:hover > .toolbarbutton-menubutton-dropmarker { The first selector is redundant. >-.toolbarbutton-menubutton-dropmarker[disabled="true"] { >+.toolbarbutton-menubutton-dropmarker[disabled="true"], >+toolbarbutton[type="menu-button"][disabled="true"]:hover:active > .toolbarbutton-menubutton-dropmarker { > padding: 3px !important; > } What's the point of this change?

Serge, do you want to finish this?

(In reply to comment #54) > *pinstripe: Adding a comma at line "63": I believe it missed, did it > not ? That was fixed by http://hg.mozilla.org/mozilla-central/rev/027ad9919237 Add missing comma

This is what remains of Hv1f: as "usual", the same work was re-done by others in other bugs in the meantime :-/ (In reply to comment #78) > >-.toolbarbutton-menubutton-dropmarker[disabled="true"] { > >+.toolbarbutton-menubutton-dropmarker[disabled="true"], > >+toolbarbutton[type="menu-button"][disabled="true"]:hover:active > .toolbarbutton-menubutton-dropmarker { > > padding: 3px !important; > > } > > What's the point of this change? I don't know (anymore): see comment 69. Dropping it (ftb).

Comment on attachment 424802 [details] [diff] [review] (Hv2) /toolkit/themes/* whitespace cleanup [Checkin: See comment 86] ># HG changeset patch ># User Serge Gautherie <sgautherie.bz@free.fr> >Bug 257280 - Replace [buttondown] and [buttonover] rules for some buttons of _the themes_ (and extensions/help) to work using :active and :hover rules; >(Hv2) /toolkit/themes/* whitespace cleanup. Please don't land it with that commit message, as that patch has nothing to do with this bug. Just call it "whitespace cleanup". > .toolbarbutton-icon { >- padding: 1px 0px 1px 0px; >+ padding: 1px 0px 1px 0px; padding: 1px 0;

Dao, just to confirm, should we keep or remove the following code? http://mxr.mozilla.org/mozilla-central/search?string=%28buttondown%7Cbuttonover%29&regexp=1&case=1&find=%2Fcontent%2Fwidgets%2Fbutton.xml Found 14 matching lines

We should remove it.

At last! :-)

Comment on attachment 424802 [details] [diff] [review] (Hv2) /toolkit/themes/* whitespace cleanup [Checkin: See comment 86] http://hg.mozilla.org/mozilla-central/rev/2714d982f5b4 Hv2, with comment 82 suggestion(s).

(In reply to comment #86) > http://hg.mozilla.org/mozilla-central/rev/2714d982f5b4 http://hg.mozilla.org/mozilla-central/rev/dd13c8f0c2ef !

Comment on attachment 424820 [details] [diff] [review] (Lv1) button.xml: Remove support code, Do (some) whitespace cleanup. > <implementation implements="nsIDOMEventListener"> Need to remove this implements attribute too because that was only there to support the handleEvent method that you're removing. > <constructor> > this.init(); > </constructor> >- >+ > <method name="init"> > <body> > <![CDATA[ > var btn = document.getAnonymousElementByAttribute(this, "anonid", "button"); > if (!btn) > throw "XBL binding for <button type=\"menu-button\"/> binding must contain an element with anonid=\"button\""; This is all now unused.

Comment on attachment 424820 [details] [diff] [review] (Lv1) button.xml: Remove support code, Do (some) whitespace cleanup. >From: Serge Gautherie <sgautherie.bz@free.fr> > >Bug 257280 - Replace [buttondown] and [buttonover] rules for some buttons of _the themes_ (and extensions/help) to work using :active and :hover rules; >(Lv1) button.xml: Remove support code, Do (some) whitespace cleanup. Again, there's some stuff that doesn't belong in a commit message. It's expected to contain the bug number, the reviewer and a description of what the patch does or fixes. This patch in particular doesn't "Replace [buttondown] and [buttonover] rules for some buttons of _the themes_ (and extensions/help) to work using :active and :hover rules." This would clutter up http://hg.mozilla.org/mozilla-central/log/tip/toolkit/content/widgets/button.xml unnecessarily.

Lv1, with comment 88 and comment 89 suggestion(s).

Comment on attachment 425085 [details] [diff] [review] (Lv2) button.xml: Remove support code, Do (some) whitespace cleanup > <binding id="menu-button-base" >- extends="chrome://global/content/bindings/button.xml#button-base"> ... >- </binding> >+ extends="chrome://global/content/bindings/button.xml#button-base"/> Looks like this binding should be removed and toolbarbutton.xml#menu-button should extend chrome://global/content/bindings/button.xml#button-base. Or maybe it should just extend chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton now? Not sure if any of the button.xml stuff is needed.

Comment on attachment 425085 [details] [diff] [review] (Lv2) button.xml: Remove support code, Do (some) whitespace cleanup > <binding id="menu-button-base" >+ extends="chrome://global/content/bindings/button.xml#button-base"/> > > <binding id="menu-button" display="xul:menu" > extends="chrome://global/content/bindings/button.xml#menu-button-base"> > <resources> > <stylesheet src="chrome://global/skin/button.css"/> > </resources> Well, this can now extend #button directly, rather than #button-base, which will allow it to inherit the resources, rather than having to duplicate them. A similar idea applies to toolbarbutton.xml where the menu-button can extend the toolbarbutton in the same file to avoid duplication of resources.

Comment on attachment 425085 [details] [diff] [review] (Lv2) button.xml: Remove support code, Do (some) whitespace cleanup See Neil's previous comment...