Opened 8 years ago

Closed 7 years ago

#15432 closed defect (fixed)

TimeTextBox: Setting constraints does not update 'state' property

Reported by: Paul Christopher Owned by: Douglas Hays
Priority: undecided Milestone: 1.8
Component: Dijit - Form Version: 1.7.2
Keywords: Cc:
Blocked By: Blocking:

Description

Description

If you e.g. specify certain constraints like a min time in the page's ready function, the TimeTextBox's get('state') function still returns an empty string, although the entry is now invalid. Therefore the whole form is reported as valid although it is not valid anymore: form.get('state') returns an empty string, too. Only calling form.validate() forces a recalculation of the state properties.

Steps to reproduce the issue

Run the attached test case. The form is - as it is - invalid. TimeTextBox's value is 9, constraints.min is set to 10 in ready().

Press button "form.get('state')". This will get the form state via the form.get('state') method. Notice: The form is reported as valid, although it is not valid.

Press button "TimeTextBox.get('state')". This will retrieve the valid state of the TimeTextBox using TimeTextBox.get('state'). Again: the control is reported as valid (=an empty string) although it is - with the new constraints given - not valid anymore.

Now press button "form.validate()". The form is now reported as invalid, the TimeTextBox highlighted red.

Discussion

This is a problem since form.js onSubmit is

onSubmit: function(/*Event?*/ /*===== e =====*/){
			// summary:
			//		Callback when user submits the form.
			// description:
			//		This method is intended to be over-ridden, but by default it checks and
			//		returns the validity of form elements. When the `submit`
			//		method is called programmatically, the return value from
			//		`onSubmit` is used to compute whether or not submission
			//		should proceed
			// tags:
			//		extension

			return this.isValid(); // Boolean
		}

and _formMixin.js isValid is

isValid: function(){
	 		// summary:
	 		//		Returns true if all of the widgets are valid.
			//		Deprecated, will be removed in 2.0.  Use get("state") instead.

			return this.state == "";
		}

Possible fix

Does ValidationTextBox.js _setConstraintsAttr simply needs calling isValid like so??

		_setConstraintsAttr: function(/*Object*/ constraints){
			if(!constraints.locale && this.lang){
				constraints.locale = this.lang;
			}
			this._set("constraints", constraints);
			this._computePartialRE();
			this.set('state', this.isValid());

		}

Or is it maybe better to have a custom getter for the state property?

Attachments (3)

testForm.html (1.6 KB) - added by Paul Christopher 8 years ago.
[CLA]patch01.txt (1.2 KB) - added by Paul Christopher 8 years ago.
[CLA]patch01.diff (1.2 KB) - added by Paul Christopher 8 years ago.

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by Paul Christopher

Attachment: testForm.html added

comment:1 Changed 8 years ago by Paul Christopher

I had some time to look at the code now. I think _setConstraintsAttr simply needs to call _refreshState (as _setRequiredAttr and _setDisabledAttr already does), see patch attached.

  • _setValueAttr uses this.validate instead of this.refreshState. Is this correct?
  • postMixinProperties sets constraints using "this._setConstraintsAttr(this.constraints)". I would have expected that "this.set('constraints', this.constraints)" is used. Is this correct?
  • _TimePicker.js postMixinProperties method uses "this._setConstraintsAttr(this.constraints)" instead of(?) "this.set('constraints', this.constraints)", too. Correct?

Changed 8 years ago by Paul Christopher

Attachment: [CLA]patch01.txt added

Changed 8 years ago by Paul Christopher

Attachment: [CLA]patch01.diff added

comment:2 Changed 8 years ago by Paul Christopher

Changed file extension from *.txt to *.diff

comment:3 Changed 8 years ago by Douglas Hays

Milestone: tbd1.8
Status: newassigned

comment:4 Changed 8 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

In [28743]:

Fixes #15432. Call _refreshState() from _setConstraintsAttr to set state. Added automated tests.

comment:5 Changed 8 years ago by bill

Resolution: fixed
Status: closedreopened

Except that it causes a failure in the beforeFocus test of ValidationTextBox.html (tested on IE8).

_AssertFailure: assertEqual() failed:
	expected
		Incomplete
	but got
		

It also causes 3 failures in Form_state.html (again on IE8).

And finally, a failure in !FilteringSelect_mouse.html.

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

comment:6 Changed 8 years ago by Douglas Hays

Fixes #15432. Run _refreshState in ValidationTextBox.js on startup() after all initial _set*Attr methods have run instead of after each.

comment:7 Changed 8 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

In [28798]:

Fixes #15432. Run _refreshState in ValidationTextBox?.js on startup() after all initial _set*Attr methods have run instead of after each.

comment:8 Changed 8 years ago by bill

In [28843]:

remove empty summary, refs #15432 !strict.

comment:9 Changed 7 years ago by Douglas Hays

In [29071]:

Refs #15432. Need to call startup() on programmatically instantiated TextBbox? subclasses in order to set this._started which in turn allows this._refreshState() to run.

comment:10 Changed 7 years ago by ben hockey

surely automatically calling startup is not right?

comment:11 Changed 7 years ago by ben hockey

Resolution: fixed
Status: closedreopened

comment:12 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

In [29076]:

Fixes #15432. Have _refreshState only validate after _created is set instead of _started since startup() is sometimes never called on TextBox? subclasses.

Note: See TracTickets for help on using tickets.