REGRESSION: Can't submit a form with <input type=radio required>

• Component: Forms
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Steps to reproduce:

1. Open http://www.avianova.com/
2. Select "one way trip".
3. Select "From: Moscow-Sheremetyevo"
4. Select To: "Naberezhnye Chelny"
5. Select departure date, for example "July 3rd"
6. Press "Find flight" button", this will load next page.
7. Pick the date from the choices
8. Turn on "Agreement" checkbox.
9. Press "Continue" button.

Results: nothing happens in release builds, an assertion fails in debug:

ASSERTION FAILED: m_isValid == validity()->valid()
(WebCore/html/HTMLFormControlElement.cpp:353 virtual bool WebCore::HTMLFormControlElement::isValidFormControlElement())

This works in Safari 4.0.5

<rdar://problem/8078475>

[Kent Tamura]

Created attachment 58451 [details]
Patch

[Darin Adler]

Comment on attachment 58451 [details]
Patch

> +        Vector<RefPtr<Node> > buttons;
> +        form()->getNamedElements(name(), buttons);

Why doesn't this function put its output in a Vector<RefPtr<Element> > or a Vector<RefPtr<HTMLFormControlElement> >?

> +        RefPtr<NodeList> nodeList = document()->getElementsByName(name());
> +        if (nodeList) {

Why is the null check needed? Can getElementsByName return null?

Why use a NodeList? That is a data structure for the public DOM, but for internal use I would recommend just writing a loop. The getElementsByName function has no special algorithm and introduces unneeded overhead.

    for (Node* node = document(); node; node = node->traverseNextNode())

The above will do the same thing more efficiently than the NodeList code.

By the way, it worries me that we have to iterate every single item in the whole document every time updateCheckedRadioButtons is called on a radio button outside a form. Can't this get pathologically slow if there are a ton of radio buttons?

[Kent Tamura]

Created attachment 58462 [details]
Patch 2

[Kent Tamura]

Thank you for reviewing.  I updated the patch.

(In reply to comment #2)
> (From update of attachment 58451 [details])
> > +        Vector<RefPtr<Node> > buttons;
> > +        form()->getNamedElements(name(), buttons);
> 
> Why doesn't this function put its output in a Vector<RefPtr<Element> > or a Vector<RefPtr<HTMLFormControlElement> >?

It's not so easy to chenge getNamedElements() output type because it is created from HTMLCollection. I found HTMLFormElement::associatedElements() and the updated patch uses it.

> 
> > +        RefPtr<NodeList> nodeList = document()->getElementsByName(name());
> > +        if (nodeList) {

> Why use a NodeList? That is a data structure for the public DOM, but for internal use I would recommend just writing a loop. The getElementsByName function has no special algorithm and introduces unneeded overhead.
> 
>     for (Node* node = document(); node; node = node->traverseNextNode())
> 
> The above will do the same thing more efficiently than the NodeList code.

Ok, I change it to traverseNextNode().

> By the way, it worries me that we have to iterate every single item in the whole document every time updateCheckedRadioButtons is called on a radio button outside a form. Can't this get pathologically slow if there are a ton of radio buttons?

Of course it can be slow.  I added a FIXME comment.

[Darin Adler]

Comment on attachment 58462 [details]
Patch 2

> +            if (element->formControlType() != formControlType())
> +                continue;
> +            if (element->formControlName() != formControlName())
> +                continue;

I think you can use type() and name() on the right side of these if statements. Since we're doing this work for every element in the entire document I think you need to be careful to use the check that is most likely to fail first. That's probably the formControlName() check.

[Kent Tamura]

(In reply to comment #5)
> (From update of attachment 58462 [details])
> > +            if (element->formControlType() != formControlType())
> > +                continue;
> > +            if (element->formControlName() != formControlName())
> > +                continue;
> 
> I think you can use type() and name() on the right side of these if statements. Since we're doing this work for every element in the entire document I think you need to be careful to use the check that is most likely to fail first. That's probably the formControlName() check.

Ok, I changed the order and changed to type()/name().

Committed as http://trac.webkit.org/changeset/61060

[Alexey Proskuryakov]

*** Bug 40591 has been marked as a duplicate of this bug. ***