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 <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1144952}
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 ca1bcd9..1f1a0a0 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::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 @@
};
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 7b82aadf..2f1e0de 100644
--- a/third_party/blink/renderer/core/html/html_element.cc
+++ b/third_party/blink/renderer/core/html/html_element.cc
@@ -1330,6 +1330,26 @@
"value for the 'popover' attribute.");
return false;
}
+ if (action == PopoverTriggerAction::kShow &&
+ GetPopoverData()->visibilityState() != PopoverVisibilityState::kHidden) {
+ if (!RuntimeEnabledFeatures::PopoverDialogDontThrowEnabled()) {
+ maybe_throw_exception(DOMExceptionCode::kInvalidStateError,
+ "Invalid on popover elements which aren't hidden.");
+ }
+ return false;
+ }
+ if (action == PopoverTriggerAction::kHide &&
+ GetPopoverData()->visibilityState() != PopoverVisibilityState::kShowing) {
+ // 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.
+ 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.");
@@ -1341,22 +1361,6 @@
"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.");
- return false;
- }
- if (action == PopoverTriggerAction::kHide &&
- GetPopoverData()->visibilityState() != PopoverVisibilityState::kShowing) {
- // 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.");
- return false;
- }
if (action == PopoverTriggerAction::kShow && IsA<HTMLDialogElement>(this) &&
hasAttribute(html_names::kOpenAttr)) {
maybe_throw_exception(DOMExceptionCode::kInvalidStateError,
diff --git a/third_party/blink/renderer/platform/runtime_enabled_features.json5 b/third_party/blink/renderer/platform/runtime_enabled_features.json5
index 758ca4d..98d16de0 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 0000000..c86cbe84
--- /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 @@
+<!DOCTYPE html>
+<link rel=author href="mailto:jarhar@chromium.org">
+<link rel=help href="https://github.com/whatwg/html/pull/9142">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<dialog>hello</dialog>
+
+<script>
+test(() => {
+ const dialog = document.querySelector('dialog');
+
+ // calling close() on a dialog that is already closed should not throw.
+ dialog.close();
+
+ dialog.show();
+ // calling show() on a dialog that is already showing non-modal should not throw.
+ dialog.show();
+ assert_throws_dom('InvalidStateError', () => dialog.showModal(),
+ 'Calling showModal() on a dialog that is already showing non-modal should throw.');
+ dialog.close();
+
+ dialog.showModal();
+ assert_throws_dom('InvalidStateError', () => dialog.show(),
+ 'Calling show() on a dialog that is already showing modal should throw.');
+ // calling showModal() on a dialog that is already showing modal should not throw.
+ dialog.showModal();
+});
+</script>
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 eab61407c..32d3deb 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 d7d1edd3..4b888169 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 9feaa4b..11f52c2 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 @@
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
+<script>
+async function iframeLoaded(iframe) {
+ return new Promise(resolve => {
+ if (iframe.contentWindow.document.readyState == 'complete') {
+ resolve();
+ } else {
+ iframe.onload = resolve;
+ }
+ });
+}
+</script>
+
<iframe id=myframe srcdoc="<p>iframe</p>"></iframe>
<div id=p1 popover=auto>p1</div>
<script>
-test(() => {
+promise_test(async () => {
+ await iframeLoaded(myframe);
+ await new Promise(resolve => {
+ if (myframe.contentWindow.document.readyState == 'complete') {
+ resolve();
+ } else {
+
+ }
+ });
p1.addEventListener('beforetoggle', () => {
myframe.contentWindow.document.body.appendChild(p1);
});
@@ -18,7 +38,8 @@
<iframe id=myframe2 srcdoc="<p>iframe</p>"></iframe>
<div id=p2 popover=auto>p2</div>
<script>
-test(() => {
+promise_test(async () => {
+ await iframeLoaded(myframe2);
const p2 = document.getElementById('p2');
p2.showPopover();
p2.addEventListener('beforetoggle', () => {
@@ -27,10 +48,7 @@
assert_true(p2.matches(':popover-open'),
'The popover should be open after calling showPopover()');
- // Because the `beforetoggle` handler changes the document,
- // and that action closes the popover, the call to hidePopover()
- // will result in an exception.
- assert_throws_dom('InvalidStateError',() => p2.hidePopover());
+ p2.hidePopover();
assert_false(p2.matches(':popover-open'),
'The popover should be closed after moving it between documents.');
}, 'Moving popovers between documents while hiding should not throw an exception.');
@@ -43,7 +61,8 @@
<div id=p5 popover=auto>p5</div>
</div>
<script>
-test(() => {
+promise_test(async () => {
+ await iframeLoaded(myframe3);
p3.showPopover();
p4.showPopover();
p4.addEventListener('beforetoggle', event => {
diff --git a/third_party/blink/web_tests/external/wpt/html/semantics/popovers/resources/popover-utils.js b/third_party/blink/web_tests/external/wpt/html/semantics/popovers/resources/popover-utils.js
index aa69b7d4..5f0e634 100644
--- a/third_party/blink/web_tests/external/wpt/html/semantics/popovers/resources/popover-utils.js
+++ b/third_party/blink/web_tests/external/wpt/html/semantics/popovers/resources/popover-utils.js
@@ -153,10 +153,10 @@
assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/false, 'A popover should start out hidden');
popover.showPopover();
if (checkVisibility) assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/true, 'After showPopover(), a popover should be visible');
- assert_throws_dom("InvalidStateError",() => popover.showPopover(),'Calling showPopover on a showing popover should throw InvalidStateError');
+ popover.showPopover(); // Calling showPopover on a showing popover should not throw.
popover.hidePopover();
if (checkVisibility) assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/false, 'After hidePopover(), a popover should be hidden');
- assert_throws_dom("InvalidStateError",() => popover.hidePopover(),'Calling hidePopover on a hidden popover should throw InvalidStateError');
+ popover.hidePopover(); // Calling hidePopover on a hidden popover should not throw.
popover.togglePopover();
if (checkVisibility) assertPopoverVisibility(popover, /*isPopover*/true, /*expectedVisibility*/true, 'After togglePopover() on hidden popover, it should be visible');
popover.togglePopover();
@@ -172,7 +172,7 @@
const parent = popover.parentElement;
popover.remove();
assert_throws_dom("InvalidStateError",() => popover.showPopover(),'Calling showPopover on a disconnected popover should throw InvalidStateError');
- assert_throws_dom("InvalidStateError",() => popover.hidePopover(),'Calling hidePopover on a disconnected popover should throw InvalidStateError');
+ popover.hidePopover(); // Calling hidePopover on a disconnected popover should not throw.
assert_throws_dom("InvalidStateError",() => popover.togglePopover(),'Calling hidePopover on a disconnected popover should throw InvalidStateError');
parent.appendChild(popover);
}
