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 comment:1
| owner: | → plumpnation |
|---|---|
| status: | new → pending |
Changed July 19, 2013 08:14AM UTC by comment:2
| status: | pending → new |
|---|
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 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 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 comment:5
| milestone: | none → 1.11.0 |
|---|---|
| status: | new → open |
| 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 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.
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.
Do you plan on sending a pull request? We've documented how to contribute code to jQuery projects.