Closed Bug 343444 Opened 14 years ago Closed 6 years ago
Radio group in ns
HTMLForm Element is buggy
Track the problem raised by neil in https://bugzilla.mozilla.org/show_bug.cgi?id=315859#c6 "nsHTMLFormElement's idea of what a radio group is buggy - it includes all form controls with the same name, whether or not they are radio elements, and then GetNextRadioButton assumes that they're at least all <INPUT> elements."
Not a layout issue.
Assignee: nobody → general
Component: Layout: Form Controls → DOM: HTML
QA Contact: layout.form-controls → ian
Not a blocker, but would be good to get fixed for 1.9.
Flags: blocking1.9? → blocking1.9-
I'd like to work on this. Do you think it would be useful to have the same infrastructure as nsDocument have (maybe trying to avoid the duplication between nsDocument::GetNextRadioButton and HTMLFormElement::GetNextRadioButton). From what I see now nsDocument saves the list of Radio groups in a hashtable of struct nsRadioGroupStruct, we could do the same in HTMLFormElement? The actual bug in HTMLFormElement::GetNextRadioButton is that when checking the type with this if (radio->GetType() != NS_FORM_INPUT_RADIO) continue; } while (radio->Disabled() && radio != currentRadio); The code jumps to the condition radio->Disabled() && radio != currentRadio which is false for, say, an <input type="text"> which is not disabled. Using a flag like this isRadio = radio->GetType() != NS_FORM_INPUT_RADIO if (isRadio) continue; } while ((radio->Disabled() && radio != currentRadio) || !isRadio); would fix this problem, but I'm not sure if we are looking for a quick fix or the above infrastructure change. Thank you.
From a quick glance here it seems like the local smaller fix for this would be ok.
Cool. Side note, I'm not sure why this check is needed radio = HTMLInputElement::FromContentOrNull(radioGroup->Item(index)); if (!radio) continue; since we're already using radio regardless in here } while (radio->Disabled() && radio != currentRadio); Probably we can get rid of it (or check if radio is not null in while). Also, can I get this bug assigned so I can modify its parameters? thanks!
Attachment #8398626 - Flags: review?(jst)
Comment on attachment 8398626 [details] [diff] [review] Fix HTMLFormElement::GetNextRadioButton to return only radio buttons Sorry for the very long delay here, the last couple of weeks have been a bit challenging as you can no doubt imagine. radio = HTMLInputElement::FromContentOrNull(radioGroup->Item(index)); if (!radio) continue; Yes, this can be removed. - if (radio->GetType() != NS_FORM_INPUT_RADIO) + isRadio = radio->GetType() == NS_FORM_INPUT_RADIO; + if (!isRadio) continue; - } while (radio->Disabled() && radio != currentRadio); + } while ((radio->Disabled() && radio != currentRadio) || !isRadio); NS_IF_ADDREF(*aRadioOut = radio); return NS_OK; So, if we have a form with a bunch of radio controls followed by a non-radio control and currentRadio is the last radio control, then will we not also fail in that case even with this change? Seems we need to check after the loop as well that isRadio is true before we return non-null, or something along those lines. r- until that's sorted out. And please add tests for this stuff as well. Thanks!
Attachment #8398626 - Flags: review?(jst) → review-
I added a mochitest for this bug in the patch. I'm not sure I understand which case would cause problems here. Do you mean something like this <form> <input name="group" type="radio"> <input name="group" type="radio" id="start"> <input name="group" type="text"> </form> and currentRadio is "start"? It looks like this case is passing on my build. Please let me know if you mean something else. Thank you!
Comment on attachment 8409539 [details] [diff] [review] Fix HTMLFormElement::GetNextRadioButton to return only radio buttons You are correct! I missed the logic change in the while condition when looking through this and didn't realize we'll wrap around back to index zero in this loop. This looks good, r=jst. Thanks for adding the test!
I Pushed to try server the patch that jst reviewed here: https://tbpl.mozilla.org/?tree=Try&rev=6a38fe0b5ddf The B2G test that was failing was because this [test_button_attributes_reflection.html] [test_change_event.html] +[test_input_radio_radiogroup.html] skip-if = (buildapp == 'b2g' && toolkit != 'gonk') #Bug 931116, b2g desktop specific, initial triage was restoring test_input_radio_radiogroup.html on B2G that is a known fail I'm changing it to this [test_button_attributes_reflection.html] +[test_input_radio_radiogroup.html] [test_change_event.html] skip-if = (buildapp == 'b2g' && toolkit != 'gonk') #Bug 931116, b2g desktop specific, initial triage Pushed again here: https://tbpl.mozilla.org/?tree=Try&rev=728bf0391dc1 Looks like it's ready to land! Thank you.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.