Opened 8 years ago
Closed 8 years ago
#16002 closed defect (fixed)
dojox.mobile._ContentPaneMixin throws exception
Reported by: | Bill Reed | Owned by: | Eric Durocher |
---|---|---|---|
Priority: | high | Milestone: | 1.8.1 |
Component: | DojoX Mobile | Version: | 1.8.0 |
Keywords: | Cc: | cjolif | |
Blocked By: | Blocking: |
Description
With Maqetta.org we allow users to dynamically change mobile themes within the html visual editor. When the theme is changed on a page we reparse the dojo widgets, the re-parsing of a dojox.mobile.ContentPane? with a href attribute throws and exception. dojo/parser::parse() error DOMException
parser.js:411
(anonymous function) parser.js:411 _4b8 dojo.js:15 _4b7.then._4c5.then dojo.js:15 lang.extend.otherwise dojo.js:15 _1a.parse parser.js:410 _2246._processWidgets davinci.js:48614 dojo.ready.dojo.addOnLoad._5de dojo.js:15 _5da dojo.js:15 _5d6 dojo.js:15 dojo.ready.dojo.addOnLoad dojo.js:15 _2246._processWidgets davinci.js:48612 _447 dojo.js:5518 _446.then._454.then dojo.js:5628 _2246._processWidgets davinci.js:48611 _2246._setSourceData davinci.js:48587 _2246._continueLoading davinci.js:48464 _2246._setSource davinci.js:48357 ret.withDoc dojo.js:6549 _2246.setSource davinci.js:48207
I have debug the problem dojox.mobile.ProgressIndicator
Here is the flow of what happens
When a dojox.mobile._ContentPaneMixin
is constructed a instance of a progress indicator is retrieved
constructor: function(){ // summary: // Creates a new instance of the class. // tags: // private if(this.prog){ this._p = ProgressIndicator.getInstance(); } },
The ProgressIndicator.getInstance
creates a ProgressIndicator
if one does not already exist and assigned it to the global cls._instance
cls.getInstance = function(props){ if(!cls._instance){ cls._instance = new cls(props); } return cls._instance; };
dojox.mobile._ContentPaneMixin._setHrefAttr
is callled if we have an instences of ProgressIndicator
it's dom is appended to the body
if(this.lazy || !href || href === this._loaded){ this.lazy = false; return null; } var p = this._p; if(p){ win.body().appendChild(p.domNode); p.start(); }
Once the content is loaded from the href dojox.mobile._ContentPaneMixin._setContentAttr
is called which then calls the ProgressIndicator.stop()
this.destroyDescendants(); if(typeof data === "object"){ this.containerNode.appendChild(data); }else{ if(this.executeScripts){ data = this.execScript(data); } this.containerNode.innerHTML = data; } if(this.parseOnLoad){ var _this = this; return Deferred.when(lazyLoadUtils.instantiateLazyWidgets(_this.containerNode), function(){ if(_this._p){ _this._p.stop(); } return _this.onLoad(); }); } if(this._p){ this._p.stop(); } return this.onLoad(); }
in ProgressIndicator.stop
the domNode of the progress indicator is removed, but the cls._instance
in not set to null
stop: function(){ // summary: // Stops the spinning of the ProgressIndicator. if(this.timer){ clearInterval(this.timer); } this.timer = null; if(this.removeOnStop && this.domNode && this.domNode.parentNode){ this.domNode.parentNode.removeChild(this.domNode); } },
So that the next time the _ContentPaneMixin
constructor is call the ProgressIndicator.getInstance();
returns the old instance of ProgressIndecator
that is missing the domNode. So that when dojox.mobile._ContentPaneMixin._setHrefAttr
and attempts to execute the line
var p = this._p; if(p){ win.body().appendChild(p.domNode); p.start(); }
an exception is thrown
if I add code to dojox.mobile.ProgressIndicator.stop
that clears the cls._instance
stop: function(){ // summary: // Stops the spinning of the ProgressIndicator. if(this.timer){ clearInterval(this.timer); } this.timer = null; if(this.removeOnStop && this.domNode && this.domNode.parentNode){ this.domNode.parentNode.removeChild(this.domNode); cls._instance = null; // wdr } },
then the exception is resolved.
Attachments (3)
Change History (18)
Changed 8 years ago by
Attachment: | ProgressIndicator.js.patch added |
---|
comment:1 Changed 8 years ago by
I have attached a possible patch, Also here is a link to the Maqetta github issue https://github.com/maqetta/maqetta/issues/3394
comment:2 Changed 8 years ago by
Component: | General → DojoX Mobile |
---|---|
Owner: | set to Eric Durocher |
comment:3 follow-up: 4 Changed 8 years ago by
I don't really see a problem in this code sequence.
The stop method remove the ProgressIndicator's domNode:
this.domNode.parentNode.removeChild(this.domNode);
Then _ContentPaneMixin re-adds it to the document:
win.body().appendChild(p.domNode);
That seems correct to me. Also I don't quite understand this:
> ProgressIndicator.getInstance(); returns the old instance of ProgressIndicator that is missing the domNode
What do you mean by "missing"? Is the domNode reference null? It has been removed from its parent, but it should still be a valid node that can be added again to the DOM.
Are you sure that there no other code that would accidentally destroy the ProgressIndicator?
Can you reproduce on a simple sample, outside of Maqetta?
comment:4 Changed 8 years ago by
Replying to edurocher:
I don't really see a problem in this code sequence.
The stop method remove the ProgressIndicator's domNode:
this.domNode.parentNode.removeChild(this.domNode);Then _ContentPaneMixin re-adds it to the document:
win.body().appendChild(p.domNode);That seems correct to me. Also I don't quite understand this:
> ProgressIndicator.getInstance(); returns the old instance of ProgressIndicator that is missing the domNode
What do you mean by "missing"? Is the domNode reference null? It has been removed from its parent, but it should still be a valid node that can be added again to the DOM.
Correct the ProgressIndicator?.getInstrance() returns the ProgressIndicator? object p
the object does not have a domNode property, p.domNode must have been delete somewhere.
Are you sure that there no other code that would accidentally destroy the ProgressIndicator?
We do not have any code that would delete the progress indicator. We do destroy and recreate the ContentPane?
Can you reproduce on a simple sample, outside of Maqetta?
I have not tried to recreate outside of Maqetta. I was hoping you might have an idea where the domNode is being destroyed. I will attempt to recreate outside of maqetta.
comment:5 Changed 8 years ago by
Thanks. No, sorry I don't see any call (at least in dojox/mobile) that could possibly destroy the indicator (especially since it is added directly to the body, so it could not be destroyed as part of a destroyDescendants for example).
comment:6 Changed 8 years ago by
We do destroy all the dojo widgets in the registry before we re-parse the page so we do not get widgetid already defined errors. It looks like when the ContentPane? or the progress bar are destoryed they do not delete the cls._instance
Here is an HTML test file that reproduces the error
<!DOCTYPE html> <html> <head> <meta http-equiv="Content-type" content="text/html; charset=utf-8"/> <meta name="viewport" content="width=device-width,initial-scale=1,maximum-scale=1,minimum-scale=1,user-scalable=no"/> <meta name="apple-mobile-web-app-capable" content="yes"/> <title>ContentPane</title> <script type="text/javascript" src="../deviceTheme.js" data-dojo-config="mblThemeFiles: ['base']"></script> <script type="text/javascript" src="../../../dojo/dojo.js" data-dojo-config="async: true, parseOnLoad: true"></script> <script type="text/javascript"> require([ "dojo/dom-construct", "dojo/ready", "dijit/registry", "dojox/mobile/parser", "dojox/mobile", "dojox/mobile/compat", "dojox/mobile/ContentPane" ], function(domConstruct, ready, registry, parser, mobile,compat,ContentPane){ ready(function(){ window.setTimeout(function(){ dijit.registry.toArray().forEach(function(w){ w.destroy(); }); var cp = new ContentPane({id:"pane2", href:"data/fragment2.html"},'div1'); },2000); }); }); </script> </head> <body style="visibility:hidden;"> <div id='div1'> <div id="pane2" data-dojo-type="dojox.mobile.ContentPane" href="data/fragment1.html"></div> </div> </div> </body> </html>
comment:7 Changed 8 years ago by
A better fix to to add a destroy()
method to ProgressIndicator
I still need to test this fix inside Maqetta but I bet it will work
Changed 8 years ago by
Attachment: | ProgressIndicator.js.2.patch added |
---|
Better path, add destroy method
comment:9 follow-up: 10 Changed 8 years ago by
Definitely better yes, but there is no reason to clear the singleton if another ProgressIndicator instance is destroyed, so we should test that the destroyed instance is actually the shared instance. I am attaching a modified patch, could you also test this in Maqetta please? thanks.
Changed 8 years ago by
Attachment: | ProgressIndicator.js.3.patch added |
---|
Test if the destroyed instance is the shared instance - Eric Durocher (IBM, CCLA)
comment:10 Changed 8 years ago by
Replying to edurocher:
Definitely better yes, but there is no reason to clear the singleton if another ProgressIndicator instance is destroyed, so we should test that the destroyed instance is actually the shared instance. I am attaching a modified patch, could you also test this in Maqetta please? thanks.
This fix works on Maqetta.
comment:11 follow-up: 12 Changed 8 years ago by
Cc: | cjolif added |
---|---|
Priority: | undecided → high |
OK, thanks for the test, then for me this patch 3 is OK to commit.
comment:12 Changed 8 years ago by
Replying to edurocher:
OK, thanks for the test, then for me this patch 3 is OK to commit.
It works for us.
comment:14 Changed 8 years ago by
Milestone: | tbd → 1.8.1 |
---|
Possible patch