Closed Bug 615152 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b10

People

(Reporter: erikvvold, Assigned: adw)

Details

Attachments

(1 file, 1 obsolete file)

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";
  }
})
using minefield
source: https://github.com/erikvold/delicious-widget-ffext @ 8c8fc085b090ce34a6ea67db508d68a09f12fccd
(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.
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?
Attached patch patch (obsolete) — Splinter Review
Attachment #500943 - Flags: review?(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.
Attachment #500943 - Flags: review?(dietrich) → review?(dtownsend)
Drew, can you update the summary, product, component to match the problem?
OS: Mac OS X → All
Product: Add-on SDK → Toolkit
QA Contact: general → general
Hardware: x86 → All
Summary: Error when clicking on icon widget → WindowDraggingElement should not assume that event targets are XUL elements
Component: General → XUL Widgets
QA Contact: general → xul.widgets
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.
Attachment #500943 - Flags: review?(dtownsend) → review-
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 #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.
... 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.
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.
(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.
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?
A side effect of the window using it.
Then the fix here would be to have shouldDrag return false early if the originalTarget isn't in the same window as this._window
Attached patch patch 2Splinter Review
Attachment #500943 - Attachment is obsolete: true
Attachment #501843 - Flags: review?(enndeakin)
Attachment #501843 - Flags: review?(enndeakin) → review+
Attachment #501843 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/a6a154157240
Assignee: nobody → adw
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Summary: WindowDraggingElement should not assume that event targets are XUL elements → WindowDraggingElement should not assume that event targets are in the same window as its element
You need to log in before you can comment on or make changes to this bug.