[Webkit-unassigned] [Bug 34930] Implement interactive validation for forms
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 6 18:06:22 PDT 2010
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.
More information about the webkit-unassigned
mailing list