XBL widgets should not set global variables for event handlers

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
16 years ago
6 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: jag (Peter Annema))

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 1

16 years ago
Created attachment 90609 [details] [diff] [review]
One approach

In this example I move the __ButtonMenuMouseDown__ property from the document
to the JS object created by the field.
(Reporter)

Comment 2

16 years ago
Created attachment 90611 [details] [diff] [review]
Another approach

In this approach I create the JS object in the _captureMouseUp method.
(Assignee)

Comment 3

15 years ago
Sounds like something we should fix. Any takers?
Keywords: helpwanted
(Reporter)

Comment 4

15 years ago
Created attachment 130122 [details] [diff] [review]
Much better way
[Checkin: Comment 7]

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
(Reporter)

Updated

15 years ago
Attachment #130122 - Flags: review?(varga)

Comment 5

12 years ago
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+
(Reporter)

Updated

12 years ago
Attachment #130122 - Flags: superreview?(jag)
(Assignee)

Comment 6

12 years ago
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+
(Reporter)

Comment 7

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED
Blocks: 282180
Created attachment 287691 [details] [diff] [review]
(Cv1-TK) Xpfe to Toolkit port [Checkin: Comment 10]

(See bug 282180 comment 39.)
Attachment #287691 - Flags: review?
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]

Updated

11 years ago
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]
You need to log in before you can comment on or make changes to this bug.