Close the dialog element when the open attribute is removed

This patch adds HTMLDialogElement::ParseAttribute which runs
HTMLDialogElement::close when the open attribute is removed to prevent a
bad state where the dialog is modal but hidden and inerting the rest of
the document.

Spec discussion is happening here:
whatwg/html#5802

Change-Id: Ib90736ced952d7aeadc791c6863c3ac2a55deb62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5269905
Reviewed-by: David Baron <dbaron@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1258629}

third_party/blink/renderer/core/html/html_dialog_element.cc

@@ -40,6 +40,7 @@
 #include "third_party/blink/renderer/core/html/forms/html_form_control_element.h"
 #include "third_party/blink/renderer/core/html/html_element.h"
 #include "third_party/blink/renderer/core/html/html_frame_owner_element.h"
+#include "third_party/blink/renderer/core/inspector/console_message.h"
 #include "third_party/blink/renderer/core/style/computed_style.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
 #include "third_party/blink/renderer/platform/instrumentation/use_counter.h"
@@ -137,11 +138,17 @@ HTMLDialogElement::HTMLDialogElement(Document& document)
   UseCounter::Count(document, WebFeature::kDialogElement);
 }
 
-void HTMLDialogElement::close(const String& return_value) {
+void HTMLDialogElement::close(const String& return_value,
+                              bool ignore_open_attribute) {
   // https://html.spec.whatwg.org/C/#close-the-dialog
+  if (is_closing_) {
+    return;
+  }
+  base::AutoReset<bool> reset_close(&is_closing_, true);
 
-  if (!FastHasAttribute(html_names::kOpenAttr))
+  if (!ignore_open_attribute && !FastHasAttribute(html_names::kOpenAttr)) {
     return;
+  }
 
   Document& document = GetDocument();
   HTMLDialogElement* old_modal_dialog = document.ActiveModalDialog();
@@ -413,4 +420,23 @@ void HTMLDialogElement::Trace(Visitor* visitor) const {
   HTMLElement::Trace(visitor);
 }
 
+void HTMLDialogElement::ParseAttribute(
+    const AttributeModificationParams& params) {
+  if (RuntimeEnabledFeatures::DialogCloseWhenOpenRemovedEnabled() &&
+      params.name == html_names::kOpenAttr && params.new_value.IsNull() &&
+      !is_closing_) {
+    auto* console_message = MakeGarbageCollected<ConsoleMessage>(
+        mojom::blink::ConsoleMessageSource::kOther,
+        mojom::blink::ConsoleMessageLevel::kWarning,
+        "The open attribute was removed from a dialog element while it was "
+        "open. This is not recommended. Please close it using the "
+        "dialog.close() method instead.");
+    console_message->SetNodes(GetDocument().GetFrame(), {GetDomNodeId()});
+    GetDocument().AddConsoleMessage(console_message);
+    close(/*return_value=*/String(), /*ignore_open_attribute=*/true);
+  }
+
+  HTMLElement::ParseAttribute(params);
+}
+
 }  // namespace blink

third_party/blink/renderer/core/html/html_dialog_element.h

@@ -44,7 +44,8 @@ class CORE_EXPORT HTMLDialogElement final : public HTMLElement {
 
   void Trace(Visitor*) const override;
 
-  void close(const String& return_value = String());
+  void close(const String& return_value = String(),
+             bool ignore_open_attribute = false);
   void show(ExceptionState&);
   void showModal(ExceptionState&);
   void RemovedFrom(ContainerNode&) override;
@@ -78,10 +79,15 @@ class CORE_EXPORT HTMLDialogElement final : public HTMLElement {
   static void SetFocusForDialogLegacy(HTMLDialogElement* dialog);
 
  private:
+  void ParseAttribute(const AttributeModificationParams&) override;
+
   void SetIsModal(bool is_modal);
   void ScheduleCloseEvent();
 
   bool is_modal_;
+  // is_closing_ is set to true at the beginning of close() and is reset to
+  // false after the call to close() finishes.
+  bool is_closing_ = false;
   String return_value_;
   WeakMember<Element> previously_focused_element_;
 

third_party/blink/renderer/platform/runtime_enabled_features.json5

@@ -1320,6 +1320,12 @@
       status: "experimental",
       base_feature: "none",
     },
+    {
+      // This feature makes the <dialog> element close properly when its "open"
+      // attribute is removed. https://github.com/whatwg/html/issues/5802
+      name: "DialogCloseWhenOpenRemoved",
+      status: "experimental",
+    },
     {
       name: "DialogNewFocusBehavior",
       status: "experimental",

third_party/blink/web_tests/external/wpt/html/semantics/interactive-elements/the-dialog-element/dialog-close-via-attribute.html

@@ -0,0 +1,59 @@
+<!DOCTYPE html>
+<link rel=author href="mailto:jarhar@chromium.org">
+<link rel=help href="https://github.com/whatwg/html/issues/5802">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/resources/testdriver.js"></script>
+<script src="/resources/testdriver-vendor.js"></script>
+
+<button>button</button>
+<dialog>hello world</dialog>
+
+<script>
+const dialog = document.querySelector('dialog');
+const button = document.querySelector('button');
+
+promise_test(async t => {
+  dialog.showModal();
+
+  let closeFired = false;
+  let cancelFired = false;
+  dialog.addEventListener('close', () => closeFired = true);
+  dialog.addEventListener('cancel', () => cancelFired = true);
+
+  dialog.removeAttribute('open');
+  await new Promise(resolve => t.step_timeout(resolve, 0));
+  await new Promise(requestAnimationFrame);
+
+  assert_false(dialog.matches(':modal'),
+    'The dialog should not match :modal after closing.');
+  assert_false(cancelFired,
+    'The cancel event should not fire when removing the open attribute.');
+  assert_true(closeFired,
+    'The close event should be fired when removing the open attribute.');
+
+  let buttonFiredClick = false;
+  button.addEventListener('click', () => buttonFiredClick = true);
+  await test_driver.click(button);
+  assert_true(buttonFiredClick,
+    'The page should not be inert or blocked after removing the open attribute.');
+}, 'Removing the open attribute from an open modal dialog should run the closing algorithm.');
+
+promise_test(async t => {
+  dialog.show();
+
+  let closeFired = false;
+  let cancelFired = false;
+  dialog.addEventListener('close', () => closeFired = true);
+  dialog.addEventListener('cancel', () => cancelFired = true);
+
+  dialog.removeAttribute('open');
+  await new Promise(resolve => t.step_timeout(resolve, 0));
+  await new Promise(requestAnimationFrame);
+
+  assert_false(cancelFired,
+    'The cancel event should not fire when removing the open attribute.');
+  assert_true(closeFired,
+    'The close event should be fired when removing the open attribute.');
+}, 'Removing the open attribute from an open non-modal dialog should fire a close event.');
+</script>

third_party/blink/web_tests/external/wpt/html/semantics/interactive-elements/the-dialog-element/dialog-showModal.html

@@ -161,11 +161,11 @@
     assert_equals(topElement(), d11);
 
     // Removing the open attribute and running through the showModal() algorithm
-    // again should not promote d10 to the top.
+    // again should promote d10 to the top.
     d10.removeAttribute("open");
     assert_equals(topElement(), d11);
     d10.showModal();
-    assert_equals(topElement(), d11);
+    assert_equals(topElement(), d10);
 
     // Closing d11 with close() should cause d10 to be the topmost element.
     d11.close();