Opened 12 years ago
Closed 12 years ago
#8974 closed defect (fixed)
[patch][ccla]Shrinksafe token naming conflicts with short var names like _e
Reported by: | Nick Fenwick | Owned by: | Adam Peller |
---|---|---|---|
Priority: | high | Milestone: | 1.4 |
Component: | ShrinkSafe | Version: | 1.3.0b3 |
Keywords: | Cc: | bill, Richard Backhouse | |
Blocked By: | Blocking: |
Description
Dojo 1.3.0 rc1 (not an option in the Version dropdown on this bug form). Firefox 3.0.7, running on a glassfish v3prelude app server.
Trying to use dijit.Editor straight from the docs:
<textarea name="item_description_html" class="itemDescriptionEditor" dojoType="dijit.Editor"> It was a dark and stormy night. Your story belongs here! </textarea>
I get error as it initialises, firebug reports:
_e.queryCommandState is not a function
It's on the lines in _Plugin.js that muck up the _e variable in the updateState handler.
Fixed code seems to be:
if(typeof this.button.checked=="boolean"){ var _d=_e.queryCommandState(_c); if(this.checked!==_d){ this.checked=_d; this.button.attr("checked",_e.queryCommandState(_c)); } }
I am [email protected]…. I previously commented on this at http://www.dojotoolkit.org/forum/dijit-dijit-0-9/dijit-support/dojo-1-2-3-dijit-editor-throwing-e-querycommandstate-line.
Attachments (4)
Change History (19)
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
Component: | Dijit → Editor |
---|---|
Owner: | set to liucougar |
svn head will be 1.3.0 final. You could try testing against 1.3.0rc2, built yesterday, or even the nightlies at archive.dojotoolkit.org
comment:3 Changed 12 years ago by
In the source, the assignment is made to a different variable called "enabled" so there should not be any conflict. Shrinksafe doesn't usually rename variables to look like "_e". Where did you get the above code?
comment:4 follow-up: 5 Changed 12 years ago by
I don't see an issue in the 1.3.0rc1 _Plugin.js, and actually I don't see any significant difference between the code you pasted above and the code in the release:
if(typeof this.button.checked == 'boolean'){ var checked=_e.queryCommandState(_c); if(this.checked!==checked){ this.checked=checked; this.button.attr('checked', _e.queryCommandState(_c)); } }
comment:5 Changed 12 years ago by
Replying to bill:
I don't see an issue in the 1.3.0rc1 _Plugin.js, and actually I don't see any significant difference between the code you pasted above and the code in the release
Bill - this referenced forum post alleges that 'var checked' was actually 'var _e' thus clobbering a variable. I don't see this, however.
comment:6 Changed 12 years ago by
Thanks for the feedback. Straight from unpacking the 1.3.0rc1 release I downloaded:
[[email protected] 1.3.0rc1]$ tar zxf /downloads/dojo-release-1.3.0rc1.tar.gz [[email protected] 1.3.0rc1]$ find . -name _Plugin.js ./dojo-release-1.3.0rc1/dijit/_editor/_Plugin.js ./dojo-release-1.3.0rc1/dojox/sketch/_Plugin.js
That _editor/_Plugin.js contains:
56: if(typeof this.button.checked=="boolean"){ 57: var _e=_e.queryCommandState(_c); 58: if(this.checked!==_e){ 59: this.checked=_e; 60: this.button.attr("checked",_e.queryCommandState(_c)); 61: } 62: }
1.3.0 rc2 (http://download.dojotoolkit.org/release-1.3.0rc2/dojo-release-1.3.0rc2.tar.gz) also contains this code. Am I going crazy?
1.3.0rc2-src.tar.gz contains correct code:
if(typeof this.button.checked == 'boolean'){ var checked=_e.queryCommandState(_c); if(this.checked!==checked){ this.checked=checked; this.button.attr('checked', _e.queryCommandState(_c)); } }
So in my state of ignorance I'd presume it's a bug caused by a shrinksafe artifact, or some other problem with the release build process.
comment:7 Changed 12 years ago by
Milestone: | tbd → 1.3 |
---|---|
Owner: | changed from liucougar to dante |
ugh this is definitely a shrinksafe thing. passing that file alone through shrinksafe gives:
if(typeof this.button.checked=="boolean"){ var _e=_e.queryCommandState(_c); if(this.checked!==_e){ this.checked=_e; this.button.attr("checked",_e.queryCommandState(_c)); }
my guess is the try{}catch(e){} is tricking it somehow.
attaching a style/cleanup and fix patch now.
also not sure why there is a console.debug, but stripConsole steals that.
relevant shrinksafe output after changes:
},updateState:function(){ var _b=this.editor,_c=this.command,_d,_e; if(!_b){ return; } if(!_b.isLoaded){ return; } if(!_c.length){ return; } if(this.button){ try{ _e=_b.queryCommandEnabled(_c); if(this.enabled!==_e){ this.enabled=_e; this.button.attr("disabled",!_e); } if(typeof this.button.checked=="boolean"){ _d=_e.queryCommandState(_c); if(this.checked!==_d){ this.checked=_d; this.button.attr("checked",_b.queryCommandState(_c)); } } } catch(e){ console.debug(e); } } }
It is a trivial fix and probably should be before 1.3 ... no?
Changed 12 years ago by
Attachment: | plugin.patch added |
---|
comment:8 Changed 12 years ago by
Cc: | bill added |
---|
fwiw, the above ss output is before a missed _e reference in the patch (which has been updated)
@bill - cool for 1.3?
I would love to see an option to do some kind of AST check for shrinksafe before/after
comment:9 Changed 12 years ago by
so this is definitely shrinksafe's issue, the output of the above fun.js sample:
(function(){ var _1,_2,_3,_4,_5,_6,_7,_8,_9,_a,_b,_c; var _d=function(_e){ var _e=_e; }; });
it is limited to the fact the var _e = something is the (1E?) variable in this scope. It likely doesn't exist when this file is in a layer and doesn't exist if the variable is anything _but_ _e ...
ugh
Changed 12 years ago by
Attachment: | 8974.patch added |
---|
Dante beat me to it. Here's my test for util/shrinksafe/tests
comment:10 Changed 12 years ago by
Cc: | Richard Backhouse added |
---|---|
Keywords: | shrinksafe added; dijit.Editor querycommandstate removed |
I suggest we patch Plugin.js for 1.3, but leave this ticket open as a ShrinkSafe? issue (change to component ShrinkSafe?, or make a new ticket, if we don't care about conserving tickets)
neek, thanks for catching this.
comment:11 Changed 12 years ago by
Priority: | normal → high |
---|
That sounds fine to me too, of course. Workaround it for now and then fix the real shrinksafe problem later, although "later" might not come for a while.
Would be really nice to get some unit tests for shrinksafe too.
comment:12 Changed 12 years ago by
Component: | Editor → ShrinkSafe |
---|---|
Keywords: | shrinksafe removed |
Milestone: | 1.3 → 1.4 |
Owner: | changed from dante to Adam Peller |
Status: | new → assigned |
Bill - we now have the beginnings of ShrinkSafe unit tests [16542] [16543] and [16909] Also note the patch for 8974.js above.
Whether !Shrinksafe bugs are fixed tends to be a function of whether anyone knows how to fix them (or if Rhino updates magically fix bugs for us)
comment:13 Changed 12 years ago by
Summary: | 1.3.0 rc1 dijit/_editor/_Plugin.js bug calling queryCommandState → [patch][ccla]1.3.0 rc1 dijit/_editor/_Plugin.js bug calling queryCommandState |
---|
comment:14 Changed 12 years ago by
Summary: | [patch][ccla]1.3.0 rc1 dijit/_editor/_Plugin.js bug calling queryCommandState → [patch][ccla]Shrinksafe token naming conflicts with short var names like _e |
---|
in [17119] patched up _Plugin.js to avoid the problem. Removed "_" chars in short var names for Dojo 1.3.
Incidentally, the problem appears to be fixed on svn head, but I may be misunderstanding, and have no idea if that version is likely to be in 1.3.0 final. The implementation looks quite different, but perhaps what I see coming from svn is different to the 1.3.0rc1 release tarball because of some javascript munging in the build process.