Closed
Bug 282180
Opened 20 years ago
Closed 18 years ago
sync xpfe button.xml with toolkit button.xml
Categories
(Core :: XUL, defect)
Core
XUL
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)
2.01 KB,
patch
|
neil
:
review+
neil
:
superreview+
neil
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
466 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
2.58 KB,
patch
|
asaf
:
review+
beltzner
:
approvalM9-
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
344 bytes,
text/html
|
Details | |
960 bytes,
application/vnd.mozilla.xul+xml
|
Details |
Assignee | ||
Comment 2•20 years ago
|
||
Diff from Xpfe to Toolkit version.
Comment 3•19 years ago
|
||
(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•19 years ago
|
||
If the diff still has not changed, I would suggest this also be applied + a migration to toolkit's directly.
Assignee | ||
Comment 5•19 years ago
|
||
Assignee | ||
Updated•19 years ago
|
Attachment #176648 -
Attachment description: 2005.03.08 X2T diff report → (Av1) 2005.03.08 X2T diff report
Assignee | ||
Comment 6•19 years ago
|
||
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 7•19 years ago
|
||
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-
Assignee | ||
Updated•19 years ago
|
Attachment #217747 -
Attachment description: (Av2) <button.xml> → (Av2) <button.xml> (TK + XPFE)
Attachment #217747 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #217747 -
Attachment description: (Av2) <button.xml> (TK + XPFE) → (Av2) <button.xml>
Assignee | ||
Comment 8•19 years ago
|
||
Av2, with comment 7 suggestion(s).
Attachment #218477 -
Flags: superreview?
Attachment #218477 -
Flags: review?
Assignee | ||
Comment 9•19 years ago
|
||
Per comment 7 suggestion(s), and email details from Neil.
Attachment #218478 -
Flags: review?(bryner)
Assignee | ||
Updated•19 years ago
|
Attachment #218477 -
Flags: superreview?(neil)
Attachment #218477 -
Flags: superreview?
Attachment #218477 -
Flags: review?(neil)
Attachment #218477 -
Flags: review?
Comment 10•19 years ago
|
||
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 11•19 years ago
|
||
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?
Reporter | ||
Comment 12•19 years ago
|
||
yeah, moving outside of the hbox should be fine, assuming themes aren't making bad assumptions.
Comment 13•19 years ago
|
||
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)
Assignee | ||
Comment 14•19 years ago
|
||
(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•19 years ago
|
||
(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.
Assignee | ||
Comment 16•19 years ago
|
||
(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
Comment 17•19 years ago
|
||
(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.
Assignee | ||
Comment 18•19 years ago
|
||
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?
Assignee | ||
Updated•19 years ago
|
Attachment #221700 -
Flags: superreview?(neil)
Attachment #221700 -
Flags: superreview?
Attachment #221700 -
Flags: review?(neil)
Attachment #221700 -
Flags: review?
Comment 19•19 years ago
|
||
If I got my CSS right this test should also work in Firefox.
Updated•19 years ago
|
Attachment #221700 -
Flags: superreview?(neil)
Attachment #221700 -
Flags: superreview+
Attachment #221700 -
Flags: review?(neil)
Attachment #221700 -
Flags: review+
Assignee | ||
Comment 20•19 years ago
|
||
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)
Assignee | ||
Comment 21•19 years ago
|
||
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)
Updated•19 years ago
|
Attachment #221700 -
Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
Assignee | ||
Comment 22•19 years ago
|
||
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]
Assignee | ||
Comment 23•19 years ago
|
||
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
Assignee | ||
Updated•19 years ago
|
Keywords: fixed-seamonkey1.1a,
fixed1.8.1
Whiteboard: [1.8.1+SM1.1a apply to Av4-XPFE patch only]
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Updated•19 years ago
|
Attachment #221763 -
Attachment description: Test → Test (for the |flex| issue)
Assignee | ||
Comment 24•19 years ago
|
||
(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.
Assignee | ||
Comment 25•19 years ago
|
||
[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]
Assignee | ||
Comment 26•19 years ago
|
||
The HTML button first changes the value, then fires the onclick event.
(And this is what SeaMonkey XUL button does.)
Assignee | ||
Comment 27•19 years ago
|
||
(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.
Assignee | ||
Comment 28•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Keywords: helpwanted → testcase
Assignee | ||
Comment 29•19 years ago
|
||
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•19 years ago
|
||
(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.
Assignee | ||
Comment 31•19 years ago
|
||
(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.)
Reporter | ||
Updated•19 years ago
|
Attachment #221775 -
Flags: approval-branch-1.8.1?(bryner)
![]() |
||
Comment 32•18 years ago
|
||
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•18 years ago
|
||
(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•18 years ago
|
||
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 35•18 years ago
|
||
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 36•18 years ago
|
||
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+
Updated•18 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•18 years ago
|
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]
Comment 37•18 years ago
|
||
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: 18 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]
Assignee | ||
Updated•18 years ago
|
Attachment #221700 -
Attachment is obsolete: false
Assignee | ||
Updated•18 years ago
|
Attachment #221775 -
Attachment description: (Bv1a-TK) <button.xml> → (Bv1a-TK) <button.xml>
[Checkin: Comment 37]
Assignee | ||
Comment 38•18 years ago
|
||
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•18 years ago
|
||
(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•18 years ago
|
||
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
Assignee | ||
Comment 41•18 years ago
|
||
(In reply to comment #40)
Could you file a dependent bug, with steps to reproduce and other relevant details ? Thanks.
Assignee | ||
Comment 42•18 years ago
|
||
(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.
Description
•