Opened 11 years ago

Closed 11 years ago

Last modified 8 years ago

#6886 closed defect (fixed)

[patch][ccla]Radiobutton: ignores <form> boundaries

Reported by: digital.or@… Owned by: Douglas Hays
Priority: high Milestone: 1.2
Component: Dijit - Form Version: 1.1.1
Keywords: radiobutton form Cc:
Blocked By: Blocking:

Description

When 2 forms contain a group of radiobutton sharing the same name, checking a radiobutton in one form alters radiobutton status in the other form.

The problem appears on both version 1.1.1 and SVN. My browser is FF 2.0.0.4 and my OS is linux 2.6.23.

Example and patch included.

Attachments (3)

sample.html (647 bytes) - added by guest 11 years ago.
patch.checkbox_js (2.0 KB) - added by guest 11 years ago.
6886.patch (2.4 KB) - added by Douglas Hays 11 years ago.
Removed _groups in favor of dojo.query approach. Needs to be reviewed.

Download all attachments as: .zip

Change History (18)

Changed 11 years ago by guest

Attachment: sample.html added

Changed 11 years ago by guest

Attachment: patch.checkbox_js added

comment:1 Changed 11 years ago by bill

Cc: digital.or@… removed
Reporter: changed from guest to digital.or@…

Hmm, thanks for the patch but have you filed a CLA? We need a CLA before accepting patches.

Is this a common thing for radio buttons in separate forms to share the same name?

comment:2 Changed 11 years ago by guest

I have not send CLA because I have no printer at home. I send it from work tomorrow morning.

About radio button names, I don't really know. I just have encountered the problem in my webapp. I am using a template and I include it in several forms in the same page. I also noticed dojo.radiobuttons don't work as standard HTML radiobutton.

comment:3 Changed 11 years ago by bill

Milestone: 1.3
Owner: set to Douglas Hays
Summary: dijit.form.Radiobutton ignores <form> boundariesRadiobutton: ignores <form> boundaries

Changed 11 years ago by Douglas Hays

Attachment: 6886.patch added

Removed _groups in favor of dojo.query approach. Needs to be reviewed.

comment:4 Changed 11 years ago by Douglas Hays

Milestone: 1.31.2
Status: newassigned

comment:5 Changed 11 years ago by Douglas Hays

Summary: Radiobutton: ignores <form> boundaries[patch][ccla]Radiobutton: ignores <form> boundaries

comment:6 Changed 11 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

(In [14671]) Fixes #6886. Replaced _groups:{} and related code with a single dojo.query().

comment:7 Changed 11 years ago by Douglas Hays

(In [14672]) References #6886. Smarter dojo.query = less code.

comment:8 Changed 11 years ago by bill

Cool, looks good. I was worried about sub-classes of dijit.form.RadioButton? that might not be based on an <input type=radio> but I guess there aren't any.

comment:9 Changed 11 years ago by bill

(In [14677]) Tiny optimization, refs #6886.

comment:10 Changed 11 years ago by bill

(In [14678]) remove invalid comment, refs #6886.

comment:11 Changed 11 years ago by bill

Resolution: fixed
Status: closedreopened

Hmm, actually there are two problems with the current code, I think the change needs to be revisited.

The dojo.query syntax you are using:

INPUT:checked[type=radio][name='+this.name+']

isn't valid. The conditions need to come before :checked, like

INPUT[type=radio][name='+this.name+']:checked

... but that won't work either because dojo.query() is running too late, after the (previously) selected radio button has been deselected. In other words, INPUT:checked will only find the current widget.

The other problem is efficiency. On IE, the current code causes dojo.query() to call form.getElementByTagName("*") . (You can see this if you trace through the dojo.query() code execution.) This happens as part of the initialization of every dijit.form.RadioButton. You can make this better by changing the query to:

input[type=radio][name='+this.name+']

which will result in a form.getElementByTagName("input") call, but it's still a lot of overhead for initialization (none of this code is necessary for when RadioButton? is initializing, is it?)

In the worst case when the radio button *isn't* inside a form, it will scan the entire document, which is unacceptable performance.

comment:12 Changed 11 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

Fixed by [14685]

comment:13 Changed 11 years ago by dante

Resolution: fixed
Status: closedreopened

A couple of notes about [14685] -- Not sure it is worthwhile to require users to call .startup() on this single widget, when the only widgets that need it are layout widgets (this being the only form dijit that apparently needs .startup() called on it) ... but more importantly, at least for sake of consistency, this._inited should be called this._started, as is it everywhere else using a startup method. Also the startup method should always call the inherited startup method through the chain.

comment:14 Changed 11 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

(In [14689]) Fixes #6886. Changed to use postCreate instead of startup.

comment:15 Changed 8 years ago by bill

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