Opened 11 years ago

Closed 11 years ago

Last modified 9 years ago

#8966 closed defect (fixed)

Form: when multiple fields initially invalid/required+blank, correcting one field marks form as valid

Reported by: Jarrod Carlson Owned by: bill
Priority: high Milestone: 1.3
Component: Dijit - Form Version: 1.3.0b3
Keywords: dijit form formmixin Cc:
Blocked By: Blocking:

Description

_FormMixin.isValid() uses dojo.every() to build an array of invalid child widgets. This behavior is broken, though, because dojo.every() stops on the first non-truthy iteration.

Thus, if a form contains two required text fields and both are blank, the isValid() method will only catch the first invalid widget and then stop.

Subsequently, when _FormMixin._widgetChange() is invoked by a child widget becoming valid, the entire form is not re-validated, only the array on _invalidWidgets, which is not complete. The onValidStateChange handler is thus incorrectly fired, and the form is considered valid when it is not.

Attached is a test page to illustrate this issue.

To rectify the problem, isValid() should use another method of locating invalid descendants.

I would call this a pretty critical bug, but it should be fairly easy to fix. If I have time today, I might be able to submit a patch...

Attachments (2)

test_FormMixin.html (2.2 KB) - added by Jarrod Carlson 11 years ago.
test file demonstrating defect in _FormMixin.isValid()
_FormMixin.js.diff (667 bytes) - added by Jarrod Carlson 11 years ago.
Patch for _FormMixin.js

Download all attachments as: .zip

Change History (10)

Changed 11 years ago by Jarrod Carlson

Attachment: test_FormMixin.html added

test file demonstrating defect in _FormMixin.isValid()

Changed 11 years ago by Jarrod Carlson

Attachment: _FormMixin.js.diff added

Patch for _FormMixin.js

comment:1 Changed 11 years ago by Jarrod Carlson

Actually, the change was incredibly simple, and the attached patch should take care of it...

comment:2 Changed 11 years ago by bill

Priority: highnormal
severity: criticalnormal

I see your point. This is fallout from #7788.

Thanks for the patch. BTW in this case a marginally better way to do it is to call dojo.filter() to generate the array. Anyway, I'll try to checkin for 1.3.

comment:3 Changed 11 years ago by bill

Owner: set to bill
Status: newassigned

comment:4 Changed 11 years ago by bill

Fixed in [17106], thanks for catching this!

comment:5 Changed 11 years ago by bill

Resolution: fixed
Status: assignedclosed
Summary: _FormMixin Does not Validate ProperlyForm: when multiple fields initially invalid/required+blank, correcting one field marks form as valid

comment:6 Changed 11 years ago by bill

Plus typo fix in [17120].

comment:7 Changed 10 years ago by bill

(In [17613]) Automated testcase for initially invalid form. Refs #8966.

comment:8 Changed 9 years ago by bill

Component: DijitDijit - Form
Note: See TracTickets for help on using tickets.