282180 - sync xpfe button.xml with toolkit button.xml

Status: RESOLVED FIXED

Description

[Mike Connor [:mconnor]]

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

see bug 282359 comment #11:
https://bugzilla.mozilla.org/show_bug.cgi?id=282359#c11

Comment 2

[Serge Gautherie (:sgautherie)]

Created attachment 176648 [details] [diff] [review]
(Av1) 2005.03.08 X2T diff report

Diff from Xpfe to Toolkit version.

Comment 3

[Stefan [:stefanh]]

(In reply to comment #2)
> Created an attachment (id=176648) [edit]
> 2005.03.08 X2T diff report
> 
> Diff from Xpfe to Toolkit version.

Looks like the diff hasn't changed.

Comment 4

[Justin Wood (:Callek)]

If the diff still has not changed, I would suggest this also be applied + a migration to toolkit's directly.

Comment 5

[Serge Gautherie (:sgautherie)]

Created attachment 217747 [details] [diff] [review]
(Av2) <button.xml>

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 217747 [details] [diff] [review]
(Av2) <button.xml>

(In reply to comment #4)
> If the diff still has not changed, I would suggest this also be applied + a
> migration to toolkit's directly.

Updated to current Trunk; untested.

Comment 7

[[e-mail]]

Comment on attachment 217747 [details] [diff] [review]
(Av2) <button.xml>

>-              if (sibs[i].checked)
>-                sibs[i].checked = false;
>+              sibs[i].removeAttribute("checked");
This change is good.

>-      <!-- While it would seem we could do this by handling oncommand, we can't
>-           because any external oncommand handlers might get called before ours,
>-           and then they would see the incorrect value of checked. Additionally
>-           a command attribute would redirect the command events anyway.-->
This comment still holds. This change is invalid.

>-          <xul:label class="button-text" xbl:inherits="value=label,accesskey,crop" flex="1"/>
>+          <xul:label class="button-text" xbl:inherits="value=label,accesskey,crop"/>
Sloppy patching in bug 233461 - all the bindings should have been patched.

Comment 8

[Serge Gautherie (:sgautherie)]

Created attachment 218477 [details] [diff] [review]
(Av3-XPFE) <button.xml>

Av2, with comment 7 suggestion(s).

Comment 9

[Serge Gautherie (:sgautherie)]

Created attachment 218478 [details] [diff] [review]
(Bv1-TK) <button.xml>

Per comment 7 suggestion(s), and email details from Neil.

Comment 10

[[e-mail]]

Comment on attachment 218477 [details] [diff] [review]
(Av3-XPFE) <button.xml>

>         <children>
>           <xul:image class="button-icon" xbl:inherits="src=image"/>
>-          <xul:label class="button-text" xbl:inherits="value=label,accesskey,crop" flex="1"/>
>+          <xul:label class="button-text" xbl:inherits="value=label,accesskey,crop"/>
>           <xul:dropmarker class="button-menu-dropmarker" xbl:inherits="open,disabled"/>
>         </children>
>       </xul:hbox>
This isn't quite working: the image is next to the label, which is fine, but the dropmarker is too, which isn't; I think moving the dropmarker out of the hbox should fix it, but I'd like someone from toolkit to agree.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 218478 [details] [diff] [review]
(Bv1-TK) <button.xml>

Sorry, it's not clear to me from the bugs that are referenced -- what is this patch fixing?

Comment 12

[Mike Connor [:mconnor]]

yeah, moving outside of the hbox should be fine, assuming themes aren't making bad assumptions.

Comment 13

[[e-mail]]

Comment on attachment 218478 [details] [diff] [review]
(Bv1-TK) <button.xml>

>           <xul:image class="button-icon" xbl:inherits="src=image"/>
>-          <xul:label class="button-text" xbl:inherits="value=label,accesskey,crop" flex="1"/>
>+          <xul:label class="button-text" xbl:inherits="value=label,accesskey,crop"/>
Obsoleting sort-of as per comment 12; unfortunately my original idea to move the dropmarker outside the <hbox> also moves it outside the <children>, which would mean that <button type="menu"><foo/></button> won't hide the dropmarker (of course <button type="menu-button"> always has a dropmarker) so maybe it's better to create a duplicate <hbox> just for these two elements.

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)
> Obsoleting sort-of as per comment 12; unfortunately my original idea to move
> the dropmarker outside the <hbox> also moves it outside the <children>, which
> would mean that <button type="menu"><foo/></button> won't hide the dropmarker
> (of course <button type="menu-button"> always has a dropmarker) so maybe it's
> better to create a duplicate <hbox> just for these two elements.

children + hbox + vbox : +/- like bug 282189.!?.
(As a sidenote, the labels in toolbarbutton.xml still have their |flex="1"|...)

Comment 15

[[e-mail]]

(In reply to comment #14)
>(As a sidenote, the labels in toolbarbutton.xml still have their |flex="1"|...)
Yes but they're not centred so that's OK.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> (In reply to comment #13)
> > Obsoleting sort-of as per comment 12; unfortunately my original idea to move
> > the dropmarker outside the <hbox> also moves it outside the <children>, which
> > would mean that <button type="menu"><foo/></button> won't hide the dropmarker
> > (of course <button type="menu-button"> always has a dropmarker) so maybe it's
> > better to create a duplicate <hbox> just for these two elements.
> 
> children + hbox + vbox : +/- like bug 282189.!?.

This should have been hbox + children + hbox, like
[[
      <xul:hbox class="box-inherit button-box" xbl:inherits="align,dir,pack,orient"
                align="center" pack="center" flex="1">
        <children>
          <xul:hbox>
            <xul:image class="button-icon" xbl:inherits="src=image"/>
            <xul:label class="button-text" xbl:inherits="value=label,accesskey,crop"/>
          </xul:hbox>
          <xul:dropmarker class="button-menu-dropmarker" xbl:inherits="open,disabled"/>
        </children>
      </xul:hbox>
]]
but I don't know if any attibute should be moved/added to the inner hbox ?

Comment 17

[[e-mail]]

(In reply to comment #16)
>but I don't know if any attibute should be moved/added to the inner hbox ?
Without trying it out I'd guess that the inner hbox should have the same attributes as the outer hbox except that its class should be just box-inherit.

Comment 18

[Serge Gautherie (:sgautherie)]

Created attachment 221700 [details] [diff] [review]
(Av4-XPFE) <button.xml>
[Checkin: Comment 22 & 23]

Av3-XPFE, with comment 17 suggestion(s).

Untested: Would you have a testcase to check this behaviour ?

Comment 19

[[e-mail]]

Created attachment 221763 [details]
Test (for the |flex| issue)

If I got my CSS right this test should also work in Firefox.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 221700 [details] [diff] [review]
(Av4-XPFE) <button.xml>
[Checkin: Comment 22 & 23]

'approval-branch-1.8.1=?': (SeaMonkey only)
Simple U.I. code synchronization (was Toolkit->XPFE, now Trunk->branch), no
risk.

Comment 21

[Serge Gautherie (:sgautherie)]

Created attachment 221775 [details] [diff] [review]
(Bv1a-TK) <button.xml>
[Checkin: Comment 37]

Bv1-TK, synchronized from Av4-XPFE.

I checked that this fixes neil's testcase in
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060511 BonEcho/2.0a2] (nightly) (W98SE)
too: 'menu' image and label are centered/next to each other.

The event handling part is untested in FF,
as I don't know what would be the steps/testcase for this;
discuss it directly with neil if needed.

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 221700 [details] [diff] [review]
(Av4-XPFE) <button.xml>
[Checkin: Comment 22 & 23]


Checkin: { 2006-05-13 04:03	bugzilla%standard8.demon.co.uk 	mozilla/xpfe/global/resources/content/bindings/button.xml 	1.29 }

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 221700 [details] [diff] [review]
(Av4-XPFE) <button.xml>
[Checkin: Comment 22 & 23]


Checkin: {
2006-05-17 08:40	neil%parkwaycc.co.uk 	mozilla/xpfe/global/resources/content/bindings/button.xml 	1.27.4.2 	MOZILLA_1_8_BRANCH
}

Comment 24

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> Created an attachment (id=221775) [edit]
> (Bv1a-TK) <button.xml>
> 
> The event handling part is untested in FF,
> as I don't know what would be the steps/testcase for this;
> discuss it directly with neil if needed.

This part comes from bug 230854 followed by bug 259724.

Comment 25

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060518 SeaMonkey/1.1a] (nightly) (W98SE)

V.Fixed on MOZILLA_1_8_BRANCH, per 'flex' testcase.

Comment 26

[Serge Gautherie (:sgautherie)]

Created attachment 222566 [details]
HTML testcase (for the |checked| issue, to compare with)

The HTML button first changes the value, then fires the onclick event.
(And this is what SeaMonkey XUL button does.)

Comment 27

[Serge Gautherie (:sgautherie)]

Created attachment 222568 [details]
XUL testcase (for the |checked| issue)

(The first 3+2 buttons were collected from bug 230854, and are not much useful for this bug)
Test 'disabled' with the 2 "press me" buttons; (collected from bug 259724, Toolkit non-regression only)
Test 'checked' with the 2 "Neil" buttons. (Toolkit fix !)

Notice that the button label changes position as the button is (un)checked.

The Toolkit button current behaviour is wrong (== it has bug 230854):
*the button with command is never checked, only the command event is fired.
*the button with oncommand checks and unchecks twice, and the oncommand event sees the value before it is swapped.

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 221775 [details] [diff] [review]
(Bv1a-TK) <button.xml>
[Checkin: Comment 37]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060517 BonEcho/2.0a2] (nightly) (W98SE)

This patch is now fully tested, as I used the attached testcase to test the event handling code.

'approval-branch-1.8.1=?': (Toolkit only)
Simple U.I. code synchronization & fix (was XPFE->Toolkit, now Trunk->branch), low
risk.
Brian/Mike, any of you could do both r?/a?...

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 222568 [details]
XUL testcase (for the |checked| issue)

(not directly related to this bug)

I noticed (in both FF and SM) a behaviour that seems "odd" to me:
*when loading the testcase from https://, the "test" label from the <command> overrides the one from the <button>s.
*when loading from file://, the <button>s keep their own labels.

Why would that be ?

Comment 30

[[e-mail]]

(In reply to comment #29)
>I noticed (in both FF and SM) a behaviour that seems "odd" to me:
>*when loading the testcase from https://, the "test" label from the <command>
>overrides the one from the <button>s.
>*when loading from file://, the <button>s keep their own labels.
> 
>Why would that be ?
The code that links the labels to the buttons is activated by the xul+xml type.
Rename your file to .xul to see the difference.

Comment 31

[Serge Gautherie (:sgautherie)]

(In reply to comment #30)
> The code that links the labels to the buttons is activated by the xul+xml type.
> Rename your file to .xul to see the difference.

(Done: I wonder why this affects the command labels only, but fine with me.)

Comment 32

[Robert Kaiser]

SeaMonkey is now using toolkit - is there still anything we need to port from xpfe button.xml to toolkit#s or can we close this now?

Comment 33

[[e-mail]]

(In reply to comment #32)
>SeaMonkey is now using toolkit - is there still anything we need to port from
>xpfe button.xml to toolkit's or can we close this now?
Yes, at least one fix is outstanding.

Comment 34

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Comment on attachment 221775 [details] [diff] [review]
(Bv1a-TK) <button.xml>
[Checkin: Comment 37]

r=mano

Comment 35

[[e-mail]]

Comment on attachment 221775 [details] [diff] [review]
(Bv1a-TK) <button.xml>
[Checkin: Comment 37]

This code has baked for 3 years or so in SeaMonkey so I'd say it's a safe patch ;-)

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 221775 [details] [diff] [review]
(Bv1a-TK) <button.xml>
[Checkin: Comment 37]

a=drivers for after M9 freeze

Comment 37

[Reed Loden [:reed] (use needinfo?)]

Checking in toolkit/content/widgets/button.xml;
/cvsroot/mozilla/toolkit/content/widgets/button.xml,v  <--  button.xml
new revision: 1.20; previous revision: 1.19
done

Comment 38

[Serge Gautherie (:sgautherie)]

There are 2 "new" differences:

1) Bug 337532 comment 15:
Toolkit only:
nothing more to do :-)

2) Bug 156459 comment 7:
Xpfe only:
Neil, can you confirm that we want to port that one to Toolkit ?
If so, I plan to add the port patch directly to that bug...

Comment 39

[[e-mail]]

(In reply to comment #38)
>2) Bug 156459 comment 7:
>Xpfe only:
>Neil, can you confirm that we want to port that one to Toolkit ?
>If so, I plan to add the port patch directly to that bug...
Yes, that would be nice, although not important; maybe ask Mano for review?

Comment 40

[Jonathan Steele]

I'm uses the latest nightly builds of Minefield. I has been seeing this error message which is Error: this._handleClick is not a function. It is in http://mxr.mozilla.org/firefox/source/toolkit/content/widgets/button.xml#140

Comment 41

[Serge Gautherie (:sgautherie)]

(In reply to comment #40)
Could you file a dependent bug, with steps to reproduce and other relevant details ? Thanks.

Comment 42

[Serge Gautherie (:sgautherie)]

(In reply to comment #41)
> (In reply to comment #40)
> Could you file a dependent bug, with steps to reproduce and other relevant
> details ? Thanks.

Bug 403162 was filed :-)