[Webkit-unassigned] [Bug 34930] Implement interactive validation for forms

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 9 09:38:28 PDT 2010


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
  Attachment #52900|review?                     |review-
               Flag|                            |

--- Comment #20 from Darin Adler <darin at apple.com>  2010-04-09 09:38:27 PST ---
(From update of attachment 52900)
Basics look great here, but the edge cases are still not quite right.

> +description('Test if the form is submitted when an "invalid" evnet for a control is canceled.');

Typo: "evnet"

> +    Document* originalDocument = document();

This should be a RefPtr. I don't know what guarantees the document will still
be around after dispatching the event in pathological cases. There there may be
some subtle guarantee I can't spot, but I think it would be best to not make
the code depend on something that subtle when using a RefPtr would eliminate
the risk.

Also, the document == originalDocument check does not replace the inDocument
check. I'm not sure why you removed the inDocument check. That covers elements
that are no longer in the document tree. I think you should have a test case
that covers that and restore the inDocument check.

> +    HTMLFormControlElement* submitElement = 0;
> +    Node* targetNode = event->target()->toNode();
> +    if (targetNode && targetNode->isElementNode()) {
> +        Element* targetElement = static_cast<Element*>(targetNode);
> +        if (targetElement->isFormControlElement())
> +            submitElement = static_cast<HTMLFormControlElement*>(targetElement);
> +    }

Breaking this out into a helper function would make the code easier to read.
Just a suggestion -- the function this code is in is getting a bit long.

> +            Document* originalDocument = unhandled->document();

May need to be a RefPtr for the same reason as above. Same inDocument issue as

> +    Frame* frame = document()->frame();
> +    for (unsigned i = 0; frame && i < unhandledInvalidControls.size(); ++i) {

Doesn't make sense to have "frame" in the loop condition. The value of "frame"
does not change as we iterate through the loop.

> +    RefPtr<HTMLFormElement> protector(this);
> +    for (unsigned i = 0; i < formElements.size(); ++i)
> +        formElements[i]->checkValidity(&unhandledInvalidControls);
>  }

The protector here doesn't seem to cover all the issues. The code iterates the
form elements vector while it may be changing. This means that we could end up
iterating the same element more than once or skipping an element if an element
is removed or added earlier in the vector than where we are in the iteration.
We should figure out how we want to handle cases where the event dispatch adds
or removes elements from the form. One approach would be to copy the elements
into a Vector<RefPtr<...>> before we begin and iterate that.

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list