[Webkit-unassigned] [Bug 86719] Submit button doesn't submit the form if the form is wrapped by an anchor tag

[bugzilla-daemon at webkit.org]

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


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #143366|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org>  2012-05-22 22:48:46 PST ---
(From update of attachment 143366)
View in context: https://bugs.webkit.org/attachment.cgi?id=143366&action=review

This looks like an improvement but I'd like to see one more iteration.

> Source/WebCore/html/HTMLButtonElement.cpp:110
>      if (event->type() == eventNames().DOMActivateEvent && !disabled()) {
> -        if (form() && m_type == SUBMIT) {
> -            m_isActivatedSubmit = true;
> -            form()->prepareForSubmission(event);
> -            m_isActivatedSubmit = false; // Do this in case submission was canceled.
> +        if (form() && (m_type == SUBMIT || m_type == RESET)) {

Can we just combine these if statements?

>> Source/WebCore/html/HTMLInputElement.cpp:1069
>> +                evt->underlyingEvent()->setDefaultHandled();
> 
> I wasn't sure if there's a need to check for the type of the underlying event, so i didn't, but let me know if it could cause problems.

Why don't we do this instead in Node::defaultEventHandler? It appears to me canceling the default action of DOMActivate event should always cancel the default action of click event regardless of which element we're firing the event at.

> LayoutTests/fast/forms/form-in-anchor-controls-activation.html:4
> +    if (window.layoutTestController)
> +        layoutTestController.dumpAsText();

No need to indent script contents like this.

> LayoutTests/fast/forms/form-in-anchor-controls-activation.html:6
> +    function log(s) { document.querySelector("pre").innerHTML += s + "\n"; }

Please don't use one-letter variable like s.

> LayoutTests/fast/forms/form-in-anchor-controls-activation.html:11
> +        document.forms[0].elements.t.value = "blah";

Please don't use one-letter name like "t".

> LayoutTests/fast/forms/form-in-anchor-controls-activation.html:14
> +            var e = document.forms[0].children[i];

Ditto about one letter variable name.

> LayoutTests/fast/forms/form-in-anchor-controls-activation.html:20
> +            (function (element) {
> +                element.addEventListener("click", function () {
> +                    log("Activated " + element + " type=" + element.type);
> +                }, false);
> +            })(e);

Instead of wrapping in a closure like this to preserve element, you can use "this" instead.

> LayoutTests/fast/forms/form-in-anchor-controls-activation.html:32
> +            eventSender.mouseMoveTo(e.offsetLeft+3, e.offsetTop+3);

Spaces before and after +. Also why is that 3? It's probably safer to compute the center and click there.

> LayoutTests/fast/forms/form-in-anchor-controls-activation.html:55
> +        <button >button</button>

What's up with the space between button and >?

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