Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#8492 closed enhancement (fixed)

Whitespace in tempates does the wrong thing?

Reported by: ryangroth5 Owned by: bill
Priority: high Milestone: 1.3
Component: Dijit Version: 1.2.3
Keywords: Cc: dojo@…, Eugene Lazutkin
Blocked By: Blocking:

Description

In a strange svn merge that left me with newlines in my template files I observed that the chain of execution from dijit._Templated threw errors with trailing whitespace in widget templates.

	// summary:
			//		Construct the UI for this widget from a template, setting this.domNode.

			// Lookup cached version of template, and download to cache if it
			// isn't there already.  Returns either a DomNode or a string, depending on
			// whether or not the template contains ${foo} replacement parameters.
			var cached = dijit._Templated.getCachedTemplate(this.templatePath, this.templateString, this._skipNodeCache);

			var node;
			if(dojo.isString(cached)){
				node = dojo._toDom(this._stringRepl(cached));
			}else{
				// if it's a node, all we have to do is clone it
				node = cached.cloneNode(true);
			}

			this.domNode = node;

			// recurse through the node, looking for, and attaching to, our
			// attachment points and events, which should be defined on the template node.
			this._attachTemplateNodes(node);

The error,in short, is that _Templated gets the template code (which looks like "<div>...</div>\n" in the error case ) then passes it to dojo._toDom() which converts the block to a DocumentFragment? (because of the hanging \n). This gets passed to _attachTemplateNodes() which expects a dom node NOT a document fragment.

This is an artifact of dojo._toDom creating a document fragment in the case that there is more than one node in the incoming string (our second node is the "\n" text node).

atachTemplateNodes doesn't know what to do with this and throws a method not found error. In the case of no trailing whitespace, then we get our node and everything proceeds.

So, what I would suggest would be one of the following:

1) Throw an error in attachTemplateNodes if you get a document fragment, or indeed, anything you don't want.

2) Trim templates somewhere so that you don't get document fragments when dojo users/extenders are accidentally leaving trailing whitespace in their template files.

3) Document profusely that NO whitespace be allowed in the widget template files

THIS ERROR APPLIES TO 1.3pre. I haven't noticed it before 1.3.

Thanks,

Ryan CLA on file

Change History (10)

comment:1 Changed 10 years ago by bill

Owner: set to Eugene Lazutkin

Eugene, so this really seems like an error/unwanted behavior with dojo._toDom(). Can you change that code to do a trim? Or should I add a trim to _Templated?

comment:2 Changed 10 years ago by Eugene Lazutkin

Cc: Eugene Lazutkin added
Owner: changed from Eugene Lazutkin to bill

Actually it is the expected behavior of dojo._toDom() --- it can be used to create text nodes as well. The best way to handle it is to add dojo.trim() to _Templated like you suggested.

comment:3 Changed 10 years ago by bill

Resolution: fixed
Status: newclosed

(In [16482]) Trim leading/trailing space from template files in case there is a trailing newline, which causes dojo._toDom() to return a DocumentFragment? (the real node and a trailing text node) rather than a single node.

Fixes #8492 !strict.

comment:4 Changed 10 years ago by ryangroth5

Resolution: fixed
Status: closedreopened

Same issue, but now I am getting this on the build version. What is happens is that the build tool puts the newline into the template string. The template string bypasses the fix in [16482] since [16482] addresses only the template file. The template file newline issue is fixed, however.

Thanks,

Ryan

comment:5 Changed 10 years ago by ryangroth5

OK. I know what is at issue here. Apparently dojo uses the native string trim if it i available (I don't know which browser might do this) in dojo.trim(). Because of namespace polution from another library my browser suddenly defined an inferior string trim operation which doesn't take care of the "\n" case. Sorry for reopening this. I'd consider it closed unless you want to take care of the dojo code assuming that String.prototype.trim is native.

comment:6 in reply to:  5 Changed 10 years ago by Adam Peller

Replying to ryangroth5:

Because of namespace polution from another library my browser suddenly defined an inferior string trim operation which doesn't take care of the "\n" case.

Which library is doing this?

comment:7 Changed 10 years ago by ryangroth5

OpenLayers? 2.7 which has some prototype in it.

comment:8 Changed 10 years ago by Adam Peller

Interesting. OpenLayers? uses this replace for its trim:

replace(/^\s*(.*?)\s*$/, "$1")

which generally strips \r and \n though some strange combinations don't work, such as

" \ra\nbc\n ".replace(/^\s*(.*?)\s*$/, "$1")

OpenLayers? overrides String.prototype.trim, but the override is now marked as 'deprecated' so they must have decided it was a mistake to do so... perhaps they can be encouraged to remove it?

comment:9 Changed 10 years ago by bill

Resolution: fixed
Status: reopenedclosed

Hmm, OK, I'm going to close this ticket again.

You could open another ticket about the Dojo/OpenLayers incompatibility, but I'm skeptical that this is worth working-around in dojo. There is a workaround possible in your code too: you can redefine String.prototype.trim to work correctly.

comment:10 Changed 10 years ago by Adam Peller

I'd encourage you to file a ticket against OpenLayers? as well. They may want to look at implementations here:

http://blog.stevenlevithan.com/archives/faster-trim-javascript

Note: See TracTickets for help on using tickets.