Core XBL widgets used in web pages can't listen to their own events

RESOLVED FIXED

Status

()

Core
XBL
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: smaug)

Tracking

({regression})

Trunk
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

12 years ago
If an XBL widget is defined in content then its <handler> elements only listen to trusted events, even when the widget is bound to a content element, where it can't actually dispatch a trusted event. Bug 350257 was caused because the binding was only tested in content so nobody noticed that it was trying to listen to the same event twice. IMHO for XBL1 handlers bound to content should listen to untrusted events by default. For XBL2 I guess that core widgets could dispatch trusted events so that the problem does not apply.
Are 350332 and 350335 also caused by this? Bug 350335 is definitely a regression from something.
(Assignee)

Comment 2

12 years ago
Created attachment 235593 [details] [diff] [review]
something like this

Didn't test this yet
(Reporter)

Comment 3

12 years ago
Created attachment 235599 [details]
simple number box demo

Until bug 350257 is fixed, fixing this bug will make this fail in content too.
Nickolay, the editable tree bugs you've filed recently are all caused by some event handling regression, which may be this one. I know for sure that the issues in the bugs I saw were working when I checked in the tree patch.

Comment 5

12 years ago
No, they don't appear to be caused by this bug. All that applying this patch does is break editable trees in content in the same way they are broken in chrome right now (because text selection event initiated in selectText in startEditing can now be - wrongly! - caught by http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tree.xml&rev=1.37&mark=408#400). It appears that phase="target" doesn't filter well enough or something.
smaug, did you want someone to review this?
(Assignee)

Comment 7

11 years ago
Comment on attachment 235593 [details] [diff] [review]
something like this

The patch seems to apply cleanly to trunk (just some fuzz)
Attachment #235593 - Flags: review?(jst)
Comment on attachment 235593 [details] [diff] [review]
something like this

- In content/xbl/src/nsXBLPrototypeHandler.h

-#define NS_HANDLER_TYPE_XBL_JS          (1 << 0)
-#define NS_HANDLER_TYPE_XBL_COMMAND     (1 << 1)
-#define NS_HANDLER_TYPE_XUL             (1 << 2)
-#define NS_HANDLER_ALLOW_UNTRUSTED      (1 << 5)
-#define NS_HANDLER_TYPE_SYSTEM          (1 << 6)
-#define NS_HANDLER_TYPE_PREVENTDEFAULT  (1 << 7)
+#define NS_HANDLER_TYPE_XBL_JS              (1 << 0)
+#define NS_HANDLER_TYPE_XBL_COMMAND         (1 << 1)
+#define NS_HANDLER_TYPE_XUL                 (1 << 2)
+#define NS_HANDLER_HAS_ALLOW_UNTRUSTED_ATTR (1 << 4)
+#define NS_HANDLER_ALLOW_UNTRUSTED          (1 << 5)
+#define NS_HANDLER_TYPE_SYSTEM              (1 << 6)
+#define NS_HANDLER_TYPE_PREVENTDEFAULT      (1 << 7)

This looks suspicious, aren't these values all flags? If so, there's some bit overlap there, i.e. with those valuyes flags & NS_HANDLER_HAS_ALLOW_UNTRUSTED_ATTR will test true for the last 4 of those values etc.

r- until that's clarified.
Attachment #235593 - Flags: review?(jst) → review-
(Assignee)

Comment 9

11 years ago
Comment on attachment 235593 [details] [diff] [review]
something like this

Sorry, but can you clarify what actually overlaps?
1 << X shouldn't
overlap with 1 << Y
when X != Y
Attachment #235593 - Flags: review- → review?(jst)
Comment on attachment 235593 [details] [diff] [review]
something like this

Duh, I got blinded by the 1, 2, 4, ..., the lack of the 3 there, and thought it was a direct filed value, w/o seeing the << right there in my face. Any idea why the 3 is missing there?

@@ -922,16 +918,17 @@ nsXBLPrototypeHandler::ConstructPrototyp
   if (aGroup && nsDependentString(aGroup).EqualsLiteral("system"))
     mType |= NS_HANDLER_TYPE_SYSTEM;
 
   nsAutoString preventDefault(aPreventDefault);
   if (preventDefault.EqualsLiteral("true"))

Want to replace that nsAutoString with a nsDependentString while you're at it? :)

r=jst
Attachment #235593 - Flags: review?(jst) → review+
(Assignee)

Comment 11

11 years ago
Comment on attachment 235593 [details] [diff] [review]
something like this

Will change that nsAutoString to nsDependentString.
Attachment #235593 - Flags: superreview?(jonas)
(Reporter)

Comment 12

11 years ago
(In reply to comment #10)
>(From update of attachment 235593 [details] [diff] [review])
>Any idea why the 3 is missing there?
Two sets of flag bits working from each end of the byte.

>>nsAutoString preventDefault(aPreventDefault);
>>if (preventDefault.EqualsLiteral("true"))
>Want to replace that nsAutoString with a nsDependentString while you're at it?
Note that it's possible for aPreventDefault to be null.
(In reply to comment #12)
> >>nsAutoString preventDefault(aPreventDefault);
> >>if (preventDefault.EqualsLiteral("true"))
> >Want to replace that nsAutoString with a nsDependentString while you're at it?
> Note that it's possible for aPreventDefault to be null.

In that case we'll need to use a Substring here, not a nsDependentString.
Attachment #235593 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 14

11 years ago
Created attachment 261340 [details]
a testcase
Assignee: general → Olli.Pettay
Status: NEW → ASSIGNED
(Assignee)

Comment 15

11 years ago
Created attachment 261341 [details] [diff] [review]
fix also keyevents

Sorry, I didn't notice that key events should be fixed too.
The change to the previous patch is in nsXBLKeyEventHandler::HandleEvent
and  having SetIsBoundToChrome().
Attachment #261341 - Flags: superreview?(jst)
Attachment #261341 - Flags: review?(jst)
Comment on attachment 261341 [details] [diff] [review]
fix also keyevents

- In nsXBLKeyEventHandler::HandleEvent():

+  nsCOMPtr<nsIContent> content = do_QueryInterface(target);
+  NS_ENSURE_STATE(content);

The point of this QI is just to make sure that target is an nsIContent right? If so, please add a comment to explain that, as "content" is not used anywhere in this function AFAICT.

r+sr=jst with that.
Attachment #261341 - Flags: superreview?(jst)
Attachment #261341 - Flags: superreview+
Attachment #261341 - Flags: review?(jst)
Attachment #261341 - Flags: review+
(Assignee)

Comment 17

11 years ago
(In reply to comment #16)
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(target);
> +  NS_ENSURE_STATE(content);
> 

Oh, that is a leftover from a previous version of the patch.
I'll just remove it.
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 18

11 years ago
needs automation
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.