Dialog: Context is not respected for modals

[plumpnation]

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.

[Scott González]

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.

[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.

[tj.vantoll]

Replying to 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.

[plumpnation]

Replying to tj.vantoll:

Replying to 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

[Scott González]

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.

[plumpnation]

Replying to 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.

[Scott González]

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

Changeset: c9815f13b487d027ef9b6095588dbb73141c9a09