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

RESOLVED FIXED in mozilla2.0b9

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

({dev-doc-complete, html5})

Trunk
mozilla2.0b9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

9 years ago
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
Assignee

Comment 1

9 years ago
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.
Assignee

Comment 2

9 years ago
Posted 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)
Assignee

Comment 3

9 years ago
Posted 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)
Assignee

Updated

9 years ago
Depends on: 615764
Assignee

Updated

9 years ago
Whiteboard: [passed-try][needs-review]
Assignee

Comment 4

9 years ago
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+
Assignee

Comment 6

9 years ago
(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.
Assignee

Updated

9 years ago
Attachment #494230 - Flags: approval2.0?
Assignee

Comment 7

9 years ago
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]
Assignee

Comment 8

9 years ago
Jonas, can you have a look at comment 6 and at the approval request?
Assignee

Comment 9

9 years ago
(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.
Assignee

Comment 10

9 years ago
Posted patch Patch v1.1Splinter Review
r=sicking
Attachment #494230 - Attachment is obsolete: true
Attachment #497551 - Flags: approval2.0?
Attachment #494230 - Flags: approval2.0?
Assignee

Updated

9 years ago
Whiteboard: [passed-try][needs-approval] → [passed-try][needs-landing]
Assignee

Comment 11

9 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/ea00eee207e8
Status: ASSIGNED → RESOLVED
Closed: 9 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?
Assignee

Comment 13

9 years ago
(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!
Assignee

Comment 15

9 years ago
(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 :)
You need to log in before you can comment on or make changes to this bug.