Opened 14 years ago
Closed 9 years ago
#7381 closed task (wontfix)
call set() for every attribute on initialization, include those with falsy values
Reported by: | bill | Owned by: | bill |
---|---|---|---|
Priority: | high | Milestone: | 2.0 |
Component: | Dijit | Version: | 1.1.1 |
Keywords: | Cc: | cjolif | |
Blocked By: | Blocking: |
Description (last modified by )
Widget.set() is called on widget initialization for all explicitly specified attributes, plus any attributes with truthy values.
Might be nice to call it for attributes with falsy values too, so we wouldn't need lines like this in postCreate():
this.attr('disabled', this.disabled);
Not sure if this is practical or not but filing ticket to consider.
Attachments (1)
Change History (11)
comment:1 Changed 10 years ago by
Summary: | call attr() for every attribute on initialization → call set() for every attribute on initialization, include those with falsy value |
---|
comment:2 Changed 10 years ago by
Cc: | cjolif added |
---|
comment:3 Changed 10 years ago by
comment:4 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Milestone: | future → 2.0 |
Summary: | call set() for every attribute on initialization, include those with falsy value → call set() for every attribute on initialization, include those with falsy values |
Marking for 2.0 for the time being. Not sure if it could be done before then (or even if it's feasible for 2.0, but hopefully it is).
comment:5 Changed 10 years ago by
See also #14341.
Back-compat:
There are a number of back-compat issues with changing this in 1.x, including:
1) unwanted inherited setters
Classes like Button define a _setValueAttr that accesses this.valueNode. Subclasses like ScrollingTabControllerMenuButton inherit that setter, but don't define this.valueNode. Thus you get an exception.
Many of these cases can be handled in _WidgetBase _applyAttributes() by checking if the attach point is set or not, but that won't handle the case of actual custom setters, in dijit or customer code, like:
_setValueAttr: function(){ this.valueNodeAttr = ... }
2) other existing setters that don't work with falsy value
For example:
dir: "", _setDirAttr: function(val){ this.domNode.setAttribute("dir", val); }
If _setDirAttr("") were called at construction, it would set this.domNode.dir to "", an invalid value. The correct behavior is to not set the dir attribute at all, or better, to call removeAttribute("dir"). There were a bunch of attributes like that, especially relating to form widgets.
Nowadays _WidgetBase has the nonEmptyAttrToDom() factory, used like
_setDirAttr: nonEmptyAttrToDom("dir")
nonEmptyAttrToDom() will call setAttribute() or removeAttribute() as appropriate. So something like that would need to be done for any custom setters that don't currently handle falsy values correctly.
3) other issues
In for example ComboBox, on IE8 (but for some timing reason not on FF), the first ComboBox in test_ComboBox.html ends up with a blank value.
Although we have code to make sure that parameters to the constructor are executed after default setters, so that the null item shouldn't override the explicitly specified value, it's still failing for some reason
comment:6 Changed 10 years ago by
Performance:
I suppose the other issue is performance. I assume that most widget properties have falsy default values, and most properties are not specified to the constructor, so it would mean an order of magnitude increase in the number of setters called during page construction.
For example, the constructor for class would be called for every widget instance:
// class: String // HTML class attribute "class": "", _setClassAttr: { node: "domNode", type: "class" },
It's interesting that Stateful only calls the custom setters for parameters passed to the constructor, and never for properties with default values.
Some performance results, running dijit/tests/benchTool.html, creating widgets programatically
browser | before | after |
FF 16 (1000 widgets) | 730 | 930 |
IE8 (100 widgets) | 200-1000 | 300 |
Chrome (1000 widgets) | 530 | 680 |
IE is all over the place, not sure what that means, although it's not unusual for me to see random benchmark results like that.
comment:7 Changed 10 years ago by
And, the final question (I hope) is what to do with "implicit setters", see [25598]. Should this.set("accept-charset", ...) be called when a form widget is initialized even if accept-charset was not explicitly specified as a parameter to the constructor? Does it matter if the default value is falsy or not?
Changed 10 years ago by
Attachment: | setFalsy.patch added |
---|
naive patch to call all custom setters, showing the issues listed above
comment:8 Changed 9 years ago by
Alternate approach: only call custom setters for parameters specified to the constructor.
comment:10 Changed 9 years ago by
Resolution: | → wontfix |
---|---|
Status: | new → closed |
We took a different approach in our 2.0 prototype: only call custom setters for parameters specified to the widget. Not sure if that means this ticket is "fixed" or "wontfix" but anyway, it should be closed.
We faced that twice when developing widgets with Boolean properties.
We were wondering why we were getting a different behavior depending on the default value of our properties (false or true).