<dialog> element: do not perform close() method steps when removing o…

…pen attribute.

https://bugs.webkit.org/show_bug.cgi?id=227872

Reviewed by Chris Dumez.

Test: web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-open.html

The close function now follows the steps at: https://html.spec.whatwg.org/multipage/interactive-elements.html#close-the-dialog

LayoutTests/imported/w3c:

* web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-open-expected.txt:

Source/WebCore:

* html/HTMLDialogElement.cpp:
(WebCore::HTMLDialogElement::show):
(WebCore::HTMLDialogElement::showModal):
(WebCore::HTMLDialogElement::close):
(WebCore::HTMLDialogElement::dispatchPendingEvent):
(WebCore::HTMLDialogElement::isOpen const): Deleted.
(WebCore::HTMLDialogElement::returnValue): Deleted.
(WebCore::HTMLDialogElement::setReturnValue): Deleted.
(WebCore::HTMLDialogElement::parseAttribute): Deleted.
(WebCore::HTMLDialogElement::isModal const): Deleted.
* html/HTMLDialogElement.h:


Canonical link: https://commits.webkit.org/239693@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@279950 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/imported/w3c/ChangeLog

@@ -1,3 +1,16 @@
+2021-07-15  Tim Nguyen  <ntim@apple.com>
+
+        <dialog> element: do not perform close() method steps when removing open attribute.
+        https://bugs.webkit.org/show_bug.cgi?id=227872
+
+        Reviewed by Chris Dumez.
+
+        Test: web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-open.html
+
+        The close function now follows the steps at: https://html.spec.whatwg.org/multipage/interactive-elements.html#close-the-dialog
+
+        * web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-open-expected.txt:
+
 2021-07-15  Tim Nguyen  <ntim@apple.com>
 
         Re-import html/semantics/interactive-elements/the-dialog-element WPT

LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-open-expected.txt

@@ -4,5 +4,5 @@ OK
 
 PASS On getting, the IDL open attribute must return true if the content open attribute is set, and false if it is absent.
 PASS On setting, the content open attribute must be removed if the IDL open attribute is set to false, and must be present if the IDL open attribute is set to true.
-FAIL On setting it to false, the close event should not be fired assert_unreached: close event should not be fired when just setting the open attribute Reached unreachable code
+PASS On setting it to false, the close event should not be fired
 

Source/WebCore/ChangeLog

@@ -1,3 +1,26 @@
+2021-07-15  Tim Nguyen  <ntim@apple.com>
+
+        <dialog> element: do not perform close() method steps when removing open attribute.
+        https://bugs.webkit.org/show_bug.cgi?id=227872
+
+        Reviewed by Chris Dumez.
+
+        Test: web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-open.html
+
+        The close function now follows the steps at: https://html.spec.whatwg.org/multipage/interactive-elements.html#close-the-dialog
+
+        * html/HTMLDialogElement.cpp:
+        (WebCore::HTMLDialogElement::show):
+        (WebCore::HTMLDialogElement::showModal):
+        (WebCore::HTMLDialogElement::close):
+        (WebCore::HTMLDialogElement::dispatchPendingEvent):
+        (WebCore::HTMLDialogElement::isOpen const): Deleted.
+        (WebCore::HTMLDialogElement::returnValue): Deleted.
+        (WebCore::HTMLDialogElement::setReturnValue): Deleted.
+        (WebCore::HTMLDialogElement::parseAttribute): Deleted.
+        (WebCore::HTMLDialogElement::isModal const): Deleted.
+        * html/HTMLDialogElement.h:
+
 2021-07-15  Jer Noble  <jer.noble@apple.com>
 
         REGRESSION (r279119?): [iOS] ASSERTION FAILED: !m_impl || !m_shouldEnableAssertions || m_impl->wasConstructedOnMainThread() == isMainThread() seen with 3 WebKitLegacy media API tests

Source/WebCore/html/HTMLDialogElement.cpp

@@ -53,27 +53,12 @@ HTMLDialogElement::~HTMLDialogElement()
     dialogCloseEventSender().cancelEvent(*this);
 }
 
