Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#11490 closed enhancement (fixed)

Support HTML5's custom data-* prefix for non-standardized attributes

Reported by: dylan Owned by: dante
Priority: high Milestone: 1.6
Component: Parser Version: 1.5
Keywords: Cc:
Blocked by: Blocking:

Description

I receive repeated requests asking if Dojo will support the HTML5 approach to custom attributes, namely using the data-* prefix for our custom attributes.

I believe we should make this available in Dojo 1.6. It's more verbose, and requires more logic in the parser, but it is the direction the world wants us to move.

Attachments (8)

html5-parser.patch (36.8 KB) - added by dante 7 years ago.
initial patch. thoughts?
html5-parser.2.patch (15.4 KB) - added by dante 7 years ago.
updated patch.
html5-parser.3.patch (83.0 KB) - added by dante 7 years ago.
html5-parser.4.patch (92.4 KB) - added by dante 7 years ago.
themetester validates. patch includes fix for select to support data-dojo-value for nonstandard children and avoids getObject for inlineeditor if passed a reference.
html5-parser.5.patch (92.4 KB) - added by dante 7 years ago.
-4 has a small bug in it. d in undefined.
html5-parser.6.patch (100.3 KB) - added by dante 7 years ago.
keeps everything working. could be faster, still need to steal some args from node for the time being. also widgetsInTemplate is causing me pain.
html5-parser.7.patch (101.6 KB) - added by dante 7 years ago.
current snapshot for bill
html5-parser.8.patch (18.0 KB) - added by dante 7 years ago.
parser patch only.

Download all attachments as: .zip

Change History (88)

comment:1 follow-up: Changed 7 years ago by bill

That just means sticking data- in front of every attribute name, like this?

<div data-dojoType="dijit.TitlePane" data-open="true"> ...

Well, I guess in front of every non-standard attribute name, and official attribute names are the same as before, like this?

<div data-dojoType="dijit.form.Button" data-iconClass="foo" name="bar"> ...

comment:2 Changed 7 years ago by dante

the easy solution we be to say "data-*" as a map to dijit[*] ... unfortunately, people want things like style="" as native, and nonstandard things like isvalid as data-isvalid .. maintaining a map of valid attributes and nonstandard-dijit-added attributes is of little interest to me.

randomly, just now:

<div data-dojoType="dijit.form.Button" data-dojoProps="isValid: somefn, label:'click me!'></div>

bad example wrt to label:"click me" and being a button/div, but the idea is that all data-dojoProps are mixed into the things that are otherwise native.

that way the onus of html validity lies on the developer. mixing "anything" in data-dojoProps (parsing the same as we do djConfig="" on dojo.js script) into whatever is already deemed part of the widget allows both options.

<div data-dojoType="Thinger" class="bar" data-dojoProps="a:1, onClick: somefn, b:[1,2]">innerhtmlstuff</html>

// example:
dojo.declare("Thinger", null, {
    "class":"baz", // standard
    a: 10, // can be a="" or data-dojoProps="a:N" 
    onClick: function(){},
    constructor: function(args){
        this.b = args.b || [];
    }
});

seems a happy compromise? support both via one additional explicit prop. also worth noting that the same reason {{data-dojoType}} will work in all the worlds older browsers is the same reason the "invalid" {{dojoType}} attribute works. we won't be gaining anything except online html validators won't complain at the cost of the additional parsing and whatnot despite this already working.

comment:3 Changed 7 years ago by dante

also plugd has had a data-dojoType "parser" for a while:

dojo.provide("plugd.parser");
// the html5 dojo-parser. only use if you can take the performance hit.
dojo.require("dojo.parser");
(function(d){
	
	var p = d.parser, op = p.parse, da = "data-", dt = "dojoType";
	d.parser.parse = function(rootNode, care){
		// summary: Identical to `dojo.parser.parse`, though defaults to
		//		the html5 data-* validation acceptance. Disable this
		//		magic by passing the `care` attribute. 
		if(!care){ 
			// find data-dojoType, and adjust to dojoType in this rootNode 
			d.query("[" + da + dt + "]", rootNode).forEach(function(n){
				dojo.attr(n, dt, dojo.attr(n, da + dt));
			});
		}
		return op.call(p, rootNode);
	}
	
})(dojo);

comment:4 in reply to: ↑ 1 Changed 7 years ago by elazutkin

Replying to bill:

That just means sticking data- in front of every attribute name, like this?

Theoretically it may mean a different way to access custom data attributes. All data- prefixed attributes are magical --- they are placed in a dataset property bag with an easy access. Even hyphenated names are converted to the camel case: http://www.w3.org/TR/html5/elements.html#custom-data-attribute

Unfortunately to my best knowledge as of today nobody implemented it in any browser. Time will cure that.

comment:5 Changed 7 years ago by bill

  • Component changed from Dijit to Parser
  • Owner set to bill

Sounds fine, if someone wants to write some unit tests and then update the parser code, go for it.

Changed 7 years ago by dante

initial patch. thoughts?

comment:6 Changed 7 years ago by markwubben

I'll just say that my custom Widget subclass goes through data-* attributes and adds them to the instance, just like any other attribute is added. Normal attributes takes precedence (IIRC), but other than that it's a normal mapping.

Don't quite see the point of data-dojo-props, we don't support dojoProps either do we? Or have I completely missed the existence thereof? Why the data-dojo-type rather than data-dojoType?

comment:7 Changed 7 years ago by dante

take a peek at the patch. The random thought above didn't turn out the be how I implemented it in this patch.

<script src="dojo.js" data-dojo-config="parseOnLoad:true"></script>

supersedes djConfig="". both still work.

<div id="foo" data-dojoType="dijit.form.Button"></div>

supersedes dojoType="". both still work.

<input data-dojoType="InlineEdit" type="text" data-dojo-editor="another.Class.Reference">

so basically data-dojo (actually data-scopeName) get's cutoff. getdataset just reads from the "native" data-* pattern, and strips the dojo part off, then mixes those props into the list of props for actual processing.

comment:8 Changed 7 years ago by dante

