Closed Bug 610687 Opened 10 years ago Closed 10 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b9

People

(Reporter: mounir, Assigned: mounir)

References

Details

(Keywords: dev-doc-complete, html5)

Attachments

(1 file, 2 obsolete files)

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
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.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #494229 - Flags: review?(bzbarsky)
Attached patch Patch v1 (obsolete) — Splinter Review
I forgot to refresh the patch, yet another time...
Attachment #494229 - Attachment is obsolete: true
Attachment #494230 - Flags: review?(bzbarsky)
Attachment #494229 - Flags: review?(bzbarsky)
Depends on: 615764
Whiteboard: [passed-try][needs-review]
Comment on attachment 494230 [details] [diff] [review]
Patch v1

Moving the review to Jonas.
Attachment #494230 - Flags: review?(bzbarsky) → review?(jonas)
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.
Attachment #494230 - Flags: review?(jonas) → review+
(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.
Attachment #494230 - Flags: approval2.0?
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.
Whiteboard: [passed-try][needs-review] → [passed-try][needs-approval]
Jonas, can you have a look at comment 6 and at the approval request?
(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.
Attached patch Patch v1.1Splinter Review
r=sicking
Attachment #494230 - Attachment is obsolete: true
Attachment #497551 - Flags: approval2.0?
Attachment #494230 - Flags: approval2.0?
Whiteboard: [passed-try][needs-approval] → [passed-try][needs-landing]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/ea00eee207e8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [passed-try][needs-landing]
Target Milestone: --- → mozilla2.0b9
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?
(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.
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!
(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 :)
Depends on: 635008
You need to log in before you can comment on or make changes to this bug.