[patch][ccla]Shrinksafe token naming conflicts with short var names like _e

[Nick Fenwick]

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 dojo@…. 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.

[Nick Fenwick]

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.

[Adam Peller]

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

[Adam Peller]

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?

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

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));
	}
}

[Adam Peller]

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.

[Nick Fenwick]

Thanks for the feedback. Straight from unpacking the 1.3.0rc1 release I downloaded:

[neek@uberneek 1.3.0rc1]$ tar zxf /downloads/dojo-release-1.3.0rc1.tar.gz
[neek@uberneek 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.

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

[dante]

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

[dante]

sample showing the bug in shrinksafe

[dante]

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

[Adam Peller]

Dante beat me to it. Here's my test for util/shrinksafe/tests

[Adam Peller]

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.

[bill]

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.

[Adam Peller]

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)

[Adam Peller]

Map all tokens which begin with "_"

[Adam Peller]

in [17119] patched up _Plugin.js to avoid the problem. Removed "_" chars in short var names for Dojo 1.3.

[Adam Peller]

Fixed in [17149]