thought, interestingly, I'm running into issues with the native branch.

in webkit nightly, a node like:

<p data-dojoType="bar" data-dojo-type="baz">

node.dataset = {
   dojotype:"bar", dojoType:"baz"
}

because of the camel case conventions in the spec. I had to account for this in the code because data-dojo-type="text" is a valid dijit prop. fortunately the actual data-dojotype attr is read at scantime, so is not considered in the props at all.

comment:9 Changed 7 years ago by bill

The patch generally looks good to me.

It seems like

<p data-dojoType="bar" ...

is contrary to the HTML5 spec because it has a capital letter, see the first sentence of http://www.w3.org/TR/html5/elements.html#custom-data-attribute.

One thing about the patch, the majority of lines changed are converting all < and > in comments to [ and ]; I don't see any point to that. Most of those comments aren't even API documentation, but just internal comments.

Also, it needs an automated test case.

About stuffing all widget parameters into one markup parameter, like your data-dojoProps example, I have thought about that in the past too, just never got around to trying it. It would probably have a big performance benefit.

comment:10 Changed 7 years ago by dante

  • Milestone changed from tbd to 1.6
  • Owner changed from bill to dante
  • Status changed from new to assigned

I apologize for the doc comment changes. TextMate's build-in jslint doesn't like a literal <script> tag, even in comments. I see the warning/error, but when I go to parse it claims "cannot find closing </script>" element, despite the type being set to "javascript" ... commenting allows me to lint the code in place. that's what those changes are there, fwiw.

I'm glad you brought up the spec. I did the data-dojoType because data-dojo-type converts to dataset.dojoType and it conflicted with mixed native/custom use of the type="" attribute. Would love to hear your thoughts on that. Also, things like searchAttr can work as data-dojo-search-attr="" and convert natively and in the fallback getAttribute branch. I pointed out above about pre-camelCased variables and native dataset support:

<div data-bar="1" data-fooBar="2" data-foo-bar="3"></div>

if the markup has a literal pre-camelCased attribute, the native dataset converts it toLowerCase() before mixing in a camelCase. eg:

var dataset = { bar:"1", foobar:"2", fooBar:"3" };

taking the spec literally, foobar:"2" shouldn't be there at _all_, as it's not a dataset item. getAttribute("data-fooBar") works though. So I chose data-dojoType so we could have an attribute to read the class from (because it works) and data-dojo-type to use as a type="" attribute replacement.

<div type="something"></div>

isn't valid.

happy to introduce a "one attr to rule them all" param:

data-dojo-props="store: literalObjectReferece, onClick:'console.log(arguments)'"

but we should decide ahead of time if we want the props from _that_ mixed in before or after any possible data-dojo-* props (excluding 'props' of course). If we do all the params{} mixing before we ever send it through to the classInfo loop, all "modes" will work. the order of that "special" param will be important to note though. It would have a good performance benefit if we mandated all nonstandard props go in that attr, but that could get ugly especially when hand typed.

comment:11 Changed 7 years ago by bill

I'm not sure what the best naming for dojoType is, but we clearly need to follow the HTML5 spec completely or there won't be any point (after all, this ticket is made to make people happy that we are following the spec). Some possible options for dojoType are:

  • data-dojo-widget
  • data-dojo-widget-class
  • data-dojo-widget-type
  • data-dojo
  • data-dojo-class (and pass in the "class" attribute as a native attribute, although maybe that's bad because then developers can't specify a DOMNode's class and a widget's class independently)
  • data-dojo-dijit

etc. I'm leaning toward the first one.

About having a data-dojo-props attribute, I actually meant for all widget attributes to go in there, even things like "type" and "class". That's the only way to achieve the performance goal (theoretically speaking, anyway).

comment:12 Changed 7 years ago by elazutkin

Hmm, it may make sense to use a property bag as a sole attribute instead of iterating through attributes. But I am not sure that data-dojo-widget is a good name. For starters right now it is possible to instantiate a non-widget class. I don't know if it is a good idea or not, but this is the current state of affairs.

comment:13 Changed 7 years ago by bill

dojo-data-class would be the obvious choice since (as you said) classes like ItemFileReadStore aren't technically widgets, depending on how you define widget. But that conflicts with widgets' class attribute.

I suppose you could call it data-dojo-constructor. That would satisfy the HTML5 zealots (who demand use of data-dojo-... attribute names), the JS language purists (who object to the word "class"), and people objecting to the word "widget".

comment:14 Changed 7 years ago by bill

PS: my comments above were about when every widget attribute is specified as a separate DOMNode attribute. If all the widget attributes are specified as a single data-dojo-props DOMNode attribute, then naming is much easier.

I'd suggest that the parser has a switch to support either old style:

<div dojoType=foo prop1=1, prop2=2>

or new style:

<div data-dojo-type="bar", data-dojo-props="prop1: 1, prop2: 2">

but doesn't support a mixture of the two.

The new style has performance benefits because the parser doesn't need to loop through every widget parameter checking if the DOMNode defines it, and it doesn't even need to generate metadata about each widget (see getClassInfo()).

However, I can see a downside to the new format for widget templates. Assume someone has a widget template (using widgetsInTemplate) like:

<div>
       <div dojoType=dijit.form.Button label=${myLabel} iconClass=${myIconClass}></div>
        ...
</div>

DTL, and in the future dijit's parser, will know that Button.label is bound to mainWidget.myLabel, so that mainWidget.set("myLabel", ...) will call Button.set("label", ...) automatically. That seems harder to do with the new syntax.

comment:15 Changed 7 years ago by dante

I'm concerned about not supporting new and old styles simultaneously. The current patch is doing the simultaneous support thing now with the dojoType/data-dojoType in place. I agree this isn't right, and should be data-dojo-ctor or something similar. by _not_ supporting native attributes as pass-throughs to widget attrs we lose degradability. Attributes like href, class, style, and other standard/valid attributes need mixed in, but also to be left alone in the case of no javascript. eg, LinkPane:

