[webkit-reviews] review denied: [Bug 86719] Submit button doesn't submit the form if the form is wrapped by an anchor tag : [Attachment 143366] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Pablo Flouret
<pablof at motorola.com>'s request for review:
Bug 86719: Submit button doesn't submit the form if the form is wrapped by an
anchor tag
https://bugs.webkit.org/show_bug.cgi?id=86719

Attachment 143366: Patch
https://bugs.webkit.org/attachment.cgi?id=143366&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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 >?