-bool HTMLDialogElement::isOpen() const
-{
-    return m_isOpen;
-}
-
-const String& HTMLDialogElement::returnValue()
-{
-    return m_returnValue;
-}
-
-void HTMLDialogElement::setReturnValue(String&& returnValue)
-{
-    m_returnValue = WTFMove(returnValue);
-}
-
 void HTMLDialogElement::show()
 {
     // If the element already has an open attribute, then return.
     if (isOpen())
         return;
-    
+
     setBooleanAttribute(openAttr, true);
 }
 
@@ -89,21 +74,35 @@ ExceptionOr<void> HTMLDialogElement::showModal()
 
     setBooleanAttribute(openAttr, true);
 
-    document().addToTopLayer(*this);
     m_isModal = true;
 
+    // FIXME: Only add dialog to top layer if it's not already in it. (webkit.org/b/227907)
+    document().addToTopLayer(*this);
+
+    // FIXME: Add steps 8 & 9 from spec. (webkit.org/b/227537)
+
     return { };
 }
 
-void HTMLDialogElement::close(const String& returnValue)
+void HTMLDialogElement::close(const String& result)
 {
     if (!isOpen())
         return;
-    
+
     setBooleanAttribute(openAttr, false);
 
-    if (!returnValue.isNull())
-        m_returnValue = returnValue;
+    m_isModal = false;
+
+    if (!result.isNull())
+        m_returnValue = result;
+
+    // FIXME: Only remove dialog from top layer if it's inside it. (webkit.org/b/227907)
+    document().removeFromTopLayer(*this);
+
+    // FIXME: Add step 6 from spec. (webkit.org/b/227537)
+
+    dialogCloseEventSender().cancelEvent(*this);
+    dialogCloseEventSender().dispatchEventSoon(*this);
 }
 
 void HTMLDialogElement::dispatchPendingEvent(DialogEventSender* eventSender)
@@ -112,31 +111,4 @@ void HTMLDialogElement::dispatchPendingEvent(DialogEventSender* eventSender)
     dispatchEvent(Event::create(eventNames().closeEvent, Event::CanBubble::No, Event::IsCancelable::No));
 }
 
-void HTMLDialogElement::parseAttribute(const QualifiedName& name, const AtomString& value)
-{
-    if (name == openAttr) {
-        bool oldValue = m_isOpen;
-        m_isOpen = !value.isNull();
-
-        // Emit close event
-        if (oldValue != m_isOpen && !m_isOpen) {
-            if (m_isModal) {
-                document().removeFromTopLayer(*this);
-                m_isModal = false;
-            }
-
-            dialogCloseEventSender().cancelEvent(*this);
-            dialogCloseEventSender().dispatchEventSoon(*this);
-        }
-        return;
-    }
-
-    HTMLElement::parseAttribute(name, value);
-}
-
-bool HTMLDialogElement::isModal() const
-{
-    return m_isModal;
-}
-
 }

Source/WebCore/html/HTMLDialogElement.h

@@ -37,28 +37,25 @@ class HTMLDialogElement final : public HTMLElement {
 public:
     template<typename... Args> static Ref<HTMLDialogElement> create(Args&&... args) { return adoptRef(*new HTMLDialogElement(std::forward<Args>(args)...)); }
     ~HTMLDialogElement();
-
-    bool isOpen() const;
 
-    const String& returnValue();
-    void setReturnValue(String&&);
+    bool isOpen() const { return hasAttribute(HTMLNames::openAttr); }
+
+    const String& returnValue() const { return m_returnValue; }
+    void setReturnValue(String&& value) { m_returnValue = WTFMove(value); }
 
     void show();
     ExceptionOr<void> showModal();
     void close(const String&);
 
     void dispatchPendingEvent(DialogEventSender*);
 
-    bool isModal() const;
+    bool isModal() const { return m_isModal; };
 
 private:
     HTMLDialogElement(const QualifiedName&, Document&);
 
-    void parseAttribute(const QualifiedName&, const AtomString&) final;
-
     String m_returnValue;
     bool m_isModal { false };
-    bool m_isOpen { false };
 };
 
 } // namespace WebCore