<div data-dojo-type="dijit.layout.TabContainer">
   <a data-dojo-type="dijit.layout.LinkPane" href="foo.html">Tab One</a>
</div>

works w/o JS but only because the selection of a native anchor and href="" attribute. Putting href:"" as a data-dojo-props defeats the purpose of providing progressive UI widgets over degradable markup. the point of data-dojo-* is to allow for our invalid attributes to gain validity.

I am not opposed to having a single attribute data-dojo-props that is fromJson'd and then mixed into the expected attributes (as opposed to the current patch, which takes data-dojo-* and mixes those as individual properties into the instance to be created), leaving the burden of validation on the developer. I understand that we gain no performance for using only a single attribute node, but for backwards compatibility reasons and for the reasons stated above, I don't believe we should force a user to commit to "one or the other".

comment:16 Changed 7 years ago by bill

There's no backwards-compatibility issue since everything that worked in 1.5 will work going forwards, at least until 2.0. Namely, the parser will still handle the old format of dojoType and attribute names fine, unless you set a "use new format" flag.

As for degradability, it's still possible to support, by repeating attribute names:

<div data-dojo-type="dijit.layout.TabContainer">
   <a data-dojo-type="dijit.layout.LinkPane" href="foo.html"
        data-dojo-props="href: foo.html">Tab One</a>
</div>

... or by using the old format:

<div data-dojo-type="dijit.layout.TabContainer">
   <a dojoType="dijit.layout.LinkPane" href="foo.html">Tab One</a>
</div>

That does add markup bloat, but I'm just not sure that degradability is worth thinking about, I doubt many people are depending on it.

comment:17 Changed 7 years ago by jburke

What about just supporting one data attribute, and putting type inside it? as in data-dojo="type: dijit.layout.LinkPane, href: foo.html", and as dante mentions those values get mixed in with the other expected regular HTML attributes?

The difference is putting the dojoType inside the data-dojo attribute. Maybe we need to use dojoType: instead of type: (type can be an attribute on input values), but the main thing is encapsulating all html5-conforming stuff into one attribute, and it avoids the issues with dojo-dojo-type. It also means we can be lighter in our code, we do not need to normalize browsers for dataset.

It means the parser query has to look for "[dojoType],[data-dojo-scopeName]", but I think it is safe to say that any attribute with a data-dojo value is meant to be parsed and will likely have a dojoType inside of it.

comment:18 follow-up: Changed 7 years ago by jburke

Forgot to put this in: I also do not think we should just fromJson the data-dojo attribute, well there are issues:

1) We should only use a strict JSON decoder. 2) The nesting syntax in HTML is *ugly*: dojo-data="\"dojoType\": \"something\"" I actually do not think that will work. Then you have to do something like dojo-data='"dojoType": "something"', but using single quotes for attributes I thought was not valid. At least it offends my delicate sensibilities.

Not sure what to do there yet for it, and it is a secondary point after working out the attribute names, but something that needs more thought.

I like using JSON because we would not have to type-guess like today's parser (or type match against properties on the widget), but just not sure how to embed the double quotes for JSON inside a double-quoted attribute.

comment:19 Changed 7 years ago by elazutkin

Replying to jburke:

What about just supporting one data attribute, and putting type inside it?

That's basically what I advocated above. I just think that data-dojo-widget is a bad name for a property bag.

comment:20 in reply to: ↑ 18 Changed 7 years ago by elazutkin

Replying to jburke:

1) We should only use a strict JSON decoder.

What's wrong with eval-ing it? It is your code => it doesn't open a new surface for an attack, just don't put nonsensical things there.

comment:21 Changed 7 years ago by bill

I like using JSON because we would not have to type-guess like today's parser (or type match against properties on the widget),

Right, assuming that all properties are defined in the property bag. If we still support standard parameters like value="2009-10-10" (for a DateTextBox), then we still need the type conversion code.

Removing all the type conversion code from the parser is >100 lines savings, which is important for mobile.

but just not sure how to embed the double quotes for JSON inside a double-quoted attribute.

Here's how some current code looks with complex objects passed to the parser:

<div dojoType="dijit.Editor" id="br"
	plugins="[{name:'dijit._editor.plugins.EnterKeyHandling', blockNodeForEnter:'BR'}, 'viewsource']"

Changed 7 years ago by dante

updated patch.

Changed 7 years ago by dante

comment:22 Changed 7 years ago by dante

-3 patch includes:

  • dojo.parser support for data-dojo-type / data-dojo-props="somejson"
  • dojo.js support for dojoConfig or script element data-dojo-config="json"
  • maybe 50% themetester porting. some issues have shown up. ProgressBar indeterminate state from mixin might be wrong, oddness with title=""/style=""
  • hitting the node attr lookup branch when no data-dojo-props specified, probably causing side effects

just taking a snapshot before I depart for the evening, code seems completeish but want feedback.

Changed 7 years ago by dante

themetester validates. patch includes fix for select to support data-dojo-value for nonstandard children and avoids getObject for inlineeditor if passed a reference.

Changed 7 years ago by dante

-4 has a small bug in it. d in undefined.

Changed 7 years ago by dante

keeps everything working. could be faster, still need to steal some args from node for the time being. also widgetsInTemplate is causing me pain.

comment:24 follow-up: Changed 7 years ago by jburke

elazutkin: sorry, I mostly skimmed the previous replies. I agree data-dojo-widget is not the best name choice. I like using data-dojo (specifically data-[scopeName]), then reusing the same attribute for djConfig extraction on a script tag. Stuff any JSON data in it to pass to the target widget/djConfig, including dojoType and jsId.

I want to avoid using eval if we can. It will be harder to make a case for supporting eval over time, particularly in "use strict" mode. If we can leverage the same syntax in a 2.0, where I expect we will not be using eval at all, all the better. The upside for eval is a much simpler syntax to use vs. the JSON double quoting inside a double-quoted attribute.

Maybe we can just do a bulk conversion of single quotes to double quotes for the data- value, but only for single quote is not preceded with a backslash, then treat it as JSON, pass it to JSON.parse(). That might be simplest, be somewhat easy to use inside double-quoted attributes and avoid eval (for platforms that have native JSON parsing).

