343444 - Radio group in nsHTMLFormElement is buggy

Status: RESOLVED FIXED

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

Description

[Alfred Peng]

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."

Comment 1

[Boris Zbarsky [:bzbarsky]]

Not a layout issue.

Comment 2

[Johnny Stenback (:jst)]

Not a blocker, but would be good to get fixed for 1.9.

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

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.

Comment 4

[Johnny Stenback (:jst)]

From a quick glance here it seems like the local smaller fix for this would be ok.

Comment 5

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Fix HTMLFormElement::GetNextRadioButton to return only radio buttons

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!

Comment 6

[Johnny Stenback (: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!

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Fix HTMLFormElement::GetNextRadioButton to return only radio buttons

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 8

[Johnny Stenback (: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!

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Fix HTMLFormElement::GetNextRadioButton to return only radio buttons

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ef39ad6e70

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c9ef39ad6e70