Bug 227872 – Do not handle open attribute removal the same as HTMLDialogElement::close()

• Component: DOM
• Status: RESOLVED FIXED

[Tim Nguyen (:ntim)]

Right now, removing the open attribute manually is handled the same as calling HTMLDialogElement::close().

However, the spec: https://html.spec.whatwg.org/multipage/interactive-elements.html#the-dialog-element:event-close explicitly says this is wrong.

Only the close() method should switch the state to closed, unblock the document from the active modal dialog and fire the close event.

[Radar WebKit Bug Importer]

<rdar://problem/80459445>

[Tim Nguyen (:ntim)]

Created attachment 433412 [details]
Patch

[Chris Dumez]

Comment on attachment 433412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433412&action=review

> Source/WebCore/ChangeLog:3
> +        <dialog> element: do not perform close() method steps when removing open attribute.

Missing tests.

[Chris Dumez]

Comment on attachment 433412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433412&action=review

r- due to lack of testing.

> Source/WebCore/html/HTMLDialogElement.cpp:64
> +    // TODO(webkit.org/b/227537): Add steps 3 & 4

No TODO comments in WebKit

> Source/WebCore/html/HTMLDialogElement.cpp:81
> +    // FIXME(webkit.org/b/227907)

Bad format, please see https://webkit.org/code-style-guidelines/#comments.

> Source/WebCore/html/HTMLDialogElement.cpp:84
> +    // TODO(webkit.org/b/227537): Add steps 8 & 9

No TODO comments in WebKit: https://webkit.org/code-style-guidelines/#comments

> Source/WebCore/html/HTMLDialogElement.cpp:89
> +void HTMLDialogElement::close(const String& result)

Please explain in the change log the changes to this function as they seem non-trivial.

> Source/WebCore/html/HTMLDialogElement.cpp:101
> +    // FIXME(webkit.org/b/227907)

This is not how we write FIXME comments: https://webkit.org/code-style-guidelines/#comments. Please write a proper sentence ending with a period.

> Source/WebCore/html/HTMLDialogElement.cpp:104
> +    // TODO(webkit.org/b/227537): Add step 6

No TODO comments in WebKit: https://webkit.org/code-style-guidelines/#comments.

> Source/WebCore/html/HTMLDialogElement.h:43
> +    const String& returnValue() { return m_returnValue; }

should be marked const.

[Tim Nguyen (:ntim)]

Created attachment 433432 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 433433 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 433438 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 433439 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 433580 [details]
Patch

[EWS]

Committed r279950 (239693@main): <https://commits.webkit.org/239693@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433580 [details].