#8991 closed defect (fixed)
dojo.attr() doesn't return current value of a text input field
Reported by: | roman2 | Owned by: | Eugene Lazutkin |
---|---|---|---|
Priority: | high | Milestone: | 1.4 |
Component: | Documentation | Version: | 1.3.0rc2 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
Assuming inputNode is a node of type text input, dojo.attr(inputNode, "value") doesn't return current value of the field (i.e. what is entered into the field). However, inputNode.value does.
I've tested this on Firefox 3.0.7.
Change History (43)
comment:1 Changed 13 years ago by
Component: | General → HTML |
---|---|
Owner: | changed from anonymous to sjmiles |
comment:2 Changed 13 years ago by
Milestone: | tbd → 1.4 |
---|---|
Owner: | changed from sjmiles to Eugene Lazutkin |
This belongs in the "should attr be a general property and attribute getter/setter". There is a good case for it, but traditionally, with the exception of a limited set of properties it has not been. The current property whitelist approach will need to change.
Assigning to Eugene for now, but just as a placeholder (no pressure Eugene :)
comment:3 Changed 13 years ago by
Until this is changed, I'd ask that the documentation be modified to reflect the specialness of some attributes. Currently (http://api.dojotoolkit.org/jsdoc/dojo/HEAD/dojo.attr), it says this: "Handles normalized getting and setting of attributes on DOM Nodes." This leads me to believe that it's been designed to handle ALL attributes. The documentation change should probably apply to attr on NodeList? too.
Thanks
comment:4 Changed 13 years ago by
Status: | new → assigned |
---|
We need a general solution for that. Any ideas besides whitelisting "direct" attributes?
comment:5 Changed 13 years ago by
I'm in favor of actually making it work, not sure if that counts as an idea or is just stating the obvious.
I am worried about this because I searched our code and we are unfortunately calling dojo.attr(foo, "value", ...) in many places. I'm suspicious there are hidden bugs there.
dijit/form/CheckBox.js: dojo.attr(this.focusNode, 'value', this.value); dijit/form/ComboBox.js: return dojo.attr(item, "value"); dojox/form/DropDownSelect.js: dojo.attr(this.valueNode, "value", this.attr("value")); dojox/form/FileUploader.js: dojo.attr(f, "value", this.postData[nm]); dojox/form/PasswordValidator.js: dojo.attr(this.focusNode, "value", v); dojox/form/Rating.js: dojo.attr(evt.target, "value"); (twice)
Plus there might be hidden usages like dojo.attr(node, attrName, attrValue);
inside DTL etc.... or usages where "dojo" has been abbreviated to "d" or some other variable name, although I tried to search for those too.
comment:6 Changed 13 years ago by
I was doing something like the following for jquery compat -- it is still a bit hacky/pseudocode, can probably be cleaned up:
Get:
var prop = node[propName]; if((propName in node) && !dojo.isObject(propName) && propName != "href"){ return prop; }else{ return [node.getAttribute stuff here] || prop; }
Set:
//The setter case. Figure out if value is a function. var prop = node[propName]; if((propName in node) && !dojo.isObject(prop) && name != "href"){ node[propName] = value; }else{ [do node.setAttribute work here = value]; }
We might still need to do the name normalization stuff for htmlFor, class, etc. In there too.
comment:8 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:9 Changed 13 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
[17931] breaks dijit/tests/_base/manager.html and possibly a bunch of other dijit tests (there are lots of tests failing in dijit now but I'm not sure when all the failures started; I only traced down this first error)
comment:10 Changed 13 years ago by
hasAttr() for "disabled" and "tabindex" are both broken, see #9430.
comment:11 Changed 13 years ago by
comment:12 Changed 13 years ago by
comment:15 follow-up: 35 Changed 13 years ago by
Brain dump
Every DOM node has a list of attributes (attribute nodes), which correspond to attributes in the HTML markup, and a list of properties, which are node-specific values available directly on the node.
Attributes are manipulated by getAttribute()
, setAttribute()
, and removeAttribute()
. If attribute is missing, null
is returned by getAttribute()
.
Alternative interface includes attributes
collection, and getAttributeNode()
. The latter returns an object with three properties:
name
--- read-only attribute name.specified
--- read-only Boolean flag.value
--- read/write attribute's value.
So any attribute can be missing, or present. In the latter case it has a value, which is a string of some sort. If an attribute was not specified but it has a default value per spec, it will be present with a value, but not specified.
Properties are always present, if they are allowed for a certain type of node. Their value type is defined by the DOM spec.
When a node is rendered, browsers takes into account both properties and attributes. It is possible to have a property and an attribute of the same name and for the same logical entity. According to the DOM spec properties are final authorities on the effective value of such entity.
Complications
It all sounds nice but different browsers exhibit different behavior:
- Some properties shadow attributes, and vice versa. It is quite possible that an attribute is missing, yet the corresponding property is readable returning a default value. And an assignment to such property can create the corresponding attribute too.
- Some corresponding properties and attributes may have totally different values. Example:
disabled
the property operates with Boolean values, whiledisabled
the attribute uses strings with values"disabled"
fortrue
and""
forfalse
. - It is possible to have an attribute node present, yet its
specified
property isfalse
. Example: for<input type="text">
IE produces an attribute withspecified === false
, and this value can be changed only once. - It is possible to have both property and attribute out of sync with different values. The worst offender is IE.
- Removing an attribute node in same cases leaves the corresponding property value unchanged --- needs more investigation.
- Some default values are different on different browsers. Example: non-existent
tabIndex
is 0 on IE, and -1 on other browsers (depending on a node type). According to the spec it cannot be negative. - Some properties and attributes are in use, yet there is no spec for them. Example: innerHTML.
Additional problem arises from the fact that browsers can be lax with enforcing upper/lower case for attribute names. Some browsers tolerate any case, some browsers require a certain camelCase as per spec. Example: tabIndex
on IE.
dojo.hasAttr()
The current version of dojo.hasAttr()
checks for the presence of an attribute node. If it is not present, or its property specified
is false
, the function returns a falsy value, otherwise it returns a truthy value.
The negative result does not mean that there is no effective value. It does not mean that there is no property with the same name. And on IE it does not mean that the attribute in question was not specified in the HTML markup.
dojo.attr()
The current version of dojo.attr()
in the "set" mode does following actions:
- Special processing for
style
. - Special processing for
innerHTML
. - If the value is a function, setting of an event handler is assumed, and specially processed.
- If the value is Boolean, it forces setting a property, rather than an attribute.
- If the name is in the special list, it forces a property setting.
- Otherwise
setAttribute()
is used.
In the "get" mode it does following actions:
- If the name is in the special list, and the existing value of a property is not
undefined
, the property value is returned. - If a value of the property is Boolean or a function, it is immediately returned.
- If
dojo.hasAttr()
is positive, {{{getAttribute()}}'s value is returned. - Otherwise
null
is returned.
To be continued...
comment:16 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
comment:17 follow-up: 19 Changed 13 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:18 Changed 13 years ago by
comment:19 follow-up: 20 Changed 13 years ago by
Replying to becky:
I didn't realize that a commit comment with both "fixes" and a ticket number anywhere in the text would close all referenced tickets - I thought they had to be directly after the fixes keyboard. Thus, #8991 is NOT fixed by [18297] (above). Thus, reopening.
Well, I just reopened this ticket because the changes here ([17931]) broke dijit. If dijit is working now then this ticket can probably be closed again, although I'll leave it to Eugene.
comment:20 Changed 13 years ago by
Replying to bill:
Well, I just reopened this ticket because the changes here ([17931]) broke dijit. If dijit is working now then this ticket can probably be closed again, although I'll leave it to Eugene.
I need to know if something is still broken, but I am not done with the ticket yet --- at the very least I need to document new addition as per peller's comment.
comment:21 Changed 13 years ago by
comment:22 follow-ups: 23 24 Changed 13 years ago by
For an input with no type specified, this changes the return code from "text" to null. In dojo/tests/_base/html.html, try
dojo.attr("input-no-type", "type")
I guess that was intended, given that there's now a unit test checking the return code is null, although the behavior change is breaking dijit.Dialog (and could break other folks depending on the old behavior). I'll check in a fix to dijit.Dialog.
comment:23 follow-up: 25 Changed 13 years ago by
Replying to bill:
For an input with no type specified, this changes the return code from "text" to null.
What browser?
comment:24 Changed 13 years ago by
Replying to bill:
I guess that was intended, given that there's now a unit test checking the return code is null, although the behavior change is breaking dijit.Dialog (and could break other folks depending on the old behavior). I'll check in a fix to dijit.Dialog.
You can try dojo.getEffectiveAttrValue()
--- it is supposed to return the effective value, and should be "text"
in this case.
Hint: dojo.getEffectiveAttrValue()
works like read-only dojo.attr()
, but defaults to node properties rather than attributes. According to the spec it is supposed to be the authoritative source.
comment:25 Changed 13 years ago by
comment:26 Changed 13 years ago by
comment:27 follow-ups: 28 29 Changed 13 years ago by
Apologies if I'm out of order, but I just hit what appears to be this bug. While parsing some custom XML, I noticed all calls to dojo.attr(xmlDomNode, 'type') returned undefined/null. I boiled this down to a simple HTML example:
var mydiv = dojo.create('div', { type: 'mytype' }); console.log(dojo.attr(mydiv, 'type')); console.log(mydiv.getAttribute('type'));
The first log prints undefined and the second properly shows "mytype". Other attribute names work fine, but "type" seems to be a special case that is somehow borked. This is with Dojo 1.3.2 running in FireFox? 3.5.1 on Windows XP SP3.
PS. I see this ticket has a 1.4 milestone. Any chance of seeing a fix in a 1.3 point release?
comment:28 Changed 13 years ago by
Replying to Foam Head:
The first log prints undefined and the second properly shows "mytype".
Amazing. This is the code from dojo.attr()
that reads attributes in three consecutive sections:
// getter // should we access this attribute via a property or // via getAttribute()? value = node[propName]; if(forceProp && typeof value != "undefined"){ // node's property return value; // Anything }
The previous if
never returns undefined
by definition.
if(propName != "href" && (typeof value == "boolean" || d.isFunction(value))){ // node's property return value; // Anything }
The previous if
can return only a Boolean and a function value.
// node's attribute // we need _hasAttr() here to guard against IE returning a default value return _hasAttr(node, attrName) ? node.getAttribute(attrName) : null; // Anything
The final line can return either node.getAttribute(attrName)
(which is "mytype"
according to your example) or null
.
Yet undefined
is returned somehow.
Are you sure you used the trunk for your test?
If it returns null
it means that _hasAttr()
fails somehow. It is very simple and hard to get wrong:
var _hasAttr = function(node, name){ var attr = node.getAttributeNode && node.getAttributeNode(name); return attr && attr.specified; // Boolean };
The alternative explanation would be that type
has a special (magic) meaning in DOM, and should not be used on DOM nodes.
Still I'll check the test snippet to see what is going on. In any case it is intriguing.
comment:29 Changed 13 years ago by
As I mentioned in my first comment, I hit this bug in Dojo 1.3.2. If I knew how to test off of trunk (without needing to do a full source tree download and build), I'd be glad to do it.
For reference, if you load up 1.3.2, you can do the following in the FireBug? console to demonstrate the problem:
>>> dojo.version 1.3.2 (18832) major=1 minor=3 patch=2 revision=18832 >>> var foo = dojo.create('div', { name: 'myname', type: 'mytype' }) >>> foo <div name="myname" type="mytype"> >>> dojo.hasAttr(foo, 'name') true >>> dojo.attr(foo, 'name') "myname" >>> foo.getAttribute('name') "myname" >>> dojo.hasAttr(foo, 'type') true >>> dojo.attr(foo, 'type') >>> typeof dojo.attr(foo, 'type') "undefined" >>> foo.getAttribute('type') "mytype"
Obviously I'd like to ensure this is fixed in trunk for 1.4, but I also think it warrants a bug fix in 1.3 point release. If you need me to open additional tickets or whatnot, just lemme know.
comment:30 Changed 13 years ago by
You can go to the nightlies (http://archive.dojotoolkit.org/) and run an test you like to load the base, e.g., http://archive.dojotoolkit.org/nightly/dojotoolkit/dojo/tests/parser.html --- now you can do the research from the Firebug console.
I tried your scenario and it works properly in the trunk --- it looks like the last rewrite fixed this bug as well.
comment:31 Changed 13 years ago by
I was able to use a nightly build test to confirm dojo.attr(something_with_a_type_attr, 'type') does work. So for 1.4, I agree this problem is closed.
However, this still fails in Dojo 1.3.2. I don't know what your screening process for point release fixes is, but can you please bring this up for discussion? IMHO it warrants a fix in a point release, but it may not warrant the creation of a point release; i.e. if a 1.3.3 release is going to happen for other reasons, this fix should definitely be included. If you need me to create a ticket for this, please let me know.
comment:32 Changed 13 years ago by
It is hard to tell about including this code in the upcoming point release (if it happens).
Typically we include only simple bugfixes. The change I did was a complete rewrite --- I felt the code was buggy, too complex, and in some places illogical, because some underlying reasons for it were not clear. It means that it will break some code for sure --- I know that because it broke some code in Dijit/DojoX, which needed some light patching.
While it is relatively simple to replace dojo.attr()
, it would be a major chore to extract related changes from other parts of the toolkit.
I frequently run my projects from a snapshot of Dojo trunk --- we try to keep it stable and in many cases it works fine. If this option is acceptable to you (obviously it requires some trials), you can try to use the trunk. Otherwise, you can patch your own copy of Dojo, or package it as a separate custom function, which will be eliminated when the next release is out.
comment:33 Changed 13 years ago by
I agree that you rarely want to back prop a redesign into a point release, but it shouldn't stop a new fix in a point release. Yes, this is throw away work, but I've since confirmed this is a bigger issue and may warrant the effort.
Looking at the code for dojo.attr, a dojo.attr(domNode, 'attrName') invocation basically just runs this code:
// getter // should we access this attribute via a property or // via getAttribute()? var prop = _attrProps[name.toLowerCase()]; if(prop){ return node[prop]; } var attrValue = node[name]; return (typeof attrValue == 'boolean' || typeof attrValue == 'function') ? attrValue : (d.hasAttr(node, name) ? node.getAttribute(name) : null);
The first few lines with prop looked a little odd to me, so I checked out _attrProps:
// non-deprecated HTML4 attributes with default values // http://www.w3.org/TR/html401/index/attributes.html // FF and Safari will return the default values if you // access the attributes via a property but not // via getAttribute() var _attrProps = { colspan: "colSpan", enctype: "enctype", frameborder: "frameborder", method: "method", rowspan: "rowSpan", scrolling: "scrolling", shape: "shape", span: "span", type: "type", valuetype: "valueType", // the following attributes don't have the default but should be treated like properties classname: "className", innerhtml: "innerHTML" }
So the intent is to get the attribute's value from the DOM node when the attribute was not specified by markup and has a default value. This makes perfect sense and works on nodes that have default attribute values. The problem is it doesn't work on nodes that don't have those default values. That's why you get this (from the FireBug? console with Dojo 1.3.2):
>>> dojo.attr(dojo.create('div', {}), 'type') >>> dojo.attr(dojo.create('div', { type: 'mytype' }), 'type') >>> dojo.attr(dojo.create('input', {}), 'type') "text" >>> dojo.attr(dojo.create('input', { type: 'reset'}), 'type') "reset"
So the code handles its intended case of providing defaults, but it accidentally short-circuits the code for nodes without the default values. There are a couple of ways to solve this, but one solution might look something like this:
// getter // should we access this attribute via a property or // via getAttribute()? var attrValue = node[name]; if (typeof attrValue == 'boolean' || typeof attrValue == 'function') { return attrValue; } else if (d.hasAttr(node, name)) { return node.getAttribute(name); } else { var prop = _attrProps[name.toLowerCase()]; return (prop ? node[prop] : null); }
This is untested and you may need to swap the first and second cases, but you get the idea. It's not a huge fix and it should be easy to ensure all of the _attrProps attributes work in all node types.
If there's anything else I can do to help with this, please let me know.
comment:34 Changed 13 years ago by
Thanks for the proposed patch. It is small enough, so I don't think we need a CLA from you. If we are to make 1.3.3, I'll try to put it in.
comment:35 Changed 13 years ago by
Adding to the Brain Dump above (elazutkin):
The Purpose
Why???
dojo.attr
It was explained to me that dojo.attr()
is meant to accomplish two things:
- Some stuff is an attribute, and some stuff is a property. Confusingly in many cases you can read/write both, which can work in some browsers and bomb in others.
dojo.attr()
was supposed to hide this duality in most common code. - Another obvious reason was to eliminate browser differences in getting/setting attributes. For example, some browsers expect certain attribute names in a correct case.
dojo.removeAttr
I think it was added mostly for completeness. What it does it resolves a node, using dojo.byId()
, and can correct a name for some known attributes --- basically it is a one-liner.
dojo.hasAttr
Amazingly this little function complicates the whole picture.
Obviously properties are always there, so it deals with attributes. It is used to see if certain attributes were set on a node (in HTML). It can do its role to a point, because there are some problems most notably with IE.
Somehow it is expected that results of dojo.hasAttr()
and dojo.attr()
should match. It leads to dojo.attr()
returning null
when dojo.hasAttr()
returns false
, otherwise it should return some sane attribute value. But the absence of an attribute doesn't mean that there is no effective (e.g., default) value for it. It can be found in a corresponding property. The current dojo.attr()
does not support reading it without an attribute present, and we cannot change its behavior without breaking a lot of code.
That's why I added dojo.getEffectiveAttrValue()
, which works only for reading values, and consults a property first falling back to attributes, when there is no property available. It returns the default/effective value, if it is available.
Exceptions
Some properties do not shadow attributes, e.g., className
, innerHTML
, and so on. They should be always accessed as node properties. To enforce that there is a small list of such names.
Some attributes have non-matching property names for various reasons (e.g., programming language conflicts). They have their own table.
While majority of properties are lower-case names, some of them use the camel case. There is a special table for that too.
There is a special processing for style
by deferring to dojo.style()
, and for innerHTML
to handle read-only cases (HTML in IE).
comment:36 Changed 13 years ago by
In order to close this ticket I need to revisit the documentation and document dojo.getEffectiveAttrValue()
. BTW I hate the name, it is too long. Name ideas?
comment:37 Changed 13 years ago by
I agree, getEffectiveAttValue is a horrible name for a public base API.
re: docs; please add this to release notes, and expand inline docs to contain at least one example (and wordage to explain stuff from this ticket, about why this is to be used over .attr)
comment:38 Changed 13 years ago by
comment:39 Changed 13 years ago by
comment:40 Changed 13 years ago by
comment:41 Changed 13 years ago by
Component: | HTML → Documentation |
---|
Need to document dojo.getNodeProp()
and review the existing documentation of dojo.attr()
.
comment:42 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Reworked documentation for:
dojo.attr()
dojo.style()
Added documentation for:
dojo.getNodeProp()
Documented undocumented functions (why weren't they documented???):
dojo.hasAttr()
dojo.removeAttr()
dojo.NodeList.attr*()
I've noticed that dojo.NodeList
is largely undocumented.
comment:43 Changed 9 years ago by
For reference, jquery went through the same growing pains, see http://stackoverflow.com/questions/5874652/prop-vs-attr.
I'm still unclear of when application code should set properties vs. setting attributes. For example, I wonder how dijit should be setting tabIndex and aria roles/states of nodes.
I tried test_Menu.html and then in the console did
dojo.attr("input1", "value")
.That works correctly, BUT if I type a different value than "top-left" into the input, the dojo.attr() still returns the original value.
This isn't a new problem though, it happened in 1.2 also.