Bug 227872 - Do not handle open attribute removal the same as HTMLDialogElement::close()
Summary: Do not handle open attribute removal the same as HTMLDialogElement::close()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Nguyen (:ntim)
URL:
Keywords: InRadar
Depends on: 227986
Blocks: dialog-element
  Show dependency treegraph
 
Reported: 2021-07-12 05:30 PDT by Tim Nguyen (:ntim)
Modified: 2021-07-15 09:16 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.52 KB, patch)
2021-07-13 09:44 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (5.32 KB, patch)
2021-07-13 11:34 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (5.31 KB, patch)
2021-07-13 11:35 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (5.54 KB, patch)
2021-07-13 11:56 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (5.55 KB, patch)
2021-07-13 12:04 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (7.72 KB, patch)
2021-07-15 06:16 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Nguyen (:ntim) 2021-07-12 05:30:47 PDT
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.
Comment 1 Radar WebKit Bug Importer 2021-07-12 05:31:43 PDT
<rdar://problem/80459445>
Comment 2 Tim Nguyen (:ntim) 2021-07-13 09:44:50 PDT
Created attachment 433412 [details]
Patch
Comment 3 Chris Dumez 2021-07-13 10:25:54 PDT
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.
Comment 4 Chris Dumez 2021-07-13 10:40:02 PDT
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.
Comment 5 Tim Nguyen (:ntim) 2021-07-13 11:34:24 PDT
Created attachment 433432 [details]
Patch
Comment 6 Tim Nguyen (:ntim) 2021-07-13 11:35:14 PDT
Created attachment 433433 [details]
Patch
Comment 7 Tim Nguyen (:ntim) 2021-07-13 11:56:09 PDT
Created attachment 433438 [details]
Patch
Comment 8 Tim Nguyen (:ntim) 2021-07-13 12:04:42 PDT
Created attachment 433439 [details]
Patch
Comment 9 Tim Nguyen (:ntim) 2021-07-15 06:16:45 PDT
Created attachment 433580 [details]
Patch
Comment 10 EWS 2021-07-15 09:16:50 PDT
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].