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.
Created attachment 142515 [details] test case Same test as attachment.
Created attachment 143366 [details] Patch
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.
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 >?
(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?
(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.
Created attachment 143687 [details] Patch
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?
Created attachment 143901 [details] Patch
(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.
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?
(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.
Created attachment 143905 [details] Patch for landing event.get() -> event
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.
(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.
Comment on attachment 143905 [details] Patch for landing Clearing flags on attachment: 143905 Committed r118442: <http://trac.webkit.org/changeset/118442>
All reviewed patches have been landed. Closing bug.