Commit b59edaa6 authored by Joey Arhar's avatar Joey Arhar Committed by Commit Bot

Fix submit buttons with children that get removed

If a <button> element inside a <form> has a child element which is
clicked, and the <button>'s onclick handler removes the child element
from the DOM, the <button>'s default event handler which submits the
form is never called.

If this happens, and the <button>'s onclick handler also manually
submits the form via form.submit(), then the form double-submit fix made
in crrev.com/c/1850358 will prevent form.submit() from submitting the
form because the form.submit() is blocked until the <button> gets the
DOMActivate event, which never happens if the child element is removed
from the DOM before it can emit the DOMActivate event to the parent
<button>.

This patch will submit the form in this case by listening to the click
event instead of the DOMActivate event. If the click event is
additionally preventDefault()ed, this patch will not cover that case.

Once we switch back to async form submission in crbug.com/1013385 then
all of the cases will be covered and this patch will be reverted.

Bug: 1037740
Change-Id: I04cdff451b363b5fafa26d2c58ec0aa76ed95a0f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1983547Reviewed-by: default avatarKent Tamura <tkent@chromium.org>
Reviewed-by: default avatarMason Freed <masonfreed@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728662}
parent 06087655
...@@ -103,6 +103,7 @@ Event::Event(const AtomicString& event_type, ...@@ -103,6 +103,7 @@ Event::Event(const AtomicString& event_type,
legacy_did_listeners_throw_flag_(false), legacy_did_listeners_throw_flag_(false),
fire_only_capture_listeners_at_target_(false), fire_only_capture_listeners_at_target_(false),
fire_only_non_capture_listeners_at_target_(false), fire_only_non_capture_listeners_at_target_(false),
copy_event_path_from_underlying_event_(false),
handling_passive_(PassiveMode::kNotPassiveDefault), handling_passive_(PassiveMode::kNotPassiveDefault),
event_phase_(0), event_phase_(0),
current_target_(nullptr), current_target_(nullptr),
...@@ -292,7 +293,9 @@ void Event::SetUnderlyingEvent(Event* ue) { ...@@ -292,7 +293,9 @@ void Event::SetUnderlyingEvent(Event* ue) {
} }
void Event::InitEventPath(Node& node) { void Event::InitEventPath(Node& node) {
if (!event_path_) { if (copy_event_path_from_underlying_event_) {
event_path_ = underlying_event_->GetEventPath();
} else if (!event_path_) {
event_path_ = MakeGarbageCollected<EventPath>(node, this); event_path_ = MakeGarbageCollected<EventPath>(node, this);
} else { } else {
event_path_->InitializeWith(node, this); event_path_->InitializeWith(node, this);
......
...@@ -292,6 +292,10 @@ class CORE_EXPORT Event : public ScriptWrappable { ...@@ -292,6 +292,10 @@ class CORE_EXPORT Event : public ScriptWrappable {
legacy_did_listeners_throw_flag_ = true; legacy_did_listeners_throw_flag_ = true;
} }
void SetCopyEventPathFromUnderlyingEvent() {
copy_event_path_from_underlying_event_ = true;
}
// In general, event listeners do not run when related execution contexts are // In general, event listeners do not run when related execution contexts are
// paused. However, when this function returns true, event listeners ignore // paused. However, when this function returns true, event listeners ignore
// the pause and run. // the pause and run.
...@@ -343,6 +347,8 @@ class CORE_EXPORT Event : public ScriptWrappable { ...@@ -343,6 +347,8 @@ class CORE_EXPORT Event : public ScriptWrappable {
unsigned fire_only_capture_listeners_at_target_ : 1; unsigned fire_only_capture_listeners_at_target_ : 1;
unsigned fire_only_non_capture_listeners_at_target_ : 1; unsigned fire_only_non_capture_listeners_at_target_ : 1;
unsigned copy_event_path_from_underlying_event_ : 1;
PassiveMode handling_passive_; PassiveMode handling_passive_;
uint8_t event_phase_; uint8_t event_phase_;
probe::AsyncTaskId async_task_id_; probe::AsyncTaskId async_task_id_;
......
...@@ -51,6 +51,7 @@ ...@@ -51,6 +51,7 @@
#include "third_party/blink/renderer/core/dom/events/event_dispatch_forbidden_scope.h" #include "third_party/blink/renderer/core/dom/events/event_dispatch_forbidden_scope.h"
#include "third_party/blink/renderer/core/dom/events/event_dispatcher.h" #include "third_party/blink/renderer/core/dom/events/event_dispatcher.h"
#include "third_party/blink/renderer/core/dom/events/event_listener.h" #include "third_party/blink/renderer/core/dom/events/event_listener.h"
#include "third_party/blink/renderer/core/dom/events/event_path.h"
#include "third_party/blink/renderer/core/dom/flat_tree_node_data.h" #include "third_party/blink/renderer/core/dom/flat_tree_node_data.h"
#include "third_party/blink/renderer/core/dom/flat_tree_traversal.h" #include "third_party/blink/renderer/core/dom/flat_tree_traversal.h"
#include "third_party/blink/renderer/core/dom/get_root_node_options.h" #include "third_party/blink/renderer/core/dom/get_root_node_options.h"
...@@ -2886,6 +2887,8 @@ DispatchEventResult Node::DispatchDOMActivateEvent(int detail, ...@@ -2886,6 +2887,8 @@ DispatchEventResult Node::DispatchDOMActivateEvent(int detail,
GetDocument().domWindow(), detail); GetDocument().domWindow(), detail);
event.SetUnderlyingEvent(&underlying_event); event.SetUnderlyingEvent(&underlying_event);
event.SetComposed(underlying_event.composed()); event.SetComposed(underlying_event.composed());
if (!isConnected())
event.SetCopyEventPathFromUnderlyingEvent();
DispatchScopedEvent(event); DispatchScopedEvent(event);
// TODO(dtapuska): Dispatching scoped events shouldn't check the return // TODO(dtapuska): Dispatching scoped events shouldn't check the return
......
<!DOCTYPE html>
<meta charset="utf-8">
<link rel="author" title="Joey Arhar" href="mailto:jarhar@chromium.org">
<link rel="help" href="https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-2">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<iframe name=frame1 id=frame1></iframe>
<form id=form1 target=frame1 action="does_not_exist.html">
<button id=submitbutton type=submit>
<span id=outerchild>
<span id=innerchild>submit</span>
</span>
</button>
</form>
<script>
async_test(t => {
window.addEventListener('load', () => {
const frame1 = document.getElementById('frame1');
frame1.addEventListener('load', t.step_func_done(() => {}));
const submitButton = document.getElementById('submitbutton');
submitButton.addEventListener('click', event => {
document.getElementById('outerchild').remove();
document.getElementById('form1').submit();
});
document.getElementById('innerchild').click();
});
}, 'This test will pass if a form navigation successfully occurs when clicking a child element of a <button type=submit> element with a onclick event handler which removes the button\'s child and then calls form.submit().');
</script>
<!DOCTYPE html>
<meta charset="utf-8">
<link rel="author" title="Joey Arhar" href="mailto:jarhar@chromium.org">
<link rel="help" href="https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#form-submission-2">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<iframe name=frame1 id=frame1></iframe>
<form id=form1 target=frame1 action="does_not_exist.html">
<button id=submitbutton type=submit>
<span id=outerchild>
<span id=innerchild>submit</span>
</span>
</button>
</form>
<script>
async_test(t => {
window.addEventListener('load', () => {
const frame1 = document.getElementById('frame1');
frame1.addEventListener('load', t.step_func_done(() => {}));
const submitButton = document.getElementById('submitbutton');
submitButton.addEventListener('click', event => {
document.getElementById('outerchild').remove();
});
document.getElementById('innerchild').click();
});
}, 'This test will pass if a form navigation successfully occurs when clicking a child element of a <button type=submit> element with a onclick event handler which removes the button\'s child.');
</script>
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment