Opened 10 years ago

Closed 10 years ago

#10324 closed defect (fixed)

dijit.form widgets do not disable it's corresponding valueNode

Reported by: Pete Smith Owned by: Nathan Toone
Priority: high Milestone: 1.4
Component: Dijit Version: 1.4.0b
Keywords: Cc: Douglas Hays
Blocked By: Blocking:

Description (last modified by bill)

Many form dijits do this on their own __setDisabledAttr methods, like the validationTextBox,

      if(this.valueNode){
                this.valueNode.disabled = value;
       }

but it really should do this in _FormValueWidget. Right now my dijit.form.Select's hidden valueNodes don't disable when I do an attr('disabled', true) on it.

We could probably remove the other dijits that do this themselves.

Attachments (2)

formValueDisable.patch (1.4 KB) - added by Nathan Toone 10 years ago.
Patch which fixes this issue
formValueDisable.2.patch (1.3 KB) - added by Nathan Toone 10 years ago.
Updated patch - follows doug's suggestions

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by Nathan Toone

Cc: wildbill added
Milestone: tbd1.5
Owner: set to Nathan Toone
Priority: highestnormal
severity: criticalnormal
Summary: dijit.__setDisabledAttr does not disable it's corresponding valueNodedijit.form widgets do not disable it's corresponding valueNode

Only ValidationTextBox? does this. Bill - is there a reason why other widgets aren't doing it? Couldn't we just move that logic into _FormValueWidget as suggested?

Changed 10 years ago by Nathan Toone

Attachment: formValueDisable.patch added

Patch which fixes this issue

comment:2 Changed 10 years ago by Nathan Toone

Cc: bill added; wildbill removed

comment:3 Changed 10 years ago by bill

Cc: Douglas Hays added; bill removed
Description: modified (diff)

Apparently the actual bug is that certain widgets, although disabled, still send their value when a form is submitted?

Looking at the code, seems like the offenders are dijit.form.Select, dijit.form.Slider, and dojox.widget.ColorPicker. Also dojox.gauge.Gauge. The widgets that already extend MappedTextBox are fine, since it has code to disable valueNode already.

I guess moving that code into _FormValueWidget makes sense. Doug what do you think?

Should also add a comment to the top of FormValueWidget defining what valueNode is, for documentation purposes:

// valueNode: [protected] DomNode
//		Hidden DomNode (usually an <input>) containing the value of this widget.
//		Used to send the value to the server when the form is submitted.
/*=====
valueNode: null,
=====*/

or something like that.

In any case I think that the code in ValidationTextBox can be removed.

On a related note:

  • shouldn't dojox.widget.ColorPicker extend FormValueWidget rather than FormWidget?
  • dojox.Gauge also has a valueNode (a hidden <input>) but it doesn't even extend FormWidget. Maybe it should also extend FormValueWidget.

comment:4 Changed 10 years ago by Douglas Hays

The patch looks OK but it seems like adding the valueNode IF to the existing _setDisabledAttr in _FormWiget instead of creating a new _setDisabledAttr to FormValueWidget? might be better. Also, using this.attr('disabled',blah) might be better than just setting node.disabled.

Changed 10 years ago by Nathan Toone

Attachment: formValueDisable.2.patch added

Updated patch - follows doug's suggestions

comment:5 Changed 10 years ago by Nathan Toone

Doug - is that second patch a bit more what you were thinking?

comment:6 Changed 10 years ago by Douglas Hays

Looks right to me. I'm not sure what to do with Bill's suggestion on documented valueNode in _FormWidget.js. It doesn't seem to really belong there and should be in widgets that introduce it but maybe he's trying to go with defining it once and not in several places.

comment:7 Changed 10 years ago by bill

Description: modified (diff)

Yah, mainly I wanted to make sure it was defined somewhere, although _FormWidget seems like a reasonable place since (after the patch) it's referenced from there. The comment should say something like "Optionally defines a ...".

I'm OK with this in 1.4

comment:8 Changed 10 years ago by Nathan Toone

Resolution: fixed
Status: newclosed

(In [20826]) Fixes #10324 - disable the underlying value widget when disabling the widget

comment:9 Changed 10 years ago by Nathan Toone

Milestone: 1.51.4

comment:10 Changed 10 years ago by Pete Smith

Resolution: fixed
Status: closedreopened

Gents, related to this, is if I this.attr('name', 'somethingelse') the valueNode does not change it's name either. Shouldn't the valueNode respect all of the attr's on the parent Widget?

comment:11 Changed 10 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

I don't think IE supports changing the name attribute. If you find that IE does support this, then please open a separate ticket that we can work on in 1.5.

Note: See TracTickets for help on using tickets.