Bug 86719 – Submit button doesn't submit the form if the form is wrapped by an anchor tag

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Zoltan Tolnai]

In every non-webkit browser, if you click on the submit button, the browser sends the form.
In WebKit, the browser goes to the anchor's url.

[Alexey Proskuryakov]

Created attachment 142515 [details]
test case

Same test as attachment.

[Pablo Flouret]

Created attachment 143366 [details]
Patch

[Pablo Flouret]

Comment on attachment 143366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143366&action=review

> Source/WebCore/html/HTMLInputElement.cpp:1069
> +            if (evt->underlyingEvent())
> +                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.

[Ryosuke Niwa]

Comment on attachment 143366 [details]
Patch

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

[Pablo Flouret]

(In reply to comment #4)
> (From update of attachment 143366 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143366&action=review
> 
> >> Source/WebCore/html/HTMLInputElement.cpp:1069
> >> +                evt->underlyingEvent()->setDefaultHandled();
> 
> 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.

When the DOMActivate event is actually handled the parent default handlers are not called, so it never reaches Node::defaultEventHandler. Are you suggesting to call the parent class before returning, or not returning and making sure the parent is indeed called (it's not in HTMLInputElement's case, the code makes sure the event wasn't handled before calling the parent), or something else and i misunderstood?

[Ryosuke Niwa]

(In reply to comment #5)
> When the DOMActivate event is actually handled the parent default handlers are not called, so it never reaches Node::defaultEventHandler. Are you suggesting to call the parent class before returning, or not returning and making sure the parent is indeed called (it's not in HTMLInputElement's case, the code makes sure the event wasn't handled before calling the parent), or something else and i misunderstood?

No, I'm talking about where DOMActivate event is dispatched in the default event handler of click event.

[Pablo Flouret]

Created attachment 143687 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 143687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143687&action=review

The code change looks good. We need more testing.

> LayoutTests/ChangeLog:9
> +        * fast/forms/form-in-anchor-controls-activation-expected.txt: Added.
> +        * fast/forms/form-in-anchor-controls-activation.html: Added.

Please add a test for non-form elements.

> LayoutTests/fast/forms/form-in-anchor-controls-activation.html:10
> +window.onload = function () {

Do we really need to wait until the load event fires? Can't we move this code to the script element below?

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

You should delete this code.

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

Why do we need this script element?

[Pablo Flouret]

Created attachment 143901 [details]
Patch

[Pablo Flouret]

(In reply to comment #9)
> Created an attachment (id=143901) [details]
> Patch

Beside the cruft in the test, i had tested the new patch with the wrong build. It was crashing and didn't work correctly for HTMLButtonElements, so i fixed that as well in this patch. I shouldn't post patches when my brain is fried.

[Darin Adler]

Comment on attachment 143901 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143901&action=review

> Source/WebCore/dom/Node.cpp:2784
> +    dispatchScopedEvent(event.get());

This can just be event, no need for event.get().

> Source/WebCore/dom/Node.cpp:2863
> +        if (dispatchDOMActivateEvent(detail, event))
> +            event->setDefaultHandled();

What about other callers of dispatchDOMActivateEvent? Is this the only call site that needs this change?

[Pablo Flouret]

(In reply to comment #11)
> (From update of attachment 143901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143901&action=review
> > Source/WebCore/dom/Node.cpp:2863
> > +        if (dispatchDOMActivateEvent(detail, event))
> > +            event->setDefaultHandled();
> 
> What about other callers of dispatchDOMActivateEvent? Is this the only call site that needs this change?

Yeah, that's the only occurrence.

[Pablo Flouret]

Created attachment 143905 [details]
Patch for landing

event.get() -> event

[Ryosuke Niwa]

Comment on attachment 143905 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=143905&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Next time you're just asking for cq?, please fill in the reviewer's name.
Otherwise the commit queue is going to reject the patch.

[Darin Adler]

(In reply to comment #12)
> Yeah, that's the only occurrence.

If that’s the only caller, then I think we should make that function private.

[WebKit Review Bot]

Comment on attachment 143905 [details]
Patch for landing

Clearing flags on attachment: 143905

Committed r118442: <http://trac.webkit.org/changeset/118442>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.