428509 - Cannot attach JavaScript listeners to XForms events

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[John L. Clark]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13) Gecko/20080325 Ubuntu/7.10 (gutsy) Firefox/2.0.0.13
Build Identifier: 0.8.5

JavaScript event listeners that listen for XForms events do not fire when those events are dispatched.  This is a regression, as this functionality used to work in 0.8.4.  My wild guess is that this is related to the context information work that went into the latest release.  I will attach a test form shortly.

A workaround is to listen for the event using an XForms event listener, then to use `xf:dispatch` to dispatch a custom event, and finally to listen for that custom event with the desired JavaScript event listener.

Reproducible: Always

Comment 1

[John L. Clark]

Attachment: Simple test case that demonstrates this bug

Comment 2

[Merle Sterling]

The problem appears to be the QI from nsIXFormsDOMEvent to nsIDOMEvent in nsEventListenerThisTranslator::TranslateThis.

nsCOMPtr<nsIDOMEvent> event(do_QueryInterface(aInitialThis));
NS_ENSURE_TRUE(event, NS_ERROR_UNEXPECTED);

nsXFormsDOMEvent wraps nsDOMEvent (named mInner) because we cannot inherit from nsDOMEvent. We probably need to override QueryInterface and return the ISupports* of mInner but that is easier said than done with the decl and impl macros in nsISupportsImpl.h.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

One doesn't have to use those macros.

Comment 4

[Merle Sterling]

(In reply to comment #3)
> One doesn't have to use those macros.

True but the idl compiler will generate ns_decl_isupports at a minimum. I was using ns_impl_isupports3 for QI of nsIXFormsDOMEvent, nsIDOMNSEvent, and nsIPrivateDOMEvent but haven't yet figured out how to get nsIDOMEvent. The nsIXFormsDOMEvent interface inherits from nsIDOMEvent. Any ideas?

Comment 5

[Merle Sterling]

Attachment: patch

We have to get nsIDOMEvent added to the interface map so it can be QI'ed from nsIXFormsDOMEvent but also need to implement nsIClassInfo.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Did you try using the nsIClassInfo implementation of nsDOMEvent?
So implementing QI something like:
NS_INTERFACE_MAP_BEGIN(nsXFormsDOMEvent)
  NS_INTERFACE_MAP_ENTRY(nsIXFormsDOMEvent)
  NS_INTERFACE_MAP_ENTRY(nsIDOMEvent)
  NS_INTERFACE_MAP_ENTRY(nsIDOMNSEvent)
  NS_INTERFACE_MAP_ENTRY(nsIPrivateDOMEvent)
  NS_INTERFACE_MAP_ENTRY(nsISupports)
NS_INTERFACE_MAP_END_AGGREGATED(mInner)

Comment 7

[Merle Sterling]

That will work too if you add NS_INTERFACE_MAP_ENTRY(nsIClassInfo). Then you need to implement AddRef and Release using NS_IMPL_ADDREF(nsXFormsDOMEvent) and NS_IMPL_RELEASE(nsXFormsDOMEvent).

NS_IMPL_ISUPPORTS5 is all of that in one macro so I used it instead.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I meant that with NS_IMPL_ADDREF and NS_IMPL_RELEASE, but without NS_INTERFACE_MAP_ENTRY(nsIClassInfo) and without the actual classinfo implementation, since mInner can be QI'ed to classinfo.

Comment 9

[Merle Sterling]

I did it this way, which is very similar to what you suggest, but it did not work without adding nsIClassInfo because the javascript code throws an exception with a message like 'Permision denied to get property UnnamedClass.type'.
 
NS_INTERFACE_MAP_BEGIN(nsXFormsDOMEvent)
  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIXFormsDOMEvent)
  NS_INTERFACE_MAP_ENTRY(nsIXFormsDOMEvent)
  NS_INTERFACE_MAP_ENTRY(nsIDOMNSEvent)
  NS_INTERFACE_MAP_ENTRY(nsIPrivateDOMEvent)
  NS_INTERFACE_MAP_ENTRY(nsIDOMEvent)
  NS_INTERFACE_MAP_ENTRY(nsIClassInfo)
NS_INTERFACE_MAP_END

NS_IMPL_ADDREF(nsXFormsDOMEvent)
NS_IMPL_RELEASE(nsXFormsDOMEvent)

I can try your way with only NS_INTERFACE_MAP_ENTRY(nsISupports).

Comment 10

[Merle Sterling]

Ah, just noticed the NS_INTERFACE_MAP_END_AGGREGATED(mInner).

Comment 11

[Merle Sterling]

It works if you use NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIXFormsDOMEvent)

I'll make a new patch using that interface map and we can do away with the nsIClassInfo implementation.

Comment 12

[Merle Sterling]

Attachment: patch2

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 316284 [details] [diff] [review]
patch2

>+#include "nsXFormsUtils.h"
>+#include "nsIProgrammingLanguage.h"
I guess these aren't needed.

> NS_METHOD
> nsXFormsDOMEvent::SetTrusted(PRBool aTrusted)
> {
>   nsCOMPtr<nsIPrivateDOMEvent> privEvent = do_QueryInterface(mInner);
>   return privEvent->SetTrusted(aTrusted);
> }
>+
Extra newline

>+#include "nsIClassInfo.h"
Is this needed?

Those addressed, r=me

Comment 14

[Merle Sterling]

Attachment: patch3

Removing unnecessary includes and extra newlines.

Comment 15

[aaronr]

Comment on attachment 316294 [details] [diff] [review]
patch3

>Index: nsXFormsDOMEvent.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsDOMEvent.cpp,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 nsXFormsDOMEvent.cpp
>--- nsXFormsDOMEvent.cpp	10 Jan 2008 23:34:07 -0000	1.1
>+++ nsXFormsDOMEvent.cpp	17 Apr 2008 22:02:40 -0000
>@@ -38,17 +38,26 @@
> 
> #include "nsXFormsDOMEvent.h"
> 
> /**
>  * Implementation for XForms events.
>  *
>  */
> 
>-NS_IMPL_ISUPPORTS3(nsXFormsDOMEvent, nsIXFormsDOMEvent, nsIDOMNSEvent, nsIPrivateDOMEvent)
>+NS_INTERFACE_MAP_BEGIN(nsXFormsDOMEvent)
>+  NS_INTERFACE_MAP_ENTRY(nsIXFormsDOMEvent)
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMEvent)
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMNSEvent)
>+  NS_INTERFACE_MAP_ENTRY(nsIPrivateDOMEvent)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIXFormsDOMEvent)
>+NS_INTERFACE_MAP_END_AGGREGATED(mInner)
>+

nit: Please add comments here explaining why all of this is needed.  Especially the ambiguous and aggregated parts since they aren't commonly found in our code.  If someone decided that we didn't need these for some reason we wouldn't notice a problem until someone tried JS event handlers which isn't in our normal test bucket.  But if we have comments here then it won't likely be touched unless there is a good reason.

Otherwise looks good to me. r+ with that change.

Comment 16

[Merle Sterling]

Attachment: patch4

Adding comments to explain the interface map.

Comment 17

[aaronr]

checked into trunk for msterlin

Comment 18

[aaronr]

checked into 1.8 branch for msterlin