Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#730 closed defect (fixed)

Combobox passes item label, not item value

Reported by: joose@… Owned by: bill
Priority: high Milestone:
Component: Widgets Version: 0.3
Keywords: Combobox Cc: joose@…, mattimustang@…
Blocked By: Blocking:

Description

ComboBox? places (in some cases) wrong value to field named in HTML... Consider this:

<select name="foo" dojoType="combobox">
 <option value="1">aaa</option>
 <option value="2">bbb</option>
 <option value="3">ccc</option>
</select>

when form is read, foo's value is "aaa" or "bbb" or "ccc", when it should be 1,2 or 3. OF course, this behaviour should be configured behavior, because it is not wanted always. It places the "key" to foo_selected, but it's not good if we already have software to handle foo as key.

Attachments (6)

dojocombobox.diff (2.3 KB) - added by mattimustang@… 13 years ago.
fix for #730
dojoselect.diff (11.4 KB) - added by mattimustang@… 13 years ago.
dojo.widget.Select
dojoselect.2.diff (17.2 KB) - added by mattimustang@… 13 years ago.
updated patch
dojoselect.3.diff (17.7 KB) - added by mattimustang@… 13 years ago.
fix clearing of _selected value in combobox.
dojoselect.4.diff (1.7 KB) - added by mattimustang@… 13 years ago.
updated patch
dojoselect.5.diff (1.2 KB) - added by mattimustang@… 13 years ago.
removed patch to test_Select.html

Download all attachments as: .zip

Change History (30)

comment:1 Changed 13 years ago by anonymous

Component: GeneralWidgets

comment:2 Changed 13 years ago by bill

Summary: ComboxCombobox passes item label, not item value

Changed 13 years ago by mattimustang@…

Attachment: dojocombobox.diff added

fix for #730

comment:3 Changed 13 years ago by mattimustang@…

Cc: mattimustang@… added

I've attached a patch to address this. I ran through the test_ComboBox.html tests and they all appeared to be fine.

regards

Matthew Flanagan

comment:4 Changed 13 years ago by mattimustang@…

Milestone: 0.40.3.1

comment:5 Changed 13 years ago by joose@…

Matthews patch seems to fix exactly this problem.

comment:6 Changed 13 years ago by bill

Matthew, have you filed a CLA?

comment:7 Changed 13 years ago by mattimustang@…

yes I have.

comment:8 Changed 13 years ago by bill

Owner: changed from anonymous to bill
Status: newassigned

comment:9 Changed 13 years ago by bill

Resolution: fixed
Status: assignedclosed

Merged in [4207]

comment:10 Changed 13 years ago by Tom Trenka

Resolution: fixed
Status: closedreopened

Something in this patch hosed the ability to set the value of the widget programmatically (at first glance it would probably be the removal of the setting of the value of the textInputNode in setValue, but that might be something different).

I'm not sure what that did for the widget...but I would expect that .setValue would, in fact, set the value of the widget.

comment:11 Changed 13 years ago by bill

setValue() does set the value but the meaning of "value" is somewhat confusing. In the text below, value is 1, not aaa

<select name="foo" dojoType="combobox">
 <option value="1">aaa</option>
 <option value="2">bbb</option>
 <option value="3">ccc</option>
</select>

Can you be more specific in describing what the issue is?

comment:12 Changed 13 years ago by Tom Trenka

Sure.

It seems like the original widget didn't preserve the value attribute of the option list (if creating via an HTML select list), and that getValue and setValue returned and set (correctly) the label of the box. The applied patch removed the ability to set that correctly programmatically...

For instance, if one was writing a data submission based on an onClick event of something else, they would have (in previous versions), used the following to find out the current value of the widget:

var comboBoxValue=dojo.widget.byId("myComboBox").getValue();

...which would have been the label (since creating new entries for the box would have to, by the nature of the widget, be string values).

The applied patch killed that.

...My suggestion right now would be to roll it back and then throw out to the contributors what the actual purpose and values of the combobox should be. The reality is that because of the nature of the widget, the labels *are* the values, which makes it kind of weird or pointless to attempt to create the combo box out of a select element (since the select element has a value and a label for that value).

It might help to simplify the widget a little bit also...

(Sorry I didn't get back to you on this sooner Bill, I don't get emails from the bug list at all)

comment:13 Changed 13 years ago by mattimustang@…

You now have to use:

var comboBoxValue=dojo.widget.byId("myComboBox").getValue();
// get the displayed label
var comboBoxSelectedValue=dojo.widget.byId("myComboBox").getSelectedValue();

and the corresponding setValue(value) to set the value and setSelectedValue(label) to set the label. Maybe these functions should be renamed or atleast clearly documented.

The combobox widget is emulating a select element and in apps I've worked on the value is quite often different to the label. eg. value="some integer id of an object in a database" and label="human readable representation of the object".

There is no reason why application developers can't just use the "myComboBox_selected" value in their form processing or set both value and label to be the same.

I vote to keep the patch as is.

comment:14 Changed 13 years ago by bill

The confusion here stems from the fact that ComboBox? can be used like HTML's <select>, where the possible input values are predefined, and there's a mapping from the displayed value "California" to a hidden value "CA"... or it can be used more like an <input> box, where the user can type in any string (in addition to selecting a predefined string), and there is no mapping. Arguing over whether the value is "California" or "CA" is just a word game.

It seems the patch effectively changed the names of setValue()/getValue() to setSelectedValue()/getSelectedValue(), which is bad merely for backwards-compatibility reasons. If we just rename the functions to setValue() and setHiddenValue(), will that fix things?

comment:15 Changed 13 years ago by bill

Milestone: 0.3.10.4

Discussed with Alex and Tom and Mathew.

I reverted ComboBox? to function like in 0.2.2, i.e., like an <input> field. User can input an arbitrary value, or select a predefined value from a list. In the case below,

  <input dojoType="ComboBox" name="foo">

when you submit the form, the value of "foo" will be the displayed value, perhaps "Arizona", selected from the drop down list, or "England" (typed in manually).

Mathew will write a new widget called Select that functions like a <select>, with the nomenclature:

  • "value" is the hidden value (ex: "AZ")
  • "label" is the displayed value (ex: "Arizona").

The value submitted with the form is the hidden value (AZ not Arizona)

comment:16 Changed 13 years ago by mattimustang@…

First pass at Select widget.

Changes in following patch:

Changed ComboBox?.js to allow easier subclassing.

Added autocomplete="off" to input in HtmlComboBox?.html to prevent browser autocomplete showing for this widget.

New Select widget.

Changed 13 years ago by mattimustang@…

Attachment: dojoselect.diff added

dojo.widget.Select

comment:17 Changed 13 years ago by mattimustang@…

In the attachment to follow i've added: cleaned up typos in comments. Added fadeTime parameter. In Select widget I now force selection of item from option list. If not from list the input gets cleared onBlur.

Changed 13 years ago by mattimustang@…

Attachment: dojoselect.2.diff added

updated patch

Changed 13 years ago by mattimustang@…

Attachment: dojoselect.3.diff added

fix clearing of _selected value in combobox.

comment:18 Changed 13 years ago by bill

Mathew, thanks for the patch! It all seems to work now. Closing this bug. Fixed in [4257].

comment:19 Changed 13 years ago by bill

Milestone: 0.40.3.1

comment:20 Changed 13 years ago by bill

Resolution: fixed
Status: reopenedclosed

comment:21 Changed 13 years ago by mattimustang@…

Resolution: fixed
Status: closedreopened

Updated patch cleans up some commented out code and fixes a reference to a null value in selectOption().

Changed 13 years ago by mattimustang@…

Attachment: dojoselect.4.diff added

updated patch

Changed 13 years ago by mattimustang@…

Attachment: dojoselect.5.diff added

removed patch to test_Select.html

comment:22 Changed 13 years ago by joose@…

myWidget.setSelectedValue(value); does not update widget.

Say I have select-options with id/label pairs like {1,one}, {2,two} .. etc.. And I choose three.

Then I push button which has like this action: "myWidget.setSelectedValue("1");"

The value in myWidget still shows three.

comment:23 Changed 13 years ago by bill

Resolution: fixed
Status: reopenedclosed

OK, new patch checked in as [4277].

Matthew - not sure about Joose's comment. I don't expect people to use setSelectedValue() w/ComboBox at all, but on the other hand it's reasonable to call setValue() on the Select widget and have the label updated, in addition to the value itself.

comment:24 Changed 12 years ago by (none)

Milestone: 0.3.1

Milestone 0.3.1 deleted

Note: See TracTickets for help on using tickets.