Closed Bug 156459 Opened 22 years ago Closed 18 years ago

XBL widgets should not set global variables for event handlers

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: neil, Assigned: jag+mozilla)

References

Details

Attachments

(2 files, 2 obsolete files)

Much better way [Checkin: Comment 7]
9.19 KB, patch
janv
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review
(Cv1-TK) Xpfe to Toolkit port [Checkin: Comment 10]
9.30 KB, patch
asaf
: review+
mtschrep
: approval1.9+
Details | Diff | Splinter Review 
Currently many XBL widgets add methods event handlers to a global object such as
window or document. Of course since the method executes in the context of the
global object there's no obvious way to obtain the original widget. These
handlers typically use some obscure global symbol as a temporary variable.
Instead of using a method as a handler, they should use a JS object with the
handleEvent method. This method can then refer to other properties of the JS object.
Attached patch One approach (obsolete) — — Splinter Review 
In this example I move the __ButtonMenuMouseDown__ property from the document
to the JS object created by the field.
Attached patch Another approach (obsolete) — — Splinter Review 
In this approach I create the JS object in the _captureMouseUp method.
Sounds like something we should fix. Any takers?
Keywords: helpwanted
If I make the element itself an nsIDOMEventListener it can just listen on the
global event instead...

This patch demonstrates using button.xml, colorpicker.xml and tabbox.xml
Attachment #90609 - Attachment is obsolete: true
Attachment #90611 - Attachment is obsolete: true
Attachment #130122 - Flags: review?(varga)
Comment on attachment 130122 [details] [diff] [review]
Much better way
[Checkin: Comment 7]

looks good to me

man, it took almost 3 years to get to it :)
Attachment #130122 - Flags: review?(Jan.Varga) → review+
Attachment #130122 - Flags: superreview?(jag)
Comment on attachment 130122 [details] [diff] [review]
Much better way
[Checkin: Comment 7]

If this doesn't apply trivially, new patch please :-)
Attachment #130122 - Flags: superreview?(jag) → superreview+
Fix checked in.

FYI, the trivial merge conflict was here:
>@@ -110,39 +110,39 @@
>         </setter>
>       </property>
> 
>-      <field name="_keyEventHandler" readonly="true">
>-      <![CDATA[({
>-        tabbox: this,
>-        handleEvent: function handleEvent(event) {
>+      <method name="handleEvent">
>+        <parameter name="event"/>
>+        <body>
>+        <![CDATA[
>           switch (event.keyCode) {
>             case event.DOM_VK_TAB:
>               if (event.ctrlKey && !event.altKey && !event.metaKey)
7 extra lines had been added before the switch in the meantime.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attachment #287691 - Flags: review? → review?(mano)
Keywords: helpwanted
OS: Windows 95 → All
Hardware: PC → All
Comment on attachment 287691 [details] [diff] [review]
(Cv1-TK) Xpfe to Toolkit port [Checkin: Comment 10]

r=mano
Attachment #287691 - Flags: review?(mano) → review+
Attachment #287691 - Flags: approval1.9?
Attachment #130122 - Attachment description: Much better way → Much better way [Checkin: Comment 7]
Attachment #287691 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Whiteboard: [c-n: Cv1-TK]
Checking in toolkit/content/widgets/button.xml;
/cvsroot/mozilla/toolkit/content/widgets/button.xml,v  <--  button.xml
new revision: 1.21; previous revision: 1.20
done
Checking in toolkit/content/widgets/colorpicker.xml;
/cvsroot/mozilla/toolkit/content/widgets/colorpicker.xml,v  <--  colorpicker.xml
new revision: 1.20; previous revision: 1.19
done
Checking in toolkit/content/widgets/tabbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbox.xml,v  <--  tabbox.xml
new revision: 1.50; previous revision: 1.49
done
Keywords: checkin-needed
Whiteboard: [c-n: Cv1-TK]
Attachment #287691 - Attachment description: (Cv1-TK) Xpfe to Toolkit port → (Cv1-TK) Xpfe to Toolkit port [Checkin: Comment 10]
Depends on: 404184
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: