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

[bugzilla-daemon at webkit.org]

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





--- Comment #12 from TAMURA, Kent <tkent at chromium.org>  2010-04-06 18:06:21 PST ---
Thank you for reviewing!
Your review is definitely helpful.

(In reply to comment #11)
> (From update of attachment 52630 [details])
> > -            form()->prepareSubmit(evt);
> > +            form()->prepareSubmit(evt, this);
> 
> Why is this needed? I would think that either event->target() or
> event->currentTarget() would give us the element.

You're right.  Removed the parameter.

> > -bool HTMLFormControlElement::checkValidity()
> > +bool HTMLFormControlElement::checkValidity(Vector<HTMLFormControlElement*>* unhandledInvalidControls)
> 
> This vector should hold RefPtr<HTMLFormControlElement> instead of a raw
> pointer. Dispatching to arbitrary JavaScript code could easily make these
> elements go away, so we can't just hold a raw pointer to them.

I see.  I have added a test which removed a node in an event handler, and
confirmed it caused a crash.
I changed it to RefPtr<HTMLFormControlElement>.

> > -        dispatchEvent(Event::create(eventNames().invalidEvent, false, true));
> > +        bool needsDefaultAction = dispatchEvent(Event::create(eventNames().invalidEvent, false, true));
> > +        if (needsDefaultAction && unhandledInvalidControls)
> > +            unhandledInvalidControls->append(this);
> 
> What guarantees that "this" is still good? I think it's possible that the event
> dispatch could remove the last reference to this form control element and so it
> could be deallocated by the time we return. Same comment as above.

Added protections for 'this' by RefPtr.

> > +                HTMLFormControlElement* unhandled = unhandledInvalidControls[i];
> > +                if (unhandled->isFocusable()) {
> > +                    unhandled->scrollIntoViewIfNeeded(false);
> > +                    unhandled->focus();
> > +                    break;
> > +                }
> 
> We should think through the case where this control is no longer in the
> document, or not visible.

Added another check for isFocusable() before focus().

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

Fixed.

> 
> > +            Frame* frame = document()->frame();
> 
> Scrolling and focusing may have triggered an event handler, which may have
> deleted this form element. So calling document() at this point may not work.
> Need to double check that we handle that case. Similar issue with the code
> later on that sets m_insubmit to false.

Added protection for the form element by RefPtr.


> > +            for (unsigned i = 0; frame && i < unhandledInvalidControls.size(); ++i) {
> > +                HTMLFormControlElement* unhandled = unhandledInvalidControls[i];
> > +                if (unhandled->isFocusable())
> > +                    continue;
> > +                String message("An invalid form control with name='%name' is not user-editable.");
> > +                message.replace("%name", unhandled->name());
> > +                frame->domWindow()->console()->addMessage(HTMLMessageSource, LogMessageType, ErrorMessageLevel, message, 0, document()->url().string());
> > +            }
> 
> I don't completely understand the goal here. You say "not user-editable" even
> though the code checks "isFocusable". Could you give a concrete exmple so I can
> understand why this is useful?

I changed "not user-editable" to "not focusable".  It was confusing.
HTML5 spec asks to warn about this case.  It is useful for page authors to
notify that users can not make this form valid.

> > +    return !unhandledInvalidControls.size();
> 
> I think this would read better if you used the isEmpty function.

Fixed.

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