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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Apr 6 12:42:12 PDT 2010


Darin Adler <darin at apple.com> changed:

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

--- Comment #11 from Darin Adler <darin at apple.com>  2010-04-06 12:42:11 PST ---
(From update of attachment 52630)
> -            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.

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

We should construct test cases with tricky event handlers for the invalid event
to see if we can reproduce these kinds of bad behavior. It's critical we not
introduce something that could be exploited as a security vulnerability.

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

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

> +            // Warn about all of unforcusable controls.

Typo: "unforcusable".

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

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

> +    return !unhandledInvalidControls.size();

I think this would read better if you used the isEmpty function.

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