bill: agreed on the type conversion code, needs to stay in for legacy support/treating existing attributes as properties on the widget.

dante: great work on pushing this through. How do you feel about supporting just one attribute, something like a data-dojo that would include things like jsId and dojoType as JSON properties inside the data-dojo value?

comment:25 in reply to: ↑ 24 Changed 7 years ago by elazutkin

Replying to jburke:

I want to avoid using eval if we can. It will be harder to make a case for supporting eval over time, particularly in "use strict" mode. If we can leverage the same syntax in a 2.0, where I expect we will not be using eval at all, all the better. The upside for eval is a much simpler syntax to use vs. the JSON double quoting inside a double-quoted attribute.

Care to explain why you don't want to use eval()? I understand that you have the same negative feelings about dynamically defined functions too, right? Why???

comment:26 follow-up: Changed 7 years ago by jburke

elazutkin: eval (and with) are not allowed in "use strict" mode, and I prefer code that will be able to run in strict mode later. It is not something we can get with existing Dojo 1.x code, but for building in features now that we want to transfer to 2.0, we should avoid building in syntax that will need to change in a 2.0 world. Might as well make the new 1.x features/syntax future compatible where possible.

Dylan also gave feedback that Dojo was not adopted in some projects because it used eval. eval has its place as a tool, but enough of the world has been taught it is evil that we should avoid using it going forward. Similar to why we are building in HTML5 validation support -- validation does not buy much, but it hurts our story by using "invalid" markup.

comment:27 in reply to: ↑ 26 Changed 7 years ago by elazutkin

Replying to jburke:

elazutkin: eval (and with) are not allowed in "use strict" mode, and I prefer code that will be able to run in strict mode later. It is not something we can get with existing Dojo 1.x code, but for building in features now that we want to transfer to 2.0, we should avoid building in syntax that will need to change in a 2.0 world. Might as well make the new 1.x features/syntax future compatible where possible.

Sorry, but it is just plain wrong. According to ECMA-262 p. 233 "The Strict Mode of ECMAScript":

Strict mode eval code cannot instantiate variables or functions in the variable environment of the caller to eval. Instead, a new variable environment is created and that environment is used for declaration binding instantiation for the eval code (10.4.2).

JavaScript is a dynamic language and removing eval (and equivalent operations like dynamically creating functions) is ... strange at the very least.

If you have credible sources that 2.0 will go static, please share.

Most probable you meant the Adobe's version of ActionScript, which is a compiled language. They submitted it to TC39 several years ago hoping to use as a foundation for the upcoming standard. It was roundly rejected.

Dylan also gave feedback that Dojo was not adopted in some projects because it used eval. eval has its place as a tool, but enough of the world has been taught it is evil that we should avoid using it going forward. Similar to why we are building in HTML5 validation support -- validation does not buy much, but it hurts our story by using "invalid" markup.

That's why there are the restrictions imposed by the strict mode. Still I do not see how we can work without creating functions dynamically or even evaling stuff.

Users who do not want to use JavaScript I usually recommend Java applets --- fast, secure, and utterly non-JavaScript. It is a complaint from the same bin along with "full XHTML compliance", "DTD verification", and "a one-page web app should degrade gracefully when JavaScript is not present" --- unpractical ideas.

comment:28 follow-up: Changed 7 years ago by jburke

elazutkin: thanks for the clarification on the spec, you are correct. I got the eval restrictions and "no with" mixed in my head (and my usage of JSLint) as meaning eval was not allowed.

That said, there are some secure environments that do not allow eval, and it ends up in a security conversation when it shows up. It is a reasonable discussion to see if we want to support those secure environments (as I recall some stricter ones do not even allow "this"), but even if we do not want to support those environments, there is an impact to using eval, it ends up being a security discussion point.

People have allergic reactions to eval(), just like folks like to have their HTML validate, no matter how reasonable those reactions/desires might be. And for Dojo base, if we did not care about backwards compatibility, we could get by without it with a new loader, and using JSON parsing/stringifying (which includes dojo.parser usage).

I appreciate some of your neater code relies on eval/Function() but it is not clear to me that those capabilities should be part of a 2.0 base/core. It just makes it harder to sell it to folks, and the extra security discussions it brings up.

comment:29 Changed 7 years ago by elazutkin

Security implications of eval are simple --- there are none in a browser environment: user can access the code, and using any debugging tool can do much more harm than with eval. And I don't see much troubles in a local app environment either. But it can have some bad implications on a server. If we are to position Dojo as a !CommonJS-based toolkit suitable for SSJS, we have to take care that no user-submitted code ends up directly in eval. OTOH !CommonJS loaders usually rely on eval, so the whole SSJS-related scare is a moot point anyway.

If you wanted to talk about some other security environments, please share your thought and relevant links.

Being unfamiliar with the "allergic reaction" to eval, I hope you'll give me links for that too. Actively consulting and talking to many companies I suspect I've heard it all, and the topic of eval was never even mentioned. It was never ever a part of "selling Dojo". Obviously if you have concrete cases when eval was a problem for potential users --- I am mighty curious.

comment:30 in reply to: ↑ 28 Changed 7 years ago by elazutkin

Replying to jburke:

I appreciate some of your neater code relies on eval/Function() but it is not clear to me that those capabilities should be part of a 2.0 base/core. It just makes it harder to sell it to folks, and the extra security discussions it brings up.

Now you are outed yourself --- you are clearly thought about 2.0 base! :-) Please share your thoughts of what should be/should not be in the base and why. Probably the wave we have is the most appropriate place for that.

comment:31 follow-up: Changed 7 years ago by markwubben

Wouldn't eval parsing allow for exploits like these:

<div data-dojo-props="description: '(function(){ alert(1) })()'">

Escaping for HTML insertion is hard enough, but to deal with the values being treated as JavaScript as well…

comment:32 in reply to: ↑ 31 ; follow-up: Changed 7 years ago by dante

Replying to markwubben:

