Opened 13 years ago
Closed 7 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 9 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 9 years ago by
Cc: | cjolif added |
---|
comment:3 Changed 9 years ago by
comment:4 Changed 9 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 8 years ago by
See also #14341.
Back-compat:
There are a number of back-compat issues with changing this in 1.x, including:
1) dependent properties
In for example ComboBox, {{{new ComboBox?({value: "foo"}) is problematic because set("value", "foo") will be called but also set("item", null) will be called, and the latter will override the first. Although we have code to make sure that parameters to the constructor are executed after default setters, I'm seeing this manifest itself on IE8 (but for some timing reason not on FF) where the first ComboBox in test_ComboBox.html ends up with a blank value.
2) 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 = ... }
3) 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 though _WidgetBase has the nonEmptyAttrToDom() factory, used like
_setDirAttr: nonEmptyAttrToDom("dir")
nonEmptyAttrToDom() will call setAttribute() or removeAttribute() as appropriate. So while some widgets with custom setters would certainly be broken by "fixing" this ticket (and thus some people would complain about breaking backwards compatibility), it might not be as big an issue as I thought.
comment:6 Changed 8 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 8 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 8 years ago by
Attachment: | setFalsy.patch added |
---|
naive patch to call all custom setters, showing the issues listed above
comment:8 Changed 8 years ago by
Alternate approach: only call custom setters for parameters specified to the constructor.
comment:10 Changed 7 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).