Bug 227872

Summary:

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

Product:

WebKit

Reporter:

Tim Nguyen (:ntim) <ntim>

Component:

DOM

Assignee:

Tim Nguyen (:ntim) <ntim>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, changseok, esprehn+autocc, ews-watchlist, gyuyoung.kim, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

227986

Bug Blocks:

84635

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Tim Nguyen (:ntim)

Reported 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.

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
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2021-07-12 05:31:43 PDT

<rdar://problem/80459445>

Tim Nguyen (:ntim)

Comment 2 2021-07-13 09:44:50 PDT

Created attachment 433412 [details] Patch

Chris Dumez

Comment 3 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.

Chris Dumez

Comment 4 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.

Tim Nguyen (:ntim)

Comment 5 2021-07-13 11:34:24 PDT

Created attachment 433432 [details] Patch

Tim Nguyen (:ntim)

Comment 6 2021-07-13 11:35:14 PDT

Created attachment 433433 [details] Patch

Tim Nguyen (:ntim)

Comment 7 2021-07-13 11:56:09 PDT

Created attachment 433438 [details] Patch

Tim Nguyen (:ntim)

Comment 8 2021-07-13 12:04:42 PDT

Created attachment 433439 [details] Patch