Wouldn't eval parsing allow for exploits like these:

<div data-dojo-props="description: '(function(){ alert(1) })()'">

Escaping for HTML insertion is hard enough, but to deal with the values being treated as JavaScript as well…

Yes. But not more than

<div dojoType='Something' prototypeMememberName="return (function(){ alert(1); })()">

We already create and execute functions from attributes. This just puts them all in one spot. Widget attributes are your data. If they required user parsed input, grab the from a server who has done the due diligence to remove XSS exploits from text that will be injected into HTML.

regarding a "single attribute" I disagree with "cramming everything humanly possible in 'data-dojo'". The html5 spec says we should prefix all library like data members with 'data-dojo-*". I don't like the idea of overloading the one attribute. By naming the attributes we avoid the cost of sniffing a potentially overloaded data-dojo attribute for things like type or was this on _the_ script tag or otherwise inferring intent. data-dojo-type, data-dojo-props, data-dojo-config are all very clear in what they are and contain.

comment:33 Changed 7 years ago by dante

FWIW, the original version of this ticket didn't use eval. Putting the props on one node was meant as a way to avoid having to do the prototype lookup for members, so only memebers in the class were read off the node. Eval here has the 'side effect' of allowing global references by name, eg: props="editor: dijit.form.TextArea" rather than typeof editor == string ? getObject(editor) : editor, eg: going through the getObject branch every time.

I had it as data-dojo-*, but that conflicts with existing members, eg: class. data-dojo-class="...." class="..." or worse:

<input type="text" data-dojo-type="foo.bar.Baz">

Also would have loved more feedback prior to patch 4, 5, and 6.

comment:34 in reply to: ↑ 32 Changed 7 years ago by markwubben

We already create and execute functions from attributes. This just puts them all in one spot. Widget attributes are your data. If they required user parsed input, grab the from a server who has done the due diligence to remove XSS exploits from text that will be injected into HTML.

But with the attribute parsing code, converting to strings, booleans, numbers etc doesn't use eval right? When do we execute functions from attributes?

comment:35 follow-up: Changed 7 years ago by jburke

markwubben: in dojo/parser.js, around around line 38 is some Function() work and line 67, attributes that are determined to have "object" values are passed to dojo.fromJson that right now uses eval.

elazutkin: I do not have specific links to people have problems with eval, but Dylan has shared before that the use of eval was given as a reason that Dojo was not used on a project. Not sure if it was the only reason. But still I strongly prefer not to use eval. That said, if it gets the patch done, the existing eval behavior can be used. Just suggesting for new things if we see a clear path for new features to fit in with a 2.0 thing, then we should favor that. Unfortunately 2.0 is not that clear, but I will be encouraging the avoidance of eval for base/core.

As I mentioned before patch 2, I prefer just the one data-dojo attribute instead of the extra typing for multiple data-dojo- attributes, but it is not a deal-breaker for me. Others seem fine with it.

comment:36 follow-up: Changed 7 years ago by bill

FYI, in this case using eval or the more politically correct dojo.fromJson() is basically the same. There are three differences:

  1. JSON's official definition apparently demands double quotes around all attribute names, making the syntax cumbersome to use inside of HTML
  2. JSON doesn't have good support for dates, and currently DateTextBox, TimeTextBox, and !Calendar use Date objects for their value. But I was talking to Adam and he thinks that for 2.0 it makes sense for those widgets to take a string parameter (ex: "2008-10-20") instead.
  3. Can't embed functions, ex <div data-dojo-type="..." data-dojo-params="onClick: function(){ ...}">. However, that can be worked around by using <script type="data-dojo-method"> tags, albeit those are much wordier than a simple onClick=myOnClick flag.

comment:37 in reply to: ↑ 36 ; follow-up: Changed 7 years ago by elazutkin

Replying to bill:

  1. Can't embed functions, ex <div data-dojo-type="..." data-dojo-params="onClick: function(){ ...}">. However, that can be worked around by using <script type="data-dojo-method"> tags, albeit those are much wordier than a simple onClick=myOnClick flag.

Most probably we can leave <script type="dojo/method">...</script> as is --- no new attributes are used, but we should replace event and args with data-dojo-* counterparts, or even cram them into one property.

BTW, what is the strategy to work with such script additions without eval?

comment:38 Changed 7 years ago by bill

<script type="dojo/method"> tags are currently evaluated via new Function(preamble+script.innerHTML+suffix), so I don't see a problem going forward.

comment:39 Changed 7 years ago by elazutkin

This is eval "the other side" too. If we allow dynamic functions, we can allow eval too --- it is one and the same for all valid considerations.

// pseudo-code to calculate expressions
function eval1(expr){
  return (new Function("return (" + expr + ")"))();
}

// pseudo-code to execute arbitrary statements
function eval2(expr){
  (new Function(expr))();
}

Less efficient, more cumbersome, yet the same.

comment:40 in reply to: ↑ 35 Changed 7 years ago by markwubben

Replying to jburke:

markwubben: in dojo/parser.js, around around line 38 is some Function() work and line 67, attributes that are determined to have "object" values are passed to dojo.fromJson that right now uses eval.

Right, so specifying properties as null, empty object or as a function leads to code execution. Those kind of values typically don't take user-generated input though, so should be fine. Still, it's a subtle security risk.

comment:41 in reply to: ↑ 37 Changed 7 years ago by dante

Replying to elazutkin:

Most probably we can leave <script type="dojo/method">...</script> as is --- no new attributes are used, but we should replace event and args with data-dojo-* counterparts, or even cram them into one property.

BTW, what is the strategy to work with such script additions without eval?

I've already added data-dojo-event for dojo/method and dojo/event scripts (will need to add unit tests for those) as the validation reports event="" as a deprecated attr. Need to add args="" support to use data-dojo-args.

So what the current patch is: 100% backwards compatible dojoType parser. Few extra cycles to look for (hopefully) a data-dojo-type. The presence of that attribute triggers the 'fastpath', which is not 100% working at the moment. I suggest I check in what I've got and we iterate from there. we have until 1.6 to perfect it, or scrap it entirely.

