257280 - Get rid of buttondown and buttonover attributes

Status: RESOLVED INVALID

(Toolkit :: UI Widgets, defect)

Description

[Serge Gautherie (:sgautherie)]

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

Comment 1

[Serge Gautherie (:sgautherie)]

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;"/>
}}

Comment 2

[[e-mail]]

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.

Comment 3

[Serge Gautherie (:sgautherie)]

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| :-<

Comment 4

[[e-mail]]

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.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <editorPrimaryToolbar.css> [Check in: See comment 14]

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 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 159322 [details] [diff] [review]
(Av1) <editorPrimaryToolbar.css>
[Check in: See comment 14]

See my previous comment about a possible regression/enhancement ;->

Comment 7

[Serge Gautherie (:sgautherie)]

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

Comment 8

[[e-mail]]

(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 [e-mail] :-)

Comment 9

[Jan Varga [:janv]]

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

Comment 10

[Serge Gautherie (:sgautherie)]

(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 ?

Comment 11

[[e-mail]]

(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 ;-)

Comment 12

[Alfred Kayser]

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.

Comment 13

[[e-mail]]

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 14

[Serge Gautherie (:sgautherie)]

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 15

[[e-mail]]

Comment on attachment 159322 [details] [diff] [review]
(Av1) <editorPrimaryToolbar.css>
[Check in: See comment 14]

Well, not aviary, unless you mean /toolkit (r?[e-mail]) and
/mail (r?[e-mail])

Comment 16

[Alfred Kayser]

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

Comment 17

[[e-mail]]

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.

Comment 18

[Alfred Kayser]

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?

Comment 19

[Justin Wood (:Callek)]

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 ;-) )

Comment 20

[Serge Gautherie (:sgautherie)]

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 ?

Comment 21

[Serge Gautherie (:sgautherie)]

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

Comment 22

[Serge Gautherie (:sgautherie)]

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

Comment 23

[[e-mail]]

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.

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) "Neil-David" [Checked in: Comment 29]

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

Comment 25

[[e-mail]]

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 26

[[e-mail]]

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.

Comment 27

[Serge Gautherie (:sgautherie)]

[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 28

[[e-mail]]

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 29

[Serge Gautherie (:sgautherie)]

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 }

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) "Neil-Neil"

(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 :-)

Comment 31

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a) "Neil-Neil" [Checked in: Comment 36]

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 32

[Serge Gautherie (:sgautherie)]

Comment on attachment 168514 [details] [diff] [review]
(Bv1a) "Neil-Neil"
[Checked in: Comment 36]

Could you (super-)review/check in this patch ? Thanks.

Comment 33

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1a) "Neil-Neil" [Checked in: Comment 47]

Bv1, UNtested part, as I did.

Comment 34

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1b) "Neil-Neil" (for comparison)

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 35

[Serge Gautherie (:sgautherie)]

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 36

[Serge Gautherie (:sgautherie)]

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 }

Comment 37

[Serge Gautherie (:sgautherie)]

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

Comment 38

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) "toolkit" <help.css> [Checked in: Comment 59]

"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 39

[Serge Gautherie (:sgautherie)]

Comment on attachment 168547 [details] [diff] [review]
(Ev1) "toolkit" <help.css>
[Checked in: Comment 59]

Neil: (I'll CC [e-mail])

I don't use these themes: Could you test/"(super-)review"/check in this patch ?
Thanks.

Comment 40

[Serge Gautherie (:sgautherie)]

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

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 42

[[e-mail]]

(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

Comment 43

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1) <navigator.css> additional

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

Comment 44

[Serge Gautherie (:sgautherie)]

Comment on attachment 168552 [details] [diff] [review]
(Fv1) <navigator.css> additional

(untested, but obvious)
Could you test/(super-)review/check in this patch ? Thanks.

Comment 45

[Serge Gautherie (:sgautherie)]

(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 46

[[e-mail]]

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 47

[Serge Gautherie (:sgautherie)]

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 }

Comment 48

[Serge Gautherie (:sgautherie)]

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 ?

Comment 49

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1) <button.css> additional [Checked in: Comment 50]

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 50

[Serge Gautherie (:sgautherie)]

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 }

Comment 51

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1) "toolkit" <toolbarbutton.css>

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

Comment 52

[Serge Gautherie (:sgautherie)]

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.

Comment 53

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1a) "toolkit" <*button.css>

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

Comment 54

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1b) "toolkit" <*button.css> ++

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

