From 69a2d95c36691021ecbb20b093fc2f813a023e6a Mon Sep 17 00:00:00 2001 From: Joey Arhar Date: Tue, 16 May 2023 21:50:37 +0000 Subject: [PATCH] Don't throw when popovers and dialogs are in requested state This is being changed in the HTML spec here: https://github.com/whatwg/html/pull/9142 Change-Id: Ib8aaaf314c2a1de5d082494e5172e029d531c8e8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4494823 Reviewed-by: Mason Freed Commit-Queue: Joey Arhar Cr-Commit-Position: refs/heads/main@{#1144952} --- .../renderer/core/html/html_dialog_element.cc | 41 +++++++++++++++---- .../blink/renderer/core/html/html_element.cc | 36 ++++++++-------- .../platform/runtime_enabled_features.json5 | 7 ++++ .../dialog-no-throw-requested-state.html | 29 +++++++++++++ .../popovers/popover-attribute-basic.html | 16 ++++++-- .../popovers/popover-light-dismiss.html | 7 +--- .../popovers/popover-move-documents.html | 33 +++++++++++---- .../popovers/resources/popover-utils.js | 6 +-- 8 files changed, 133 insertions(+), 42 deletions(-) create mode 100644 third_party/blink/web_tests/external/wpt/html/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state.html diff --git a/third_party/blink/renderer/core/html/html_dialog_element.cc b/third_party/blink/renderer/core/html/html_dialog_element.cc index ca1bcd950d7a14..1f1a0a08bd10a9 100644 --- a/third_party/blink/renderer/core/html/html_dialog_element.cc +++ b/third_party/blink/renderer/core/html/html_dialog_element.cc @@ -188,8 +188,21 @@ void HTMLDialogElement::ScheduleCloseEvent() { } void HTMLDialogElement::show(ExceptionState& exception_state) { - if (FastHasAttribute(html_names::kOpenAttr)) - return; + if (RuntimeEnabledFeatures::PopoverDialogDontThrowEnabled()) { + if (FastHasAttribute(html_names::kOpenAttr)) { + if (is_modal_) { + exception_state.ThrowDOMException( + DOMExceptionCode::kInvalidStateError, + "The dialog is already open as a modal dialog, and therefore " + "cannot be opened as a non-modal dialog."); + } + return; + } + } else { + if (FastHasAttribute(html_names::kOpenAttr)) { + return; + } + } if (RuntimeEnabledFeatures::HTMLPopoverAttributeEnabled( GetDocument().GetExecutionContext()) && @@ -247,12 +260,24 @@ class DialogCloseWatcherEventListener : public NativeEventListener { }; void HTMLDialogElement::showModal(ExceptionState& exception_state) { - if (FastHasAttribute(html_names::kOpenAttr)) { - return exception_state.ThrowDOMException( - DOMExceptionCode::kInvalidStateError, - "The element already has an 'open' " - "attribute, and therefore cannot be " - "opened modally."); + if (RuntimeEnabledFeatures::PopoverDialogDontThrowEnabled()) { + if (FastHasAttribute(html_names::kOpenAttr)) { + if (!is_modal_) { + exception_state.ThrowDOMException( + DOMExceptionCode::kInvalidStateError, + "The dialog is already open as a non-modal dialog, and therefore " + "cannot be opened as a modal dialog."); + } + return; + } + } else { + if (FastHasAttribute(html_names::kOpenAttr)) { + return exception_state.ThrowDOMException( + DOMExceptionCode::kInvalidStateError, + "The element already has an 'open' " + "attribute, and therefore cannot be " + "opened modally."); + } } if (!isConnected()) { return exception_state.ThrowDOMException( diff --git a/third_party/blink/renderer/core/html/html_element.cc b/third_party/blink/renderer/core/html/html_element.cc index 7b82aadf99b08e..2f1e0de035d368 100644 --- a/third_party/blink/renderer/core/html/html_element.cc +++ b/third_party/blink/renderer/core/html/html_element.cc @@ -1330,21 +1330,12 @@ bool HTMLElement::IsPopoverReady(PopoverTriggerAction action, "value for the 'popover' attribute."); return false; } - if (!isConnected()) { - maybe_throw_exception(DOMExceptionCode::kInvalidStateError, - "Invalid on disconnected popover elements."); - return false; - } - if (expected_document && &GetDocument() != expected_document) { - maybe_throw_exception(DOMExceptionCode::kInvalidStateError, - "Invalid when the document changes while showing or " - "hiding a popover element."); - return false; - } if (action == PopoverTriggerAction::kShow && GetPopoverData()->visibilityState() != PopoverVisibilityState::kHidden) { - maybe_throw_exception(DOMExceptionCode::kInvalidStateError, - "Invalid on popover elements which aren't hidden."); + if (!RuntimeEnabledFeatures::PopoverDialogDontThrowEnabled()) { + maybe_throw_exception(DOMExceptionCode::kInvalidStateError, + "Invalid on popover elements which aren't hidden."); + } return false; } if (action == PopoverTriggerAction::kHide && @@ -1352,9 +1343,22 @@ bool HTMLElement::IsPopoverReady(PopoverTriggerAction action, // Important to check that visibility is not kShowing (rather than // popoverOpen()), because a hide transition might have been started on this // popover already, and we don't want to allow a double-hide. - maybe_throw_exception( - DOMExceptionCode::kInvalidStateError, - "Invalid on popover elements that aren't already showing."); + if (!RuntimeEnabledFeatures::PopoverDialogDontThrowEnabled()) { + maybe_throw_exception( + DOMExceptionCode::kInvalidStateError, + "Invalid on popover elements that aren't already showing."); + } + return false; + } + if (!isConnected()) { + maybe_throw_exception(DOMExceptionCode::kInvalidStateError, + "Invalid on disconnected popover elements."); + return false; + } + if (expected_document && &GetDocument() != expected_document) { + maybe_throw_exception(DOMExceptionCode::kInvalidStateError, + "Invalid when the document changes while showing or " + "hiding a popover element."); return false; } if (action == PopoverTriggerAction::kShow && IsA(this) && diff --git a/third_party/blink/renderer/platform/runtime_enabled_features.json5 b/third_party/blink/renderer/platform/runtime_enabled_features.json5 index 758ca4d384e1cb..98d16de0ad9862 100644 --- a/third_party/blink/renderer/platform/runtime_enabled_features.json5 +++ b/third_party/blink/renderer/platform/runtime_enabled_features.json5 @@ -2697,6 +2697,13 @@ name: "PointerEventDeviceId", status: "test", }, + // PopoverDialogDontThrow makes popover and dialog elements stop throwing + // exceptions when calling their show and hide methods when they are + // already in their requested state. See https://github.com/whatwg/html/pull/9142 + { + name: "PopoverDialogDontThrow", + status: "stable", + }, { name: "Portals", // Portals must be enabled by blink::features::kPortals as we require the diff --git a/third_party/blink/web_tests/external/wpt/html/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state.html b/third_party/blink/web_tests/external/wpt/html/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state.html new file mode 100644 index 00000000000000..c86cbe84a62294 --- /dev/null +++ b/third_party/blink/web_tests/external/wpt/html/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state.html @@ -0,0 +1,29 @@ + + + + + + +hello + + diff --git a/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-attribute-basic.html b/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-attribute-basic.html index eab61407c8b387..32d3deb3848c22 100644 --- a/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-attribute-basic.html +++ b/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-attribute-basic.html @@ -254,11 +254,21 @@ },{once: true}); assert_true(popover.matches(':popover-open')); assert_true(other_popover.matches(':popover-open')); - assert_throws_dom('InvalidStateError', () => popover.hidePopover()); + popover.hidePopover(); // Calling hidePopover on a hidden popover should not throw. assert_false(other_popover.matches(':popover-open'),'unrelated popover is hidden'); assert_false(popover.matches(':popover-open'),'popover is still hidden if its type changed during hide event'); - assert_throws_dom("InvalidStateError",() => other_popover.hidePopover(),'Nested popover should already be hidden'); - },`Changing the popover type in a "beforetoggle" event handler should throw an exception (during hidePopover())`); + other_popover.hidePopover(); // Calling hidePopover on a hidden popover should not throw. + },`Changing the popover type in a "beforetoggle" event handler during hidePopover() should not throw an exception`); + + test(t => { + const popover = document.createElement('div'); + assert_throws_dom('NotSupportedError', () => popover.hidePopover(), + 'Calling hidePopover on an element without a popover attribute should throw.'); + popover.setAttribute('popover', 'auto'); + popover.hidePopover(); // Calling hidePopover on a disconnected popover should not throw. + assert_throws_dom('InvalidStateError', () => popover.showPopover(), + 'Calling showPopover on a disconnected popover should throw.'); + },'Calling hidePopover on a disconnected popover should not throw.'); function interpretedType(typeString,method) { if (validTypes.includes(typeString)) diff --git a/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-light-dismiss.html b/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-light-dismiss.html index d7d1edd3a4b1fa..4b888169e1becc 100644 --- a/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-light-dismiss.html +++ b/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-light-dismiss.html @@ -532,7 +532,7 @@ p14.hidePopover(); },{once:true}); assert_true(p13.matches(':popover-open') && p14.matches(':popover-open') && p15.matches(':popover-open'),'all three should be open'); - assert_throws_dom('InvalidStateError',() => p14.hidePopover(),'should throw because the event listener has already hidden the popover'); + p14.hidePopover(); assert_true(p13.matches(':popover-open'),'p13 should still be open'); assert_false(p14.matches(':popover-open')); assert_false(p15.matches(':popover-open')); @@ -579,10 +579,7 @@ p20.showPopover(); }); p20.addEventListener('beforetoggle', logEvents); - // Because the `beforetoggle` handler shows a different popover, - // and that action closes the p19 popover, the call to hidePopover() - // will result in an exception. - assert_throws_dom('InvalidStateError',() => p19.hidePopover()); + p19.hidePopover(); assert_array_equals(events,['hide p19','show p20'],'There should not be a second hide event for 19'); assert_false(p19.matches(':popover-open')); assert_true(p20.matches(':popover-open')); diff --git a/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-move-documents.html b/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-move-documents.html index 9feaa4b2bf8756..11f52c2f2f0d14 100644 --- a/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-move-documents.html +++ b/third_party/blink/web_tests/external/wpt/html/semantics/popovers/popover-move-documents.html @@ -4,10 +4,30 @@ + +
p1