Opened 9 years ago

Closed 9 years ago

#11432 closed defect (wontfix)

[patch][cla]dijit.Editor - prevent onLoadDeferred from resolving multiple times

Reported by: ben hockey Owned by:
Priority: high Milestone: tbd
Component: Editor Version: 1.5.0b2
Keywords: Cc:
Blocked By: Blocking:

Description

if an existing instance of a dijit.Editor is removed from the document and then placed back in the document, it will cause the onload of the iframe to fire again. this will then cause the onLoadDeferred to be resolved again which will cause the following error: Error: This deferred has already been resolved

removing the editor from the document and placing it back in later happens when using dtl and also happens if you are trying to reduce the amount of browser rendering by temporarily removing some content into a Document Fragment, make a whole lot of changes and then place that content back in the document. i've attached a test case that simulates the document fragment example.

is there a way to prevent the onload handler from firing multiple times?

Attachments (2)

11432.html (1.2 KB) - added by ben hockey 9 years ago.
11432.diff (814 bytes) - added by ben hockey 9 years ago.

Download all attachments as: .zip

Change History (9)

Changed 9 years ago by ben hockey

Attachment: 11432.html added

comment:1 Changed 9 years ago by bill

Hmm, we could easily remember the first time it fires and then ignore subsequent fire events... not sure if that will make everything work or not.

comment:2 Changed 9 years ago by ben hockey

yeah... i can see ways of how we might do it - checking this.isLoaded might be one way to do it but my concern is more about what i don't know. will anything break if we do that? i've tried to put together a patch that wraps the current onLoad in an if(!this.isLoaded){ /* current onLoad code */ } but i can't get a diff right now. the patch is not really needed - it's just adding that if statement. adding the if doesn't break any unit tests and fixes the problem but someone else more familiar with the editor should try it and look over it.

comment:3 Changed 9 years ago by liucougar

reattach an iframe will cause browser to reload the iframe, so I think onLoad should still fire

as to this bug: we can create a new deferred when the editor is reattached?

comment:4 in reply to:  3 Changed 9 years ago by ben hockey

Summary: dijit.Editor - prevent onLoadDeferred from resolving multiple times[patch][cla]dijit.Editor - prevent onLoadDeferred from resolving multiple times

Replying to liucougar:

reattach an iframe will cause browser to reload the iframe, so I think onLoad should still fire

i agree that it could make sense to have some indication of onLoad since the iframe is reloaded. i would think that anything that should only be run once is added as a callback to onLoadDeferred and anything which needs to know about every time onLoad happens would connect to onLoad rather than adding a callback to the Deferred.

as to this bug: we can create a new deferred when the editor is reattached?

i had thought about something like this as well but since the editor does not know that it has been detached and reattached there are some problems i see with this idea. the first indication that the editor receives about something like this happening is that onLoad is called after the iframe has loaded again. at this point, it's not useful to create another Deferred since onLoad has already happened - ie the Deferred would only be created during onLoad and so if anybody was interested in adding a callback to this new Deferred they could only add their callback after onLoad which makes the Deferred mostly worthless since the Deferred only exists after onLoad has happened.

i think if somebody had an interest in knowing every time the iframe is loaded, their best option is to connect to onLoad. using a Deferred to indicate multiple onLoad events is probably not the appropriate thing to do anyway since Deferreds should only indicate the completion of something which is expected to happen once.

i gave it quite some thought when i found this bug and couldn't figure out what was best to do for indicating the 2nd onLoad but i ended up deciding that creating a new Deferred wouldn't be a good option since there is not a good time to create it and a Deferred should only be resolved once so a property which is a Deferred should behave in the same way.

i took a look at the plugins in dijit (i didn't look at dojox) and all of them add a callback to onLoadDeferred for any code that needs to run after the iframe is ready. this is good since that code won't be called again if onLoad is called but i'm also left wondering if some of that code might need to be called again. do you know if anything happens with the iframe which would mean that a plugin might need to reinitialize itself?

anyhow, for this bug, we at least need to prevent onLoadDeferred from resolving more than once. i've attached a patch for something which will achieve this.

Changed 9 years ago by ben hockey

Attachment: 11432.diff added

comment:5 Changed 9 years ago by bill

I agree that it doesn't make sense to have a Deferred for something that happens multiple times.

do you know if anything happens with the iframe which would mean that a plugin might need to reinitialize itself

I suspects the connect() calls will have issues, like for LinkDialog, clicking on an image to open the TooltipDialog for modifying the image's src attributes.

comment:6 Changed 9 years ago by Jared Jurkiewicz

There are many plugins that connect to the iframe document (and need to). So any reload of the iframe will flat break them. This is a known issue with Firefox, you cannot move an iframe without it reloading. It's annoying, but it's a limitation. I've not found any way around it (and believe me, I looked when I wrote FullScreen? because it would have made its logic tons simpler if I could move the frame).

-- Jared

comment:7 Changed 9 years ago by Jared Jurkiewicz

Resolution: wontfix
Status: newclosed

Limitation of iframes in FireFox?. Cannot resolve as many plugins may/do depend on binding to the iframe document on instantiation and moving the editor reloads he iframe. I spent considerable time trying to resolve this without much luck, as it would make plugins like FullScreen? simpler to implement if the editor could be moved.

Marking wontfix, there is no real solution here. The editor can't be moved in the DOM once positioned.

Note: See TracTickets for help on using tickets.