The definition for WKBundlePageWillSendSubmitEventCallback is this: typedef void (*WKBundlePageWillSendSubmitEventCallback)(WKBundlePageRef page, WKBundleNodeHandleRef htmlFormElementHandle, WKBundleFrameRef frame, WKBundleFrameRef sourceFrame, WKDictionaryRef values, const void* clientInfo); It's clearly intended to parallel WKBundlePageWillSubmitFormCallback, since almost all the parameters are the same. Now, in WKBundlePageWillSubmitFormCallback, the first WKBundleFrameRef, "frame", is the frame of the form target, and the second WKBundleFrameRef, sourceFrame, is the frame containing the form. That's correct. But in WKBundlePageWillSendSubmitEventCallback, both frame and sourceFrame are always identical. The problem is that the FrameLoaderClient delegate is called on the wrong FrameLoaderClient. It should be called on the FrameLoaderClient of the target frame, but HTMLFormElement calls it on the FrameLoaderClient of the source frame instead. The forthcoming patch fixes this and adds a test. Since the change is in WebCore, it *technically* doesn't require owner approval, but it would certainly be good for someone from Apple to review it please.
Created attachment 320455 [details] Patch
I think it broke fast/forms/input-image-submit.html... waiting for the EWS results.
Comment on attachment 320455 [details] Patch Attachment 320455 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4514623 New failing tests: fast/forms/input-image-submit.html
Created attachment 320463 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 320455 [details] Patch Attachment 320455 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4514876 New failing tests: fast/forms/input-image-submit.html
Created attachment 320464 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 320465 [details] Patch
Comment on attachment 320465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320465&action=review Good fix but I have a suggestion to make the fix a bit cleaner. > Source/WebCore/html/HTMLFormElement.cpp:281 > + auto temporarySubmission = FormSubmission::create(*this, m_attributes, &event, LockHistory::Yes, NotSubmittedByJavaScript); I do not like this part. I would suggest: 1. Add an effectiveTarget() method to HTMLFormElement which uses the submitButton's formtarget attribute is available, falls back to HTMLFormElement::target() and then falls back to document().baseTarget(). 2. Use effectiveTarget() here instead of constructing a FormSubmission 3. Use effectiveTarget() in FormSubmission::create() to avoid code duplication.
Thanks for the review, Chris, and sorry for the delay. I think/hope I've properly implemented your suggestion. Let's see if it passes EWS.
Created attachment 324777 [details] Patch
Comment on attachment 324777 [details] Patch Attachment 324777 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4979053 New failing tests: fast/forms/formtarget-attribute-button-html.html fast/forms/formtarget-attribute-input-html.html fast/forms/formtarget-attribute-input-2.html
Created attachment 324784 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to Michael Catanzaro from comment #9) > Thanks for the review, Chris, and sorry for the delay. I think/hope I've > properly implemented your suggestion. Let's see if it passes EWS. Signs point to "no"
Comment on attachment 324777 [details] Patch Attachment 324777 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4979047 New failing tests: fast/forms/formtarget-attribute-button-html.html fast/forms/formtarget-attribute-input-html.html
Created attachment 324788 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 324777 [details] Patch Attachment 324777 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4979226 New failing tests: fast/forms/formtarget-attribute-button-html.html fast/forms/formtarget-attribute-input-html.html fast/forms/formtarget-attribute-input-2.html
Created attachment 324789 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 324777 [details] Patch Attachment 324777 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4984334 New failing tests: fast/forms/formtarget-attribute-button-html.html fast/forms/formtarget-attribute-input-html.html fast/forms/formtarget-attribute-input-2.html
Created attachment 324840 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 324891 [details] Patch
OK, looks better now. mac-debug is still orange, but the failures there appear to be unrelated.
Comment on attachment 324891 [details] Patch Attachment 324891 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4990660 Number of test failures exceeded the failure limit.
Created attachment 324928 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 324891 [details] Patch Seems the mac-wk2 EWS is having some unrelated problems with Apache.
Pinging cdumez :)
Comment on attachment 324891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324891&action=review r=me > Source/WebCore/html/HTMLFormElement.cpp:671 > + for (auto node = event->target()->toNode(); node; node = node->parentNode()) { assuming toNode() returns a raw pointer, I'd prefer auto* than auto. But really, I think this should use: auto* targetNode = event->target()->toNode(); return targetNode ? lineageOfType<HTMLFormControlElement>(*targetNode)).first() : nullptr;
Question! (In reply to Chris Dumez from comment #26) > Comment on attachment 324891 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=324891&action=review > > r=me > > > Source/WebCore/html/HTMLFormElement.cpp:671 > > + for (auto node = event->target()->toNode(); node; node = node->parentNode()) { > > assuming toNode() returns a raw pointer, I'd prefer auto* than auto. It's a RefPtr<Node>, so it has to be plain auto. > But > really, I think this should use: > auto* targetNode = event->target()->toNode(); > return targetNode ? > lineageOfType<HTMLFormControlElement>(*targetNode)).first() : nullptr; I failed to get this to work, because lineageOfType requires an Element, not a Node. I messed around a bit and came up with this: HTMLFormControlElement* HTMLFormElement::findSubmitButton(const Event* event) const { if (event && event->target()) { if (auto targetNode = event->target()->toNode()) { if (auto* element = downcast<Element>(targetNode.get())) return lineageOfType<HTMLFormControlElement>(*element).first(); } } return nullptr; } But that breaks fast/forms/form-submission-create-crash.xhtml: ASSERTION FAILED: !source || is<Target>(*source) ../../Source/WTF/wtf/TypeCasts.h(89) : typename WTF::match_constness<Source, Target>::type* WTF::downcast(Source*) [with Target = WebCore::Element; Source = WebCore::Node; typename WTF::match_constness<Source, Target>::type = WebCore::Element] Not sure what to do here. Is the existing version of the code OK?
Comment on attachment 324891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324891&action=review >>> Source/WebCore/html/HTMLFormElement.cpp:671 >>> + for (auto node = event->target()->toNode(); node; node = node->parentNode()) { >> >> assuming toNode() returns a raw pointer, I'd prefer auto* than auto. But really, I think this should use: >> auto* targetNode = event->target()->toNode(); >> return targetNode ? lineageOfType<HTMLFormControlElement>(*targetNode)).first() : nullptr; > > It's a RefPtr<Node>, so it has to be plain auto. Oh, it is true that lineageOfType() expects an element in parameter :/ Just leave as is then. You are merely moving this code anyway.
(In reply to Michael Catanzaro from comment #27) > Question! > > (In reply to Chris Dumez from comment #26) > > Comment on attachment 324891 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=324891&action=review > > > > r=me > > > > > Source/WebCore/html/HTMLFormElement.cpp:671 > > > + for (auto node = event->target()->toNode(); node; node = node->parentNode()) { > > > > assuming toNode() returns a raw pointer, I'd prefer auto* than auto. > > It's a RefPtr<Node>, so it has to be plain auto. > > > But > > really, I think this should use: > > auto* targetNode = event->target()->toNode(); > > return targetNode ? > > lineageOfType<HTMLFormControlElement>(*targetNode)).first() : nullptr; > > I failed to get this to work, because lineageOfType requires an Element, not > a Node. I messed around a bit and came up with this: > > HTMLFormControlElement* HTMLFormElement::findSubmitButton(const Event* > event) const > { > if (event && event->target()) { > if (auto targetNode = event->target()->toNode()) { > if (auto* element = downcast<Element>(targetNode.get())) > return > lineageOfType<HTMLFormControlElement>(*element).first(); Yes, this is an unsafe downcast as we don't know that the Node is an Element.
Does downcast not do any type checking? I guess that's why it's not dynamic_cast. Foolish me!
Committed r224206: <https://trac.webkit.org/changeset/224206>
<rdar://problem/35569037>