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

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from TAMURA, Kent <tkent at chromium.org>  2010-04-07 02:34:00 PST ---
(In reply to comment #15)
> > +     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.

Oh, I thought a node couldn't be moved to another document.  Right, Webkit
supports node adopting.
I added document() check.

> 
> > +    // 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.

Right.  Fixed.
And I moved the additional code in prepareSubmit() to another function.

> > +                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.

I misunderstood it was refCount() > 0.  Removed.

> > +                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.

Add document() check and updated the comment.
I don't think inDocument check is required because a focusable element must be
inDocument().

> > +            // Warn about all of unforcusable controls.
> 
> Typo: "unforcusable".

Oops, fixed.

> > +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 changed it to "void collectUnhandledInvalidControls()".

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

It means, invalid controls of which 'invalid' event was canceled are not
registered into the vector.
"Unhandled invalid controls" is a term in the HTML5 specification.
http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#constraint-validation

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