Closed
Bug 282180
Opened 19 years ago
Closed 16 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•19 years ago
|
||
Diff from Xpfe to Toolkit version.
Comment 3•18 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•18 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•18 years ago
|
||
Assignee | ||
Updated•18 years ago
|
Attachment #176648 -
Attachment description: 2005.03.08 X2T diff report → (Av1) 2005.03.08 X2T diff report
Assignee | ||
Comment 6•18 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•18 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•18 years ago
|
Attachment #217747 -
Attachment description: (Av2) <button.xml> → (Av2) <button.xml> (TK + XPFE)
Attachment #217747 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Attachment #217747 -
Attachment description: (Av2) <button.xml> (TK + XPFE) → (Av2) <button.xml>
Assignee | ||
Comment 8•18 years ago
|
||
Av2, with comment 7 suggestion(s).
Attachment #218477 -
Flags: superreview?
Attachment #218477 -
Flags: review?
Assignee | ||
Comment 9•18 years ago
|
||
Per comment 7 suggestion(s), and email details from Neil.
Attachment #218478 -
Flags: review?(bryner)
Assignee | ||
Updated•18 years ago
|
Attachment #218477 -
Flags: superreview?(neil)
Attachment #218477 -
Flags: superreview?
Attachment #218477 -
Flags: review?(neil)
Attachment #218477 -
Flags: review?
Comment 10•18 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•18 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•18 years ago
|
||
yeah, moving outside of the hbox should be fine, assuming themes aren't making bad assumptions.
Comment 13•18 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•18 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•18 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•18 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•18 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•18 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•18 years ago
|
Attachment #221700 -
Flags: superreview?(neil)
Attachment #221700 -
Flags: superreview?
Attachment #221700 -
Flags: review?(neil)
Attachment #221700 -
Flags: review?
Comment 19•18 years ago
|
||
If I got my CSS right this test should also work in Firefox.
Updated•18 years ago
|
Attachment #221700 -
Flags: superreview?(neil)
Attachment #221700 -
Flags: superreview+
Attachment #221700 -
Flags: review?(neil)
Attachment #221700 -
Flags: review+
Assignee | ||
Comment 20•18 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•18 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•18 years ago
|
Attachment #221700 -
Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
Assignee | ||
Comment 22•18 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•18 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•18 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•18 years ago
|
Attachment #221763 -
Attachment description: Test → Test (for the |flex| issue)
Assignee | ||
Comment 24•18 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•18 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•18 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•18 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•18 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•18 years ago
|
Keywords: helpwanted → testcase
Assignee | ||
Comment 29•18 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•18 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•18 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•18 years ago
|
Attachment #221775 -
Flags: approval-branch-1.8.1?(bryner)
![]() |
||
Comment 32•17 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•17 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•16 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•16 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•16 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•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•16 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•16 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: 16 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•16 years ago
|
Attachment #221700 -
Attachment is obsolete: false
Assignee | ||
Updated•16 years ago
|
Attachment #221775 -
Attachment description: (Bv1a-TK) <button.xml> → (Bv1a-TK) <button.xml>
[Checkin: Comment 37]
Assignee | ||
Comment 38•16 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•16 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•16 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•16 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•16 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
•