Closed Bug 282180 Opened 16 years ago Closed 14 years ago

sync xpfe button.xml with toolkit button.xml

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: mconnor, Assigned: sgautherie)

References

Details

(Keywords: fixed-seamonkey1.1a, fixed1.8.1, testcase, Whiteboard: [fixed1.8.1 applies to Av4-XPFE patch only] [verified-seamonkey1.1a])

Attachments

(5 files, 4 obsolete files)

 
Blocks: 282177
Depends on: 282359
Attached patch (Av1) 2005.03.08 X2T diff report (obsolete) — Splinter Review
Diff from Xpfe to Toolkit version.
(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.
If the diff still has not changed, I would suggest this also be applied + a migration to toolkit's directly.
Attached patch (Av2) <button.xml> (obsolete) — Splinter Review
Assignee: nobody → gautheri
Attachment #176648 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #176648 - Attachment description: 2005.03.08 X2T diff report → (Av1) 2005.03.08 X2T diff report
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.
Attachment #217747 - Flags: superreview?(neil)
Attachment #217747 - Flags: review?(neil)
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.
Attachment #217747 - Flags: superreview?(neil)
Attachment #217747 - Flags: superreview-
Attachment #217747 - Flags: review?(neil)
Attachment #217747 - Flags: review-
Attachment #217747 - Attachment description: (Av2) <button.xml> → (Av2) <button.xml> (TK + XPFE)
Attachment #217747 - Attachment is obsolete: true
Attachment #217747 - Attachment description: (Av2) <button.xml> (TK + XPFE) → (Av2) <button.xml>
Attached patch (Av3-XPFE) <button.xml> (obsolete) — Splinter Review
Av2, with comment 7 suggestion(s).
Attachment #218477 - Flags: superreview?
Attachment #218477 - Flags: review?
Attached patch (Bv1-TK) <button.xml> (obsolete) — Splinter Review
Per comment 7 suggestion(s), and email details from Neil.
Attachment #218478 - Flags: review?(bryner)
Attachment #218477 - Flags: superreview?(neil)
Attachment #218477 - Flags: superreview?
Attachment #218477 - Flags: review?(neil)
Attachment #218477 - Flags: review?
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.
Attachment #218477 - Flags: superreview?(neil)
Attachment #218477 - Flags: review?(neil)
Attachment #218477 - Flags: review-
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?
yeah, moving outside of the hbox should be fine, assuming themes aren't making bad assumptions.
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.
Attachment #218478 - Attachment is obsolete: true
Attachment #218478 - Flags: review?(bryner)
(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"|...)
(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.
(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 ?
Keywords: helpwanted
(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.
Av3-XPFE, with comment 17 suggestion(s).

Untested: Would you have a testcase to check this behaviour ?
Attachment #218477 - Attachment is obsolete: true
Attachment #221700 - Flags: superreview?
Attachment #221700 - Flags: review?
Attachment #221700 - Flags: superreview?(neil)
Attachment #221700 - Flags: superreview?
Attachment #221700 - Flags: review?(neil)
Attachment #221700 - Flags: review?
If I got my CSS right this test should also work in Firefox.
Attachment #221700 - Flags: superreview?(neil)
Attachment #221700 - Flags: superreview+
Attachment #221700 - Flags: review?(neil)
Attachment #221700 - Flags: review+
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.
Attachment #221700 - Flags: approval-branch-1.8.1?(neil)
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.
Attachment #221775 - Flags: review?(mconnor)
Attachment #221700 - Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
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 }
Attachment #221700 - Attachment description: (Av4-XPFE) <button.xml> → (Av4-XPFE) <button.xml> [Checkin: Comment 22]
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
}
Attachment #221700 - Attachment description: (Av4-XPFE) <button.xml> [Checkin: Comment 22] → (Av4-XPFE) <button.xml> [Checkin: Comment 22 & 23]
Attachment #221700 - Attachment is obsolete: true
Whiteboard: [1.8.1+SM1.1a apply to Av4-XPFE patch only]
Target Milestone: --- → mozilla1.9alpha
Attachment #221763 - Attachment description: Test → Test (for the |flex| issue)
(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.
Depends on: 230854, 259724
[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.
Whiteboard: [1.8.1+SM1.1a apply to Av4-XPFE patch only] → [fixed1.8.1 applies to Av4-XPFE patch only] [verified-seamonkey1.1a]
The HTML button first changes the value, then fires the onclick event.
(And this is what SeaMonkey XUL button does.)
(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 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?...
Attachment #221775 - Flags: approval-branch-1.8.1?(bryner)
Keywords: helpwantedtestcase
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 ?
(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.
(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.)
Attachment #221775 - Flags: approval-branch-1.8.1?(bryner)
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?
(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 on attachment 221775 [details] [diff] [review]
(Bv1a-TK) <button.xml>
[Checkin: Comment 37]

r=mano
Attachment #221775 - Flags: review?(mconnor)
Attachment #221775 - Flags: review+
Attachment #221775 - Flags: approval1.9?
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 ;-)
Attachment #221775 - Flags: approvalM9?
Comment on attachment 221775 [details] [diff] [review]
(Bv1a-TK) <button.xml>
[Checkin: Comment 37]

a=drivers for after M9 freeze
Attachment #221775 - Flags: approvalM9?
Attachment #221775 - Flags: approvalM9-
Attachment #221775 - Flags: approval1.9?
Attachment #221775 - Flags: approval1.9+
Flags: in-testsuite?
Whiteboard: [fixed1.8.1 applies to Av4-XPFE patch only] [verified-seamonkey1.1a] → [c-n: Bv1a-TK] [fixed1.8.1 applies to Av4-XPFE patch only] [verified-seamonkey1.1a]
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: Bv1a-TK] [fixed1.8.1 applies to Av4-XPFE patch only] [verified-seamonkey1.1a] → [fixed1.8.1 applies to Av4-XPFE patch only] [verified-seamonkey1.1a]
Attachment #221700 - Attachment is obsolete: false
Attachment #221775 - Attachment description: (Bv1a-TK) <button.xml> → (Bv1a-TK) <button.xml> [Checkin: Comment 37]
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...
Depends on: 156459, 337532
Target Milestone: mozilla1.9alpha1 → mozilla1.9 M10
(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?
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
(In reply to comment #40)
Could you file a dependent bug, with steps to reproduce and other relevant details ? Thanks.
Depends on: 403162
(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 :-)
You need to log in before you can comment on or make changes to this bug.