Comment 55

[Serge Gautherie (:sgautherie)]

Attachment: (Iv1) <tabbox.css> ++ [Checked in: Comment 56]

Not a bug, simple code "reordering".

(untested, but obvious)
Could you test/(super-)review/check in this patch ? Thanks.

Comment 56

[Serge Gautherie (:sgautherie)]

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 }

Comment 57

[Serge Gautherie (:sgautherie)]

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

Comment 58

[Jeff Walden [:Waldo]]

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 59

[Jeff Walden (gone starting June 8)]

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

Comment 60

[Serge Gautherie (:sgautherie)]

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)
}}

Comment 61

[Asaf Romano (gone)]

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

Comment 62

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1c) "toolkit" <*button.css> ++

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.

Comment 63

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1a) <navigator.css> additional [Checked in: Comment 64]

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 64

[Serge Gautherie (:sgautherie)]

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 }

Comment 65

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1d) "toolkit" <*button.css> ++

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 <[e-mail]> since "2005-01-26" :-(
Trying mconnor + bryner, as in bug 248579.

Comment 66

[Brian Ryner (not reading)]

Comment on attachment 177813 [details] [diff] [review]
(Hv1d) "toolkit" <*button.css> ++

I don't have time to verify that this doesn't regress anything.

Comment 67

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1e) "toolkit" <*button.css> ++

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 68

[Mike Connor [:mconnor]]

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 [e-mail] if you create a new version of the patch.

Comment 69

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1f) "toolkit" <*button.css> ++

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.

Comment 70

[Serge Gautherie (:sgautherie)]

Attachment: (Jv1) </calendar/*/calendar-daypicker.css> [Checkin: Comment 73]

(untested, but obvious)

NB: Not sure where to find out who to ask review from...

Comment 71

[Daniel Boelzle [:dbo]]

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

Moving review over to Fallen.

Comment 72

[Philipp Kewisch [: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 73

[Serge Gautherie (:sgautherie)]

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

http://hg.mozilla.org/comm-central/rev/5a17ea83a93b

Comment 74

[Serge Gautherie (:sgautherie)]

Attachment: (Kv1) </suite/themes/modern/global/globalBindings.xml> [Checkin: Comment 76]

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

Comment 75

[[e-mail]]

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

rs=me

Comment 76

[Serge Gautherie (:sgautherie)]

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 77

[Serge Gautherie (:sgautherie)]

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

rflint: looking for r.

Comment 78

[Dão Gottwald [:dao]]

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?

Comment 79

[Dão Gottwald [:dao]]

Serge, do you want to finish this?

Comment 80

[Serge Gautherie (:sgautherie)]

(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

Comment 81

[Serge Gautherie (:sgautherie)]

Attachment: (Hv2) /toolkit/themes/* whitespace cleanup [Checkin: See comment 86]

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 82

[Dão Gottwald [:dao]]

Comment on attachment 424802 [details] [diff] [review]
(Hv2) /toolkit/themes/* whitespace cleanup
[Checkin: See comment 86]

># HG changeset patch
># User Serge Gautherie <[e-mail]>
>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;

Comment 83

[Serge Gautherie (:sgautherie)]

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

Comment 84

[Dão Gottwald [:dao]]

We should remove it.

Comment 85

[Serge Gautherie (:sgautherie)]

Attachment: (Lv1) button.xml: Remove support code, Do (some) whitespace cleanup.

At last! :-)

Comment 86

[Serge Gautherie (:sgautherie)]

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

Comment 87

[Serge Gautherie (:sgautherie)]

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

http://hg.mozilla.org/mozilla-central/rev/dd13c8f0c2ef !

Comment 88

[[e-mail]]

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 89

[Dão Gottwald [:dao]]

Comment on attachment 424820 [details] [diff] [review]
(Lv1) button.xml: Remove support code, Do (some) whitespace cleanup.

>From: Serge Gautherie <[e-mail]>
>
>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.

Comment 90

[Serge Gautherie (:sgautherie)]

Attachment: (Lv2) button.xml: Remove support code, Do (some) whitespace cleanup

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

Comment 91

[Dão Gottwald [:dao]]

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 92

[[e-mail]]

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 93

[Dão Gottwald [:dao]]

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

See Neil's previous comment...

Comment 94

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:mstriemer, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 95

[Mark Striemer [:mstriemer]]

I don't see any buttonover or buttondown CSS styles anymore. Looks like this is probably invalid now