615152 - WindowDraggingElement should not assume that event targets are in the same window as its element

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Erik Vold]

I get the following error:

Error: parent.getAttribute is not a function
Source File: resource://gre/modules/WindowDraggingUtils.jsm
Line: 63

when I try this addon:


const widgets = require("widget");
const panels = require("panel");

let panel = panels.add(panels.Panel({
  contentURL: "http://delicious.com/save?url=",
  contentScript: "alert(document.location.href)",
  width: 900,
  height: 600
}))

widgets.Widget({
  label: "Delicious",
  contentURL: "http://www.delicious.com/favicon.ico",
  panel: panel,
  onClick: function() {
    panel.contentURL = "http://delicious.com/save?url=" + "http://google.com";
  }
})

Comment 1

[Erik Vold]

using minefield

Comment 2

[Erik Vold]

source: https://github.com/erikvold/delicious-widget-ffext @ 8c8fc085b090ce34a6ea67db508d68a09f12fccd

Comment 3

[Erik Vold]

(In reply to comment #0)
> I get the following error:
> widgets.Widget({
>   label: "Delicious",
>   contentURL: "http://www.delicious.com/favicon.ico",
>   panel: panel,
>   onClick: function() {
>     panel.contentURL = "http://delicious.com/save?url=" + "http://google.com";
>   }
> })

It looks to me like the issue is due to the existence of the onClick attribute on the Widget, when I comment that out the panel will open and the error does not occur.

Comment 4

[Drew Willcoxon :adw]

Thanks for filing, I see this often.

The problem is WindowDraggingElement.prototype.shouldDrag() assumes event targets and their ancestors will always be (XUL) DOM elements.  For a widget though, event targets are things in the widget's iframe.  In the test case in comment 0, the event target is an HTMLImageElement.  Its ancestor chain is HTMLBodyElement, HTMLHtmlElement, and HTMLDocument, which is where the error happens because documents are not elements and therefore don't have a getAttribute() method.

Dietrich, I don't see how we can fix this from the Jetpack side, so I think the only way is to modify WindowDraggingElement in toolkit.  What do you think?

Comment 5

[Drew Willcoxon :adw]

Attachment: patch

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 500943 [details] [diff] [review]
patch

Switching review to Dave, who might be the right person to review this. I'm not sure.

Comment 7

[Dietrich Ayala (:dietrich)]

Drew, can you update the summary, product, component to match the problem?

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 500943 [details] [diff] [review]
patch

(In reply to comment #4)
> Thanks for filing, I see this often.
> 
> The problem is WindowDraggingElement.prototype.shouldDrag() assumes event
> targets and their ancestors will always be (XUL) DOM elements

That doesn't seem to be quite right (well it kind of is but isn't the cause of the problem). WindowDraggingElement assumes that the event target is in the same document as the draggable element when in this case it is in a child document. The patch given walks up the element tree until it hits the top of the inner-document and then stops but I think it actually needs to keep walking up the elements in the outer-document until it reaches the draggable element.

So at the end of the loop if parent is a DOM Document then use the containing <browser> or <iframe> element instead I think.

Comment 9

[Drew Willcoxon :adw]

Thanks Dave.  But are we sure that's the behavior we want?  Widget iframes can have anything in them, like say a slider made from HTML.  If I drag that slider, I don't want the browser window to drag.  I think that's what your suggestion would imply.

Comment 10

[Drew Willcoxon :adw]

(comment #9)
> Thanks Dave.  But are we sure that's the behavior we want?  Widget iframes can
> have anything in them, like say a slider made from HTML.  If I drag that
> slider, I don't want the browser window to drag.  I think that's what your
> suggestion would imply.

Comment 11

[Drew Willcoxon :adw]

... unless the widget iframe's mousethrough="never".

So, is there an easy way to get at the iframe from the window or document inside it?  I know I can get at the chrome window containing the iframe via nsIDocShellTreeItem etc., but I don't know how to easily get the iframe.

Comment 12

[Dave Townsend [:mossop]]

You're right, though the inner frame part is a complication. An inner frame could have pure XUL elements, or HTML content could be embedded straight into the XUL.

So there are two bugs here I think. One is that it doesn't handle nested frames and the other is that it doesn't handle non-XUL content properly as you said before.

Copying Enn on this as he may have the best opinion but I think the following is true:

The first loop walks up the element tree checking the mousethrough attribute to ensure we know what the target element was. I think this loop ought to walk up all nested frames but since mousethrough is a XUL attribute it should only check that on XUL elements and skip over non-XUL elements.

The second loop walks up from the target element until it finds one that is in the list of elements that handle dragging themselves. I think this too should walk up all nested frames but it should assume any non-XUL element handled dragging itself.

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #11)
> ... unless the widget iframe's mousethrough="never".
> 
> So, is there an easy way to get at the iframe from the window or document
> inside it?  I know I can get at the chrome window containing the iframe via
> nsIDocShellTreeItem etc., but I don't know how to easily get the iframe.

I think window.frameElement will get you the iframe or browser element holding the window, I'm not totally sure whether that works when you're transitioning from a content to a chrome document though.

Comment 14

[Neil Deakin]

Are you using WindowDraggingUtils yourself or is this bug a side effect of the window using it? If the former, what do you initialize WindowDraggingUtils with, and what element structure is being used here that you need a child in a frame to drag the window?

Comment 15

[Drew Willcoxon :adw]

A side effect of the window using it.

Comment 16

[Neil Deakin]

Then the fix here would be to have shouldDrag return false early if the originalTarget isn't in the same window as this._window

Comment 17

[Drew Willcoxon :adw]

Attachment: patch 2

Comment 18

[Drew Willcoxon :adw]

http://hg.mozilla.org/mozilla-central/rev/a6a154157240