sync xpfe button.xml with toolkit button.xml

RESOLVED FIXED in mozilla1.9beta2

Status

()

Core
XUL
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: mconnor, Assigned: sgautherie)

Tracking

({fixed-seamonkey1.1a, fixed1.8.1, testcase})

Trunk
mozilla1.9beta2
fixed-seamonkey1.1a, fixed1.8.1, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

13 years ago
 
(Reporter)

Updated

13 years ago
Blocks: 282177
(Assignee)

Comment 2

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

Diff from Xpfe to Toolkit version.

Comment 3

12 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

12 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

12 years ago
Created attachment 217747 [details] [diff] [review]
(Av2) <button.xml>
Assignee: nobody → gautheri
Attachment #176648 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #176648 - Attachment description: 2005.03.08 X2T diff report → (Av1) 2005.03.08 X2T diff report
(Assignee)

Comment 6

12 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

12 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

12 years ago
Attachment #217747 - Attachment description: (Av2) <button.xml> → (Av2) <button.xml> (TK + XPFE)
Attachment #217747 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #217747 - Attachment description: (Av2) <button.xml> (TK + XPFE) → (Av2) <button.xml>
(Assignee)

Comment 8

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

Av2, with comment 7 suggestion(s).
Attachment #218477 - Flags: superreview?
Attachment #218477 - Flags: review?
(Assignee)

Comment 9

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

Per comment 7 suggestion(s), and email details from Neil.
Attachment #218478 - Flags: review?(bryner)
(Assignee)

Updated

12 years ago
Attachment #218477 - Flags: superreview?(neil)
Attachment #218477 - Flags: superreview?
Attachment #218477 - Flags: review?(neil)
Attachment #218477 - Flags: review?

Comment 10

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

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

Comment 13

12 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

12 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

12 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

12 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

12 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

12 years ago
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 ?
Attachment #218477 - Attachment is obsolete: true
Attachment #221700 - Flags: superreview?
Attachment #221700 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #221700 - Flags: superreview?(neil)
Attachment #221700 - Flags: superreview?
Attachment #221700 - Flags: review?(neil)
Attachment #221700 - Flags: review?

Comment 19

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

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

Updated

12 years ago
Attachment #221700 - Flags: superreview?(neil)
Attachment #221700 - Flags: superreview+
Attachment #221700 - Flags: review?(neil)
Attachment #221700 - Flags: review+
(Assignee)

Comment 20

12 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

12 years ago
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.
Attachment #221775 - Flags: review?(mconnor)

Updated

12 years ago
Attachment #221700 - Flags: approval-branch-1.8.1?(neil) → approval-branch-1.8.1+
(Assignee)

Comment 22

12 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

12 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

12 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

12 years ago
Attachment #221763 - Attachment description: Test → Test (for the |flex| issue)
(Assignee)

Comment 24

12 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.
Depends on: 230854, 259724
(Assignee)

Comment 25

12 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

12 years ago
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.)
(Assignee)

Comment 27

12 years ago
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.
(Assignee)

Comment 28

12 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

12 years ago
Keywords: helpwanted → testcase
(Assignee)

Comment 29

12 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

12 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

12 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

12 years ago
Attachment #221775 - Flags: approval-branch-1.8.1?(bryner)

Comment 32

11 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

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

11 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 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+
Keywords: checkin-needed
(Assignee)

Updated

10 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]
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
Last Resolved: 10 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

10 years ago
Attachment #221700 - Attachment is obsolete: false
(Assignee)

Updated

10 years ago
Attachment #221775 - Attachment description: (Bv1a-TK) <button.xml> → (Bv1a-TK) <button.xml> [Checkin: Comment 37]
(Assignee)

Comment 38

10 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...
Depends on: 156459, 337532
Target Milestone: mozilla1.9alpha1 → mozilla1.9 M10

Comment 39

10 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

10 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

10 years ago
(In reply to comment #40)
Could you file a dependent bug, with steps to reproduce and other relevant details ? Thanks.
Depends on: 403162
(Assignee)

Comment 42

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