Opened 11 years ago

Closed 6 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 7 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 8 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 8 years ago by cjolif

Cc: cjolif added

comment:3 Changed 8 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 8 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 7 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) 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

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

comment:6 Changed 7 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 7 years ago by bill (previous) (diff)

comment:7 Changed 7 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 7 years ago by bill

Attachment: setFalsy.patch added

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

comment:8 Changed 6 years ago by bill

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

comment:9 Changed 6 years ago by bill

#17005 is a duplicate of this ticket.

comment:10 Changed 6 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.