610687 - Make all the radio button group suffering from being missing (instead of only radio's with the required attribute)

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

Currently, when a radio button is required, it will suffer from being missing
if no radio elements in the radio button group is checked. However, radio
elements in the group will not suffer from being missing if they do not have
the required attribute. In other words, if you try to style invalid elements
with :invalid, and do:
<input type='radio' name='s' value='M' required>
<input type='radio' name='s' value='F'>
only the first element will be styled.

I think we should move the requirement to the radio button group that way: "The
radio button group suffers from being missing if one of the input elements in
the radio button group is required and all of them have a checkedness that is
false." and radio elements would have this constraint: "If the radio button
group is suffering from being missing, the element is suffering from being
missing.".

That way, all radio elements in the same radio button group will have the same
validity state. That would be less annoying for authors and error proof while
making things clearer (IMO).

See w3c bug:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=11212

Comment 1

[Mounir Lamouri (:mounir)]

To fix that I would have to update nsIRadioGroupContainer (to save the current state of the radio group). Would that worth doing this change in Gecko 2.0? It would only require adding one or two methods.

Another way would be to have all the logic in nsHTMLInputElement: each time a radio element requirement state is changed, it will check the group requirement state and inform all radios from the group.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Actually, it was a bad idea to try to add stuff to nsIRadioGroupContainer given that implementation would have been done in nsDocument.cpp and nsHTMLFormElement.cpp.
However, it might be interesting to save the current validity state in the group so we could prevent updates when not needed.

Another follow-up would consist of removing the visitor updating the value missing

Boris, feel free to move the review to Jonas or Olli.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I forgot to refresh the patch, yet another time...

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 494230 [details] [diff] [review]
Patch v1

Moving the review to Jonas.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 494230 [details] [diff] [review]
Patch v1

>diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
...
>   nsCOMPtr<nsIRadioGroupContainer> container = GetRadioGroupContainer();
>   if (container) {
>     if (!gotName) {
>       if (!GetNameIfExists(name)) {
>         // If the name doesn't exist, nothing is going to happen anyway
>         return;
>       }
>       gotName = PR_TRUE;
>     }
>+
>+    UpdateValueMissingValidityStateForRadio(true);
>+
>     container->RemoveFromRadioGroup(name,
>                                     static_cast<nsIFormControl*>(this));

Wouldn't it be easier to remove from the group first and then update the container? Rather than implementing an 'exclude' argument?

>@@ -3816,18 +3813,18 @@ nsHTMLInputElement::IsValueMissing()
>   }
> 
>   switch (mType)
>   {
>     case NS_FORM_INPUT_CHECKBOX:
>       return !GetChecked();
>     case NS_FORM_INPUT_RADIO:
>       {
>-        nsCOMPtr<nsIDOMHTMLInputElement> selected = GetSelectedRadioButton();
>-        return !selected;
>+        // We should only be there if the radio has no group.
>+        return !GetChecked();

Please assert that there is no group. Though wouldn't it work as well to always use the group code? It doesn't seem worth optimizing the group-less radio button case IMHO. I've seen next to no pages with just one radio button in a group.

If you are keeping this code, just combine the checkbox and radio branches.

>+nsIRadioVisitor*
>+NS_GetRadioGroupRequiredVisitor(nsIFormControl* aExcludeElement,
>+                                bool* aRequired)
>+{
>+  return new nsRadioGroupRequiredVisitor(aExcludeElement, aRequired);
>+}
>+
>+nsIRadioVisitor*
>+NS_SetRadioValueMissingState(nsIFormControl* aExcludeElement,
>+                             nsIDocument* aDocument,
>+                             bool aValidity, bool aNotify)
>+{
>+  return new nsRadioSetValueMissingState(aExcludeElement, aDocument, aValidity,
>+                                         aNotify);
>+}

No need for these factory functions, just create the objects directly where needed.

r=me with those things fixed.

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> Comment on attachment 494230 [details] [diff] [review]
> Patch v1
> 
> >diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
> ...
> >   nsCOMPtr<nsIRadioGroupContainer> container = GetRadioGroupContainer();
> >   if (container) {
> >     if (!gotName) {
> >       if (!GetNameIfExists(name)) {
> >         // If the name doesn't exist, nothing is going to happen anyway
> >         return;
> >       }
> >       gotName = PR_TRUE;
> >     }
> >+
> >+    UpdateValueMissingValidityStateForRadio(true);
> >+
> >     container->RemoveFromRadioGroup(name,
> >                                     static_cast<nsIFormControl*>(this));
> 
> Wouldn't it be easier to remove from the group first and then update the
> container? Rather than implementing an 'exclude' argument?

I guess if we remove the radio from the group, it will have no group, thus we wouldn't be able to update the group. Seem less annoying to add an exclude argument than a "use this group" argument.

Comment 7

[Mounir Lamouri (:mounir)]

Approval request: the way radios and required work today is weird. We asked for a change in the specs and it seems this is going to be accepted so it would be better to have a correct behavior in Firefox 4 for web sanity. The risks are small given that this is only changing input/radio code so any regression would be easy to catch.

Comment 8

[Mounir Lamouri (:mounir)]

Jonas, can you have a look at comment 6 and at the approval request?

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> >@@ -3816,18 +3813,18 @@ nsHTMLInputElement::IsValueMissing()
> >   }
> > 
> >   switch (mType)
> >   {
> >     case NS_FORM_INPUT_CHECKBOX:
> >       return !GetChecked();
> >     case NS_FORM_INPUT_RADIO:
> >       {
> >-        nsCOMPtr<nsIDOMHTMLInputElement> selected = GetSelectedRadioButton();
> >-        return !selected;
> >+        // We should only be there if the radio has no group.
> >+        return !GetChecked();
> 
> Please assert that there is no group. Though wouldn't it work as well to always
> use the group code? It doesn't seem worth optimizing the group-less radio
> button case IMHO. I've seen next to no pages with just one radio button in a
> group.

I've updated the patch to have the group code taking into account radios with no group. I've also added tests for that.

> >+nsIRadioVisitor*
> >+NS_GetRadioGroupRequiredVisitor(nsIFormControl* aExcludeElement,
> >+                                bool* aRequired)
> >+{
> >+  return new nsRadioGroupRequiredVisitor(aExcludeElement, aRequired);
> >+}
> >+
> >+nsIRadioVisitor*
> >+NS_SetRadioValueMissingState(nsIFormControl* aExcludeElement,
> >+                             nsIDocument* aDocument,
> >+                             bool aValidity, bool aNotify)
> >+{
> >+  return new nsRadioSetValueMissingState(aExcludeElement, aDocument, aValidity,
> >+                                         aNotify);
> >+}
> 
> No need for these factory functions, just create the objects directly where
> needed.

I will do that in bug 586298.  I don't know why we have factories for all radio visitors but if I want to prevent to create them for these new ones, I will have to move a lot of code (because visitors are declared and defined in the end of the cpp file). Better to do that in another patch.

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

r=sicking

Comment 11

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/ea00eee207e8

Comment 12

[Eric Shepherd [:sheppy]]

I don't understand what needs to be documented here. The explanation of what this bug involves isn't clear to me. Can someone explain?

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to comment #12)
> I don't understand what needs to be documented here. The explanation of what
> this bug involves isn't clear to me. Can someone explain?

It depends of what was documented before.

The previous behavior was:
<input type='radio' name='a' required>
<input type='radio' name='a'>

If none of the radio was selected, the first one was invalid but not the second one. If one of the radio was selected (the first or the second) both radio were invalid.

Now, if none of the radio is selected, both radio are invalid.

If the previous behavior wasn't documented, I guess you can just skip that doc request.

Comment 14

[Eric Shepherd [:sheppy]]

Added a brief note about this here:

https://developer.mozilla.org/en/CSS/:invalid#Radio_buttons

Please let me know if I misunderstood the point to this bug. Thanks!

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to comment #14)
> Added a brief note about this here:
> 
> https://developer.mozilla.org/en/CSS/:invalid#Radio_buttons
> 
> Please let me know if I misunderstood the point to this bug. Thanks!

It's great. Thanks :)