567522 - onMouseover and onMouseOut called twice with different event targets (jetpack 0.4 rc1 widget)

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[David Illsley]

With the new Widget API in 0.4 RC1, when I register an onMouseover/onMouseout I get called back twice for each with different event targets.

STR
const widgets = require("widget");
widgets.add(widgets.Widget({
        label: "Widget with changing image on mouseover",
        content: "<img src=http://www.yahoo.com/favicon.ico>",
        onMouseover: function(e) {
			console.log("over "+e.target);
            e.target.src = "http://www.bing.com/favicon.ico";
        },
        onMouseout: function(e) {
		    console.log("out "+e.target)
            e.target.src = "http://www.yahoo.com/favicon.ico";
        }
    }));

Actual Results:
info: over [object XULElement]
info: over [object XPCNativeWrapper [object HTMLImageElement]]
info: out [object XPCNativeWrapper [object HTMLImageElement]]
info: out [object XULElement]

Expected results:
A single call to over/out as implied by the docs.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: v1

- only fire events for the content document, not for the xul iframe
- update tests to ensure only one call to the listener per event fired

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 447119 [details] [diff] [review]
v1

>diff --git a/packages/jetpack-core/lib/widget.js b/packages/jetpack-core/lib/widget.js

>     let listener = function(e) {
>+      if (e.target.tagName && e.target.tagName == "iframe" &&
>+          e.target.toString() == "[object XULElement]")
>+        return;

Nit: this'll work, but it'd be better to do |e.target == item.node.firstElementChild|, which is both simpler and more robust.


>       // URL-specific handling
>       if (e.type == "load" &&
>           contentType == CONTENT_TYPE_URI || contentType == CONTENT_TYPE_IMAGE) {
>         if (!loadURIIsMatch(e.target.location, item.widget))
>           return;
>       }

While you're making changes to this area of the code, can you fix a bug in this conditional?  It evaluates to true when contentType == CONTENT_TYPE_IMAGE, even when e.type != "load", because && has higher precedence than ||, leading to exceptions because loadURIIsMatch gets called with a null e.target.location when mousing over/out of an image:

Error: uncaught exception: [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: file:///mnt/hgfs/myk/Projects/jetpack-sdk-widget/packages/jetpack-core/lib/securable-module.js -> resource://widgets-jetpack-core-lib/widget.js :: loadURIIsMatch :: line 428"  data: no]


>diff --git a/packages/jetpack-core/tests/test-widget.js b/packages/jetpack-core/tests/test-widget.js

>-      content: "<div id='me'>foo</div>",
>+      content: "0",
>       onReady: function(e) {
>-        if (e.target.defaultView)
>-          sendMouseEvent({type:"click"}, "me", e.target.defaultView);
>+        // ignore subsequent loads
>+        if (this.content > 0)
>+          return;
>+        // send click once, and later check that it only fired once.
>+        sendMouseEvent({type:"click"}, null, e.target.defaultView);
>+        let self = this;
>+        require("timer").setTimeout(function() {
>+          test.assertEqual(self.content, 1, "onClick only called once per event fired");
>+          widgets.remove(self);
>+          doneTest();
>+        }, 100);

These tests don't seem to test the behavior in question, as they pass even if I remove the change to widget.js.  I suspect "content" has to contain something like the <img> tag in comment 0.  However, when I make it something like "<img src=http://www.yahoo.com/favicon.ico>", I get a bunch of exceptions thrown to the console, perhaps because "content" is no longer something that can be incremented like a number.

However, using a variable to track the call count like in the following example doesn't fail the tests either, so I'm not sure what's going on:

    tests.push(function() { let count = 0; testSingleWidget(widgets.Widget({
      label: "mouseover test widget",
      content: "<img src=http://www.yahoo.com/favicon.ico>",
      onReady: function(e) {
        // ignore subsequent loads
        if (count > 0)
          return;
        // send mouseover once, and later check that it only fired once.
        sendMouseEvent({type:"mouseover"}, null, e.target.defaultView);
        let self = this;
        require("timer").setTimeout(function() {
          test.assertEqual(count, 1, "onMouseover only called once per event fired");
          widgets.remove(self);
          doneTest();
        }, 100);
      },
      onMouseover: function(e) {
        test.pass("onMouseover called");
        count++;
      }
    }))});

Comment 3

[Dietrich Ayala (:dietrich)]

> While you're making changes to this area of the code, can you fix a bug in this
> conditional?  It evaluates to true when contentType == CONTENT_TYPE_IMAGE, even

should be fixed from the check-in for bug 567505.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> should be fixed from the check-in for bug 567505.

Ah, yes, of course!

Comment 5

[Dietrich Ayala (:dietrich)]

This was driving me crazy, because I can reproduce the original bug in a separate addon via cfx run, but not in the unit tests.

Current theory is that while moving the mouse to the widget, it passes over the iframe, while in the test i trigger the event manually, directly on the target element (no pass-over!).

I confirmed manually that this patch fixes the problem (and with Myk's change also), so going to not include the hacky unit test bits.

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: v2

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 447278 [details] [diff] [review]
v2

Ah, Mozilla, so much fun to debug. r=myk

Comment 8

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/labs/jetpack-sdk/rev/201e60065b1a

Comment 9

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".