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 bill)

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)

setFalsy.patch (1.1 KB) - added by bill 8 years ago.
naive patch to call all custom setters, showing the issues listed above

Download all attachments as: .zip

Change History (11)

comment:1 Changed 9 years ago by bill

Summary: call attr() for every attribute on initializationcall set() for every attribute on initialization, include those with falsy value

comment:2 Changed 9 years ago by cjolif

Cc: cjolif added

comment:3 Changed 9 years ago by cjolif

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).

comment:4 Changed 9 years ago by bill

Description: modified (diff)
Milestone: future2.0
Summary: call set() for every attribute on initialization, include those with falsy valuecall 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 bill

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.

Version 3, edited 8 years ago by bill (previous) (next) (diff)

comment:6 Changed 8 years ago by bill

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

browserbeforeafter
FF 16 (1000 widgets)730930
IE8 (100 widgets)200-1000300
Chrome (1000 widgets)530680

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.

Last edited 8 years ago by bill (previous) (diff)

comment:7 Changed 8 years ago by bill

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 bill

Attachment: setFalsy.patch added

naive patch to call all custom setters, showing the issues listed above

comment:8 Changed 8 years ago by bill

Alternate approach: only call custom setters for parameters specified to the constructor.

comment:9 Changed 8 years ago by bill

#17005 is a duplicate of this ticket.

comment:10 Changed 7 years ago by bill

Resolution: wontfix
Status: newclosed

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.

Note: See TracTickets for help on using tickets.