Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#14341 closed defect (fixed)

Change _WidgetBase._applyAttributes() logic to avoid ignoring some properties values passed in the constructor

Reported by: cjolif Owned by: bill
Priority: high Milestone: 1.8
Component: Dijit Version: 1.7.0
Keywords: Cc: Douglas Hays
Blocked By: Blocking:

Description

We are encountering an issue / limitation with _WidgetBase._applyAttributes().

I have two properties with side effects on each other (I know, not necessarily a good idea, but sometimes quite handy):

define(["dojo/_base/declare", "dijit/_WidgetBase"], function(declare,
_WidgetBase){
       return declare(_WidgetBase, {
               single: null,
               multiple: [],
               _setSingleAttr: function(value){
                       this.multiple = value!=null?[value]:null;
                       this._set("single", value);
               },
               _setMultipleAttr: function(value){
                       this.single = value?(value.length >0?value[0]:null):null;
                       this._set("multiple", value);
               }
       });
});

If I set one of these properties in the ctor, I don't get the expected results:

               require(["dijit/tests/TestWidget"], function(TestWidget){
                       var w = new TestWidget({
                               single : 5
                       });

                       console.log(w.single);
               });

will output null, not 5.

This comes from the mechanism in applyAttributes. What happens is the following:

0/ default values are: single: null, multiple: []

1/ in create() the ctor properties are mixed in the component so I have single: 5 multiple: []

2/ in applyAtttributes() all properties not set in the ctor but that have a not null/""/0 default values have their setter called, which gives: multiple: [] -> side effect (through the setter) -> single: null

3/ still in applyAttributes() all properties from the ctor have their setter called _with_ the value in the component (not with the value passed in the ctor). which gives: single -> stays null because null is what is in the component.

I'll provide a possible patch + regression later on.

Attachments (2)

fixes_#14341.patch (2.2 KB) - added by cjolif 7 years ago.
patch fixing the issue including doh test update
14341.patch (1.7 KB) - added by bill 7 years ago.
updated patch, no need for a separate file for test widget, plus _Widget-attr.html converted to AMD format

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by cjolif

Owner: set to bill

Changed 7 years ago by cjolif

Attachment: fixes_#14341.patch added

patch fixing the issue including doh test update

comment:2 Changed 7 years ago by bill

Unfortunately CheckBox_mouse.html fails [on IE8] with this patch, in the setValues() test:

_AssertFailure: assertEqual() failed:
	expected
		{"cb1":["foo"],"cb2":[],"cb4":[],"cb5":["on"],"cb6":[],"cb7":[],"g[1]":null,"g2":"country"}
	but got
		{"cb1":["foo"],"cb2":[],"cb4":[],"cb5":[],"cb6":[],"cb7":[],"g[1]":null,"g2":"country"}

Changed 7 years ago by bill

Attachment: 14341.patch added

updated patch, no need for a separate file for test widget, plus _Widget-attr.html converted to AMD format

comment:3 Changed 7 years ago by cjolif

Thanks a lot Bill for the full testing.

Spending some time trying to understand what happened I think the regression failure is more revealing a hidden problem in Checkbox than anything else.

The problem appears when CheckBox is build with passing value:"" in the ctor.

Then after the mixin of ctor properties, in _CheckBoxMinx.postMixInProperties() when CheckBox.value is "" it is transform back to "on" (the default value). As we now use this.params["value"] and not anymore this["value"] to actually set the value in applyAttributes the values goes back to "" instead of the previously expected "on". And the test finally fail because it obviously expects "on" to be there.

I think the actual problem does not come from the code modification but from the fact CheckBox behavior is not consistent between what happens in ctor (where postMixInProperties override "" to "on") and what happens in the setter where the same job is not done (i.e. "" is kept and not transformed to "on").

Changing CheckBox._setValueAttr implementation to do something like the following:

if(newValue==""){
  newValue = "on";
}

does solve the issue.

Does it sound logical to you?

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

comment:4 Changed 7 years ago by bill

Cc: Douglas Hays added

Thanks for tracing that down. I guess your suggestion makes sense although I'd like to hear from Doug, because that code in postMixinProperties()

if(this.value == ""){
	this.value = "on";
}

comes from from Doug's checkin [14287].

The comment is that it's to match the behavior of FF3, apparently meaning that markup like:

<input name="cb5" id="cb5" type="checkbox" value="" ...

will cause a form submission with value="on" rather than value="".

I have no idea why someone would write value="" in their markup though, or what "should" happen when someone calls checkbox.set("value", ""). It seems like setting the value to "" either in the constructor or afterwards is invalid.

PS: this does make me wonder if any other widgets are depending on setting this.xyz in postMixInProperties() and then depending on _setXyzAttr() being called with the value they set. Hopefully not. The function name "postMixInProperties" itself is worrisome. Some red flag code in various widgets' postMixinProperties():

ProgressBar.js:

if(!("value" in this.params)){
	this.value = this.indeterminate ? Infinity : this.progress;
}

FontChoice.js:

if(!this.values){
	this.values = this.generic ?
		["serif", "sans-serif", "monospace", "cursive", "fantasy"] : // CSS font-family generics
			["Arial", "Times New Roman", "Comic Sans MS", "Courier New"];
}

NumberTextBox.js:

postMixInProperties: function(){
	this.inherited(arguments);
	this._set("type", "text"); // in case type="number" was specified which messes up parse/format
},

_DateTimeTextBox.js:

postMixInProperties: function(){
	this.inherited(arguments);
	this._set("type", "text"); // in case type="date"|"time" was specified which messes up parse/format
},

SimpleTextArea.js:

// Copy value from srcNodeRef, unless user specified a value explicitly (or there is no srcNodeRef)
// TODO: parser will handle this in 2.0
if(!this.value && this.srcNodeRef){
	this.value = this.srcNodeRef.value;
}

_AutoCompleterMixin.js:

postMixInProperties: function(){
	if(!this.store){
		var srcNodeRef = this.srcNodeRef;
		var list = this.list;
		if(list){
			this.store = registry.byId(list);
		}else{
			// if user didn't specify store, then assume there are option tags
			this.store = new DataList({}, srcNodeRef);
		}
...

LinkPane.js:

// If user has specified node contents, they become the title
// (the link must be plain text)
if(this.srcNodeRef){
	this.title += this.srcNodeRef.innerHTML;
}

TabContainer.js:

if(!this.controllerWidget){
	this.controllerWidget = (this.tabPosition == "top" || this.tabPosition == "bottom") && !this.nested ?
				"dijit.layout.ScrollingTabController" : "dijit.layout.TabController";
}

I also see a lot of code in postMixInProperties() methods setting variables used in the template, code which probably more logically belongs in buildRendering().

comment:5 Changed 7 years ago by Douglas Hays

I think cjolif's suggested Checkbox change makes sense and should be committed along with the patch. It'll consistently set value to "on" if someone programmatically sets value="". I think the postMixInProperties functions for _DateTimeTextBox.js and NumberTextBox?.js are correct to override type="text" there. Presumably the type attribute was specified as an unsupported HTML5 value by the user and this value needs to be changed before being rendered. buildRendering seems like an attribute-read-only phase to me and I don't think calling this.inherited from a user-override of the function would be expected to change an attribute.

comment:6 Changed 7 years ago by bill

Resolution: fixed
Status: newclosed

In [27308]:

Change _WidgetBase._applyAttributes() logic to avoid ignoring some properties values passed in the constructor. Fixes #14341 !strict.

comment:7 Changed 7 years ago by bill

Thanks Doug, I checked in those changes. NumberTextBox.js and _DateTimeTextBox.js are working, although just barely. When type="number" is specified to the constructor, after postMixInProperties() executes the line:

this.type="text";

_applyAttributes() will reverse that by calling

set("type", this.params["type"])

where this.params["type"] == "number".

Since there's a _setTypeAttr: null setting, the set() call above doesn't do anything, but still feels a bit weird.

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

comment:8 Changed 7 years ago by bill

In [27311]:

add some comments, refs #14341 !strict.

comment:9 Changed 7 years ago by bill

In [27328]:

Fallout from [27308], problem with missingMessage and invalidMessage implicit values based on promptMessage. Fixes failure in validationMessages.html test. Refs #14341 !strict.

comment:10 Changed 7 years ago by cjolif

We just hit a similar but different issue. We have a widget with a Boolean property that default to false.

Our _setOurProperty method is not called at init.

This is due to the non null/""/0/false test in applyAttributes:

if(this[attr]){
  this.set(attr, this[attr]);			
}

This is confusing. Because if our Boolean had a truish default value the setter would have been called.

I'm not sure of the implications but something along those lines:

if(this[attr] != undefined){
  this.set(attr, this[attr]);
}

would sound more logical to me?

comment:11 Changed 7 years ago by bill

Right, we talked about this over email IIRC, and actually there's a separate ticket #7381 for it.

comment:12 Changed 7 years ago by cjolif

Thanks Bill this was ringing a bell to me but was not finding references to this. Now I have it.

comment:13 Changed 7 years ago by bill

See also #16080, for fallout from this fix.

comment:14 Changed 7 years ago by bill

In [29760]:

Rework fix to #14341 so that properties can still be modified in postMixInProperties(). Refs #16080, #14341 on trunk !strict.

comment:15 Changed 7 years ago by bill

In [29761]:

Rework fix to #14341 so that properties can still be modified in postMixInProperties(). Fixes #16080, #14341 on 1.8 branch !strict.

comment:16 Changed 6 years ago by bill

#16312 is a duplicate of this ticket.

comment:17 Changed 6 years ago by bill

In [29920]:

update comment to match behavior, refs #14341 !strict

Note: See TracTickets for help on using tickets.