Things not working:

Date-based members we being cast to date objects by the parser. previously string values were cast using fromISOStrig. I've added a check in one of the date widgets (Calendar) to check typeof == string. Other places I added typeof == string checks around thing intended to be constructors to go through getObject or work from direct reference. Thinking of it, only date based widgets are changed by this, and that could be solved by putting a "real" date object in the params. Bill mentioned in 2.0 allowing string date values, and doing the work to convert there.

Gist is: dojo parts of the parser work seems 99% done. There is a lot of outstanding issues in how dijit will play with the new props on things that were cast, and what the implications of change means to downstream code.

Also haven't solved any widgetsInTemplate issues. My first attempt to 'just replace internal dojoTypes with data-dojo-type' for widgetsInTemplate fell apart. seemed attachpoints or something else internally was failing. The parser seems to carry around a dead _query member which _Templated changes to force "[dojoType]", but that query isn't used, and the data-dojo-type detection is unchanged.

themeTester is 99% working again. I put in a few lines of shim to keep things working for title, value, disabled and selected attributes to pass through if they exist on the .prototype, so they are read not from the data-dojo-props attribute, but from their native/degradable attributes.

comment:42 Changed 7 years ago by dante

Also I mentioned not using eval initially: uhop linked to the html5 spec for dataset, and turns out a couple of browsers implement this. I was able to make a shim for inferior browsers and had initially intended to use that 'native' data-* mapping to just do each invalid prop as it's own data-dojo-*

You can see my finished dataset() getter/setter function in the fd branch: http://svn.dojotoolkit.org/src/branches/dojo-fd/dojo/dataset.js

It is using the new has() API i'm proposing for base.

Changed 7 years ago by dante

current snapshot for bill

comment:43 Changed 7 years ago by bill

(In [22676]) comment some code to be removed in 2.0, refs #11490 !strict

Changed 7 years ago by dante

parser patch only.

comment:44 Changed 7 years ago by dante

(In [22696]) refs #11490 - significant changes to the parser. deprecates dojoType without warning. Also deprecates all nonstandard attributes when using dojoType replacement: data-dojo-type. details:

  • makes parser test use html5 doctype
  • unit tests added for string, obj, number, and functional props="" inherits
  • jsId deprecated, use dojo-data-id old jsId behavior.
  • dojoType deprecates, use data-dojo-type instead.
  • all invalid attributes deprecated. All widget properties from declarative markup are shoved on ONE valid data-dojo-props attribute. (note: you must duplicate standard attributes for degradability, like type, name, disabled, selected, an so onergs)
  • wrt to script type='dojo/method' and connect: the nonstandard event="" and args="" attributes have been deprecated. use data-dojo-event and data-dojo-args.
  • removes unused parser._query value. removed from _Templated, as it no longer uses query()
  • move proto lookup code into own function to assist in fast path from getClassInfo
  • avoids calling proto lookup code when using data-dojo-type (but still might have to for other instances of that class using dojoTYpe, eg: in a template)
  • data-dojo-props is try{}'d, but does NOT fallback to native attributes if catch(ing) an exception. A warning is thrown with the srcNode and error for easy debugging.
  • added relevant inline docs
  • makes themeTester.html validate html5 (using above attributes), also making it the only test page that does not have deprecated code usage :) (note: there may be missed value/disabled/selected translations in some widgets. look at srcNodeRef before filing a bug, probably just needs duplicated in props="")
  • added a note to address data-dojo-value in SelectOption with regard to selected and disabled
  • Made _DateTimeTextBox accept a string (as it is cumbersome to create a new date object in a string props="" attribute) for .set('value', n)
  • Made InlineEditBox accept object literals for editor and editorWrapper, as it is much more clean to specify objects by reference in props=""

TODO:

  • figure out what to do with widgetsInTemplate (attachPoint, attachEvent)
  • find other instances that accept date objects and make them accept a string
  • update LOTS of documentation.

probably \!strict

comment:45 Changed 7 years ago by blowery

(In [22697]) Merged revisions 22696 via svnmerge from https://svn.dojotoolkit.org/src/dojo/trunk

........

r22696 | dante | 2010-08-07 08:26:26 -0400 (Sat, 07 Aug 2010) | 38 lines

refs #11490 - significant changes to the parser. deprecates dojoType without warning. Also deprecates all nonstandard attributes when using dojoType replacement: data-dojo-type. details:

  • makes parser test use html5 doctype
  • unit tests added for string, obj, number, and functional props="" inherits
  • jsId deprecated, use dojo-data-id old jsId behavior.
  • dojoType deprecates, use data-dojo-type instead.
  • all invalid attributes deprecated. All widget properties from declarative markup are shoved on ONE valid data-dojo-props attribute. (note: you must duplicate standard attributes for degradability, like type, name, disabled, selected, an so onergs)
  • wrt to script type='dojo/method' and connect: the nonstandard event="" and args="" attributes have been deprecated. use data-dojo-event and data-dojo-args.
  • removes unused parser._query value. removed from _Templated, as it no longer uses query()
  • move proto lookup code into own function to assist in fast path from getClassInfo
  • avoids calling proto lookup code when using data-dojo-type (but still might have to for other instances of that class using dojoTYpe, eg: in a template)
  • data-dojo-props is try{}'d, but does NOT fallback to native attributes if catch(ing) an exception. A warning is thrown with the srcNode and error for easy debugging.
  • added relevant inline docs
  • makes themeTester.html validate html5 (using above attributes), also making it the only test page that does not have deprecated code usage :) (note: there may be missed value/disabled/selected translations in some widgets. look at srcNodeRef before filing a bug, probably just needs duplicated in props="")
  • added a note to address data-dojo-value in SelectOption with regard to selected and disabled
  • Made _DateTimeTextBox accept a string (as it is cumbersome to create a new date object in a string props="" attribute) for .set('value', n)
  • Made InlineEditBox accept object literals for editor and editorWrapper, as it is much more clean to specify objects by reference in props=""

