Opened 11 years ago

Closed 11 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 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.

Attachments (4)

plugin.patch (1.3 KB) - added by dante 11 years ago.
fun.js (145 bytes) - added by dante 11 years ago.
sample showing the bug in shrinksafe
8974.patch (921 bytes) - added by Adam Peller 11 years ago.
Dante beat me to it. Here's my test for util/shrinksafe/tests
varconflict.patch (743 bytes) - added by Adam Peller 11 years ago.
Map all tokens which begin with "_"

Download all attachments as: .zip

Change History (19)

comment:1 Changed 11 years ago by 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.

comment:2 Changed 11 years ago by Adam Peller

Component: DijitEditor
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 11 years ago by 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?

comment:4 Changed 11 years ago by 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));
	}
}

comment:5 in reply to:  4 Changed 11 years ago by 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.

comment:6 Changed 11 years ago by 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.

comment:7 Changed 11 years ago by dante

Milestone: tbd1.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 11 years ago by dante

Attachment: plugin.patch added

comment:8 Changed 11 years ago by dante

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

Changed 11 years ago by dante

Attachment: fun.js added

sample showing the bug in shrinksafe

comment:9 Changed 11 years ago by 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

Changed 11 years ago by Adam Peller

Attachment: 8974.patch added

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

comment:10 Changed 11 years ago by Adam Peller

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 11 years ago by bill

Priority: normalhigh

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 11 years ago by Adam Peller

Component: EditorShrinkSafe
Keywords: shrinksafe removed
Milestone: 1.31.4
Owner: changed from dante to Adam Peller
Status: newassigned

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)

Changed 11 years ago by Adam Peller

Attachment: varconflict.patch added

Map all tokens which begin with "_"

comment:13 Changed 11 years ago by Adam Peller

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 11 years ago by Adam Peller

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.

comment:15 Changed 11 years ago by Adam Peller

Resolution: fixed
Status: assignedclosed

Fixed in [17149]

Note: See TracTickets for help on using tickets.