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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34930


Darin Adler <darin at apple.com> changed:

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




--- Comment #15 from Darin Adler <darin at apple.com>  2010-04-06 22:28:47 PST ---
(From update of attachment 52691)
Getting better, but I still think this needs work.

> +     if (needsDefaultAction && unhandledInvalidControls && inDocument())
> +         unhandledInvalidControls->append(this);

Besides checking inDocument() I think you might want to check that it's still
in the same document, and hasn't been moved into another.

> +    // Interactive validation must be done before dispatching the submit event.
> +    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);
> +    }

Seems like all that logic could go after the !noValidate() check, inside an if
statement.

> +                ASSERT(unhandled->hasOneRef());

This assertion is wrong. How can you be sure this has exactly one ref? I think
perhaps you misunderstand the meaning of the hasOneRef function. There's really
no way to assert that the pointer is in a RefPtr, but there's no need to assert
that either.

> +                if (unhandled->isFocusable()) {
> +                    unhandled->scrollIntoViewIfNeeded(false);
> +                    // scrollIntoViewIfNeeded() dispatches events, so the state
> +                    // of 'unhandled' might be changed.
> +                    if (unhandled->isFocusable()) {
> +                        unhandled->focus();
> +                        break;
> +                    }
> +                }

I think you should go further and say "the state of 'unhandled' might be
changed so it's no longer focusable."

But further, is isFocusable the only check you need to make? Don't we need to
check other things like inDocument, too? Probably also need to check it's still
in the same document and hasn't been moved into another.

> +            // Warn about all of unforcusable controls.

Typo: "unforcusable".

> +bool HTMLFormElement::checkValidity(Vector<RefPtr<HTMLFormControlElement> >& unhandledInvalidControls)
> +{
> +    RefPtr<HTMLFormElement> protector(this);
>      for (unsigned i = 0; i < formElements.size(); ++i) {
>          HTMLFormControlElement* control = formElements[i];
> +        control->checkValidity(&unhandledInvalidControls);
>      }
> +    return unhandledInvalidControls.isEmpty();
>  }

The name "checkValidity" doesn't seem quite right for this function. I would
call it something more like collectUnhandledInvalidControls. I also think
there's no need to have it return a boolean. The caller can check if the vector
is empty or not.

I don't understand, though, why these are "unhandled invalid controls" rather
than just "invalid controls". What are "handled invalid controls"?

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