TODO:

  • figure out what to do with widgetsInTemplate (attachPoint, attachEvent)
  • find other instances that accept date objects and make them accept a string
  • update LOTS of documentation.

probably \!strict

........

comment:46 Changed 7 years ago by dante

(In [22698]) refs #11490 - putting in an original copy of the themeTester ... should have copied from before commit for history, but forgot

comment:47 Changed 7 years ago by blowery

(In [22699]) Add a meta tag to tell HTML validators which content type this document uses, even if they ignore HTTP headers. Removes warning on html5.validator.nu. refs #11490.

comment:48 Changed 7 years ago by dante

(In [22700]) refs #11490 - forgot to save last whitespace and prop change. also blowery's whitespace

comment:49 Changed 7 years ago by chrism

A sed script that converts from old to new markup would be extremely helpful here for the significant # of pages that now exist in the older formats.

comment:50 Changed 7 years ago by dante

I had considered that, though am illequipped to undertake the task myself. It also isn't entirely straightforward, as some properties are intentionally left on the domNodes for degradability. converting dojoType and djConfig references are more straightforwards, but the props="" attribute requires additional attention.

comment:51 Changed 7 years ago by bill

(In [22775]) Note for 2.0, refs #11490 !strict.

comment:52 Changed 7 years ago by bill

(In [22810]) Spacing fixes, refs #11490 !strict.

comment:53 Changed 7 years ago by bill

(In [22811]) just fixing some comments, refs #11490 !strict.

comment:54 Changed 7 years ago by bill

(In [22814]) Support for HTML5 syntax in templates:

  • data-dojo-type, data-dojo-props (for widgetsInTemplate)
  • data-dojo-attach-point (superseding dojoAttachPoint)
  • data-dojo-attach-event (superseding dojoAttachEvent)

Refs #11490 !strict.

comment:55 Changed 7 years ago by bill

(In [22927]) ContentPane needs to be aware of new data-dojo-type syntax too. Refs #11490 !strict.

comment:56 Changed 7 years ago by bill

(In [23251]) Multi-version support related fixes:

  • Add scope parameter to parser that controls which attribute names it searches for (dojoType vs. dojo16Type, data-dojo-props vs. data-dojo16-props, etc.) For back-compat defaults to dojo._scopeName although for 2.0 it should default to "dojo".
  • Fix bug where templates with data-dojo-type=... didn't work in multi-version pages. attrData was a true private variable and thus the attempts to modify it from outside the parser had no effect.
  • Change dojo.html to make parser search for dojoType / data-dojo-type even when multi-version support has renamed dojo to something else. Same as _Templated was already doing.
  • Update InlineEditBox template to parser 2.0 format.

Since multi-version support requires a build there are no tests checked in for this, but I did attach a test case to #11499.

Refs #11490, fixes #11499 !strict.

comment:57 Changed 7 years ago by peller

(In [23371]) remove global reference, fix some typos. Refs #11490

comment:58 Changed 7 years ago by bill

(In [23381]) rollback [23371], it makes themeTester.html fail on load (with an exception in firebug), refs #11490

comment:59 Changed 7 years ago by peller

(In [23393]) Redo of [23371] Remove global reference, fix some typos, declare global function in closure. Refs #11490

comment:60 Changed 7 years ago by unscriptable

Hey guys,

I understand the need for performance, but this all-or-nothing (a.k.a. "fastpath") approach is just not an option. Here's why:

1) As dante points out, graceful degradation is now clunky, at best.

<input type="password" name="pwd" class="myCssClass" data-dojo-type="dijit.form.ValidationTextBox" data-dojo-props="class:'myCssClass', type:'password', name:'pwd'"/>
<button class="myCssClass" type="submit" data-dojo-type="my.ButtonCtor" data-dojo-props="class:'myCssClass', type:'submit'">my button label</button>

2) We can no longer simply add a class attribute onto an element to override dijit styling.

For instance, in the example above, the class attr needs to be added to data-dojo-props because it won't get transferred from the widget's srcNode to its domNode anymore.

3) It's not just the class attr.

An [incomplete] list of common native attributes that we were once able to use freely but now must be duplicated in data-dojo-props:

type="submit"

type="password"

class="myClass"

name="formElementName"

alt="alternate text"

title="additional info"

Some of these attrs are required by the HTML validators. Therefore, we are forced to duplicate them as both native and data-dojo-props if we want to validate.

Having to duplicate these common native attrs in the data-dojo-props attr is a source of frustration and a very real source of maintenance bugs.

4) "HTML5 Validation" isn't the actual goal: "HTML5" is.

While it's true that some people just want HTML5 Validation, the real HTML5 advocates (myself included) are going to puke when they learn that they have to duplicate native attrs in the data-dojo-props attr. The goal should be to make using dojo feel like an extension of using HTML(5). This is not the feeling I get. This was a total WTF for me and the other advanced developers on my team.

The fact that we've broken ALL NATIVE ATTRIBUTES, makes this feature absolutely useless, imho.

-- J

comment:61 Changed 7 years ago by unscriptable

Hey! How about this?

data-dojo-config="Html5FastPath: true"

When the "Html5FastPath" dojo.config parameter is turned on, the fastpath is taken in the parser. Otherwise, both the native and the data-dojo-props are parsed.

If my vote counts for anything, the default should be false or we're just going to make it even harder for noobs to learn dojo.

Also, the data-dojo-props properties should override/supersede the native attrs.

-- J

comment:62 Changed 7 years ago by SubtleGradient

I recommend using CSS syntax inside of data-dojo-config so that there are only two syntaxes you have to remember in the browser instead of three.

e.g. <div style="color:blue; border:1px solid blue" data-dojo-config="foo:bar; baz:banjo bob">

I happen to know of a pretty good CSS parser named Sheet.js that was created by someone who has already signed a CLA and would totally be glad to contribute it to the Dojo Foundation. https://github.com/subtleGradient/Sheet.js TDD FTW

comment:63 Changed 7 years ago by SubtleGradient

Re: The issue where the dojoType masks the type attribute e.g. <div data-dojo-type="foo.Bar" type="awesome">

Simply ditch the -type in dojo-data-type e.g. <div data-dojo="foo.Bar" type="awesome">

dojoType=Foo makes great sense in 'invalid' HTML.

data-dojo=Foo is best (imho) in HTML5

comment:64 Changed 7 years ago by bill

@unscriptable - I understand your concern.

Note that it wasn't done this way just for performance. One issue is consistency, that if we only read some of the native attributes from the DOMNode then users might get confused why

<button data-dojo-type="dijit.form.Button" type=submit iconClass=save>

only half works (i.e. why iconClass is silently ignored by the parser).

There are other issues such as that on IE the parser can't differentiate between <button> and <button type=submit>. It always seems like type=submit is specified, even when it isn't.

There's are also times when the app wants to specify an attribute to the widget without it appearing as a native HTML attribute, such as:

<div dojoType=!ContentPane title=hello>

In this case "title" is used as an argument to the parent TabContainer, but we don't want it to appear as a tooltip when the mouse is hovered over the ContentPane itself. Of course apps could avoid that problem by (in this case) specifying the title in data-dojo-props rather than as a native attribute.

@SubtleGradient - There's no issue where dojoType/data-dojo-type masks the type attribute. dojoType/data-dojo-type and type are essentially unrelated.

As for "CSS syntax", that would simplify the syntaxes apps needed to use in markup, but it's limited. For example:

data-dojo-props="hash: {a: 3, b:4},  ary: [1,2,3],
    func: function(x, y){ return x+y;}"

That can't be represented in CSS syntax.

CSS syntax would also slow down the parser and add code bloat.

comment:65 follow-up: Changed 7 years ago by bill

FYI, I checked with Doug, the other problematic attribute name is "value". On IE6, when you inspect a node with no value specified (ex: <input>), it seems like value is specified as "". That's problematic especially for CheckBox, because the CheckBox's default value is "on" and it can get "overridden" to "" in this case. CheckBox.js has code to workaround that.

The parser also has code so that value="" is treated as NaN for NumberTextBox, and new Date("") for DateTextBox.

@unscriptable - Were you suggesting to have the "slow path" processing only for those special attribute names like type, class, value, style, etc.? Or for all possible attributes?

comment:66 Changed 7 years ago by bill

(In [23585]) Benchmark for data-dojo-type parse syntax too. Is there an easier way to find out which radio button is selected? Refs #11490.

comment:67 in reply to: ↑ 65 Changed 7 years ago by unscriptable

Replying to bill:

@unscriptable - Were you suggesting to have the "slow path" processing only for those special attribute names like type, class, value, style, etc.? Or for all possible attributes?

@bill: I think that most people would expect that the standard attributes ( type, class, value, style, etc.) would be supported as normal attributes and that the custom attributes would be in data-dojo-props. If this is the "slow path", then this would be great, imho.

<span class="myClassAttr" data-dojo-type="myWidget" data-dojo-props="myBool:true"></span>

comment:68 Changed 7 years ago by dante

  • Resolution set to fixed
  • Status changed from assigned to closed

in the 'degradable' sense, natives attrs do work. in the interest of performance we opted to go with "one or the other" (the old behavior is still 100% compat). reverting/changing any of this work I spent months discussing here and on dojo-contrib at the last hour before 1.6 can't happen.

the idea is this is an optional incremental addition to support html5 validation. we can workout the actual implementation in 2.0 when we are able to break back compat some.

comment:69 Changed 7 years ago by bill

(In [23714]) add TODO comment about new parser syntax and dir/lang inheritance, refs #11490 !strict

comment:70 Changed 7 years ago by bill

(In [23720]) fix style attribute for Dialog, so it doesn't keep expanding horizontally, refs #11490.

comment:71 Changed 7 years ago by bill

(In [23867]) fix specification of disabled for TextArea widgets, refs #11490

comment:72 Changed 6 years ago by bill

(In [24185]) Refactor parser to allow attributes (for a single node) to be partly specified in data-dojo-props, and partly specified directly ex: value=123. Uses node.attributes to detect which attributes are specified on a node, or for older versions of IE calls cloneNode(false) followed by some regex's on clone.outerHTML.

Due to lowercase/uppercase issues (ex: tabIndex, onClick), and for type conversion, the code still introspects each widget to get it's attribute metadata. In the future, would like to defer/avoid that in the common case. Fixes #10153, #12423, #12476, #10150, #9823, refs #11490 !strict.

Also fixes the problem on IE8 where a button without type=... defaults to type=submit rather than whatever the widget defines the default as, fixes #10163, refs #9334, #8946.

Future updates will be attached to ticket #12476.

comment:14 Changed 6 years ago by bill

In [26315]:

Update API examples to use data-dojo-type rather than dojoType. Also updated to use data-dojo-props when necessary (i.e. for non-standard attribute names). Refs #11490 !strict.

comment:15 Changed 6 years ago by bill

In [26316]:

Fix typo from [26315], refs #11490 !strict.

comment:16 Changed 6 years ago by bill

In [26321]:

upgrade to use data-dojo-type syntax, refs #11490

comment:17 Changed 6 years ago by bill

In [26324]:

Use data-dojo-type / data-dojo-props in dijit templates (with widgets in the templates), refs #11490, #4925 !strict.

comment:11 Changed 6 years ago by bill

In [26329]:

modify test not to depend on deprecated dojoAttachPoint parameter in the Tooltip template, refs #11490

comment:12 Changed 6 years ago by bill

In [26331]:

Use data-dojo-attach-point / data-dojo-attach-event in dijit templates, refs #11490, #4925 !strict.

comment:13 Changed 6 years ago by bill

In [26352]:

Support of data-dojo-attach-event / data-dojo-attach-point (in addition to dojoAttachEvent/dojoAttachPoint) for root node of Declaration, refs #11490 !strict.

comment:14 Changed 6 years ago by bill

In [27067]:

fix missing dependency for dijit.form.Select, refs #11490

Note: See TracTickets for help on using tickets.