Closed Bug 350334 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: XBL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: smaug)

References

Details

(Keywords: regression)

Attachments

(4 files)

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.
Didn't test this yet
Attached file 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.
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?
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-
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+
Comment on attachment 235593 [details] [diff] [review]
something like this

Will change that nsAutoString to nsDependentString.
Attachment #235593 - Flags: superreview?(jonas)
(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+
Attached file a testcase
Assignee: general → Olli.Pettay
Status: NEW → ASSIGNED
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+
(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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
needs automation
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.