Skip to main content

Search and Top Navigation

#9439 closed bug (fixed)

Opened July 18, 2013 06:54PM UTC

Closed August 07, 2013 12:59PM UTC

Dialog: Context is not respected for modals

Reported by: plumpnation Owned by: plumpnation
Priority: minor Milestone: 1.11.0
Component: ui.dialog Version: 1.10.3
Keywords: Cc:
Blocked by: Blocking:
Description

Example

I created this fiddle (http://jsfiddle.net/9Qzza/16/) but jsfiddle struggles and the dialog is almost unusable in Firefox Aurora, Chrome(Version 28.0.1500.71) and Chromium.

To replicate

The issue crops up when a modal draggable dialog is appended to an iframe body, thus using jQuery from a different window. The console error only pops up in firefox. There are other issues in Chrome and Chromium, however that is a different matter.

The dialog settings need to include to replicate:

{
    modal: true,
    draggable: true,
    appendTo: myIframe.contentWindow.document.body
}

When you try to drag the dialog box, the error pops up in the console.

The code

There is a selection in the _createOverlay function that takes no care to ensure scope safety or to make a relative DOM query.

It looks like this

$(".ui-dialog:visible:last .ui-dialog-content")

Since $ is not in the same scope as the dialog, the context needs to be passed into the query. Like this

var elementContext = that.element[0].ownerDocument;
$(".ui-dialog:visible:last .ui-dialog-content", elementContext)

The error in the firefox console is:

TypeError: $(...).data(...) is null

I have created a patch in my jquery ui fork.

Attachments (0)
Change History (7)

Changed July 18, 2013 07:10PM UTC by scottgonzalez comment:1

owner: → plumpnation
status: newpending

I don't think we want to support this. However, this would be supported: http://jsfiddle.net/9Qzza/17/

I can land a fix for that, but I'm worried about the potential for lots of bugs if we start allowing the creation of a widget in one window and then the dynamic use of it in another.

I have created a patch in my jquery ui fork.

Do you plan on sending a pull request? We've documented how to contribute code to jQuery projects.

Changed July 19, 2013 08:14AM UTC by plumpnation comment:2

status: pendingnew

I checked out your fiddle and it does indeed solve this individual problem, (although not in jsfiddle :D ).

I can understand your point of view regarding the potential for lots of bugs, and see that this could become something larger in terms of support, however it is my opinion (nothing more) that a ui widget should operate in a relative-to-itself way. In fact, in the large majority of the ui library, DOM traversing IS done in relation to the widget element itself, (this.element.find() etc).

Because the particular query I want to edit is making such heavy use of the widget state via pseudo selectors I thought that the least-likely-to-break-tests way to 'fix' it was to offer the ownerDocument as context.

Changed July 19, 2013 10:39AM UTC by tj.vantoll comment:3

Replying to [comment:2 plumpnation]:

I checked out your fiddle and it does indeed solve this individual problem, (although not in jsfiddle :D ). I can understand your point of view regarding the potential for lots of bugs, and see that this could become something larger in terms of support, however it is my opinion (nothing more) that a ui widget should operate in a relative-to-itself way. In fact, in the large majority of the ui library, DOM traversing IS done in relation to the widget element itself, (this.element.find() etc). Because the particular query I want to edit is making such heavy use of the widget state via pseudo selectors I thought that the least-likely-to-break-tests way to 'fix' it was to offer the ownerDocument as context.

If we allow it for one widget we need to allow it for all for consistency. There are some widgets that require document level operations. For example menu and a number of other widgets need to listen for clicks on the document to close when they are open. Those sort of things are where this becomes really messy.

Changed July 19, 2013 12:05PM UTC by plumpnation comment:4

Replying to [comment:3 tj.vantoll]:

Replying to [comment:2 plumpnation]: > I checked out your fiddle and it does indeed solve this individual problem, (although not in jsfiddle :D ). > > I can understand your point of view regarding the potential for lots of bugs, and see that this could become something larger in terms of support, however it is my opinion (nothing more) that a ui widget should operate in a relative-to-itself way. In fact, in the large majority of the ui library, DOM traversing IS done in relation to the widget element itself, (this.element.find() etc). > > Because the particular query I want to edit is making such heavy use of the widget state via pseudo selectors I thought that the least-likely-to-break-tests way to 'fix' it was to offer the ownerDocument as context. If we allow it for one widget we need to allow it for all for consistency. There are some widgets that require document level operations. For example menu and a number of other widgets need to listen for clicks on the document to close when they are open. Those sort of things are where this becomes really messy.

Surely the document level operations should be on the ownerDocument of the widget though?

Since jQuery supports document context anyway, the required 'fixes' seem quite low risk to me, as patches would not break usage in single window applications, but would allow more scope flexibility.

Also, supporting this usage for all widgets doesn't have to mean that all the widgets would need to be updated overnight. We are using the dialog across window scopes right now, and that gave us only one issue, which I was trying to fix here. The workaround supplied by scott solved our immediate issue, however my fix works with the current dialog option:

'appendTo': element

Changed July 19, 2013 12:31PM UTC by scottgonzalez comment:5

milestone: none1.11.0
status: newopen
summary: Dragging a modal dialog appended to an iframe body causes console error as ownerDocument is not respected.Dialog: Context is not respected for modals
Surely the document level operations should be on the ownerDocument of the widget though?

Yes, this is the fix that I am willing to land.

Since jQuery supports document context anyway, the required 'fixes' seem quite low risk to me, as patches would not break usage in single window applications, but would allow more scope flexibility.

Except that there's a very big difference between respecting the context of the widget, which is defined at initialization time, and officially supporting dynamic context changes. We're trying hard to make all widgets support the former, but supporting the latter seems very much like an edge case.

Changed July 21, 2013 10:01AM UTC by plumpnation comment:6

Replying to [comment:5 scott.gonzalez]:

> Surely the document level operations should be on the ownerDocument of the widget though? Yes, this is the fix that I am willing to land. > Since jQuery supports document context anyway, the required 'fixes' seem quite low risk to me, as patches would not break usage in single window applications, but would allow more scope flexibility. Except that there's a very big difference between respecting the context of the widget, which is defined at initialization time, and officially supporting dynamic context changes. We're trying hard to make all widgets support the former, but supporting the latter seems very much like an edge case.

I agree with you. Thanks for your time.

Changed August 07, 2013 12:59PM UTC by Scott González comment:7

resolution: → fixed
status: openclosed

Dialog: Search the correct document for focus trapping. Fixes #9439 - Dialog: Context is not respected for modals.

Changeset: c9815f13b487d027ef9b6095588dbb73141c9a09