Closed Bug 343444 Opened 14 years ago Closed 6 years ago

Radio group in nsHTMLFormElement is buggy

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: alfred.peng, Assigned: Agi)

Details

Attachments

(1 file, 2 obsolete files)

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
Flags: blocking1.9?
QA Contact: layout.form-controls → ian
Not a blocker, but would be good to get fixed for 1.9.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
Assignee: general → nobody
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.
Flags: needinfo?(alfred.peng)
Flags: needinfo?(alfred.peng) → needinfo?(jst)
From a quick glance here it seems like the local smaller fix for this would be ok.
Flags: needinfo?(jst)
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-
Assignee: nobody → agi.novanta
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!
Attachment #8398626 - Attachment is obsolete: true
Attachment #8409539 - Flags: feedback?(jst)
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!
Attachment #8409539 - Flags: review+
Attachment #8409539 - Flags: feedback?(jst)
Attachment #8409539 - Flags: feedback+
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.
Attachment #8409539 - Attachment is obsolete: true
Attachment #8413320 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c9ef39ad6e70
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.