Don't throw when popover/dialog is in requested state #9142

josepharhar · 2023-04-10T15:32:00Z

This patch makes the following changes:

Don't throw exceptions when showPopover or hidePopover is called and the popover is already in the requested state.
Don't throw exceptions when dialog.showModal, dialog.show, or dialog.close is called and the dialog is already in the requested state.
Throw exceptions when trying to switch between modal and non-modal dialog modes via dialog.showModal or dialog.show.

At least two implementers are interested (and none opposed):
- Chrome
- WebKit
Tests are written and can be reviewed and commented upon at:
- Don't throw when popovers and dialogs are in requested state web-platform-tests/wpt#39781
Implementation bugs are filed:
- Chromium: https://chromium-review.googlesource.com/c/chromium/src/+/4494823
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1830737
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=256167
MDN issue is filed: n/a

(See WHATWG Working Mode: Changes for more details.)

/interactive-elements.html ( diff )
/popover.html ( diff )

This patch makes the following changes: - Don't throw exceptions when showPopover or hidePopover is called and the popover is already in the requested state. - Don't throw exceptions when dialog.showModal, dialog.show, or dialog.close is called and the dialog is already in the requested state. - Throw exceptions when trying to switch between modal and non-modal dialog modes via dialog.showModal or dialog.show. Fixes whatwg#9045

source

annevk

Looks good editorially.

annevk · 2023-04-24T14:10:54Z

source

   <li><p>If <span>this</span> has an <code data-x="attr-dialog-open">open</code> attribute, then
-   return.</p></li>
+   throw an <span>"<code>InvalidStateError</code>"</span> <code>DOMException</code>.</p></li>


This is a new exception, how certain are we that this is web compatible?

Tim convinced me that it's unlikely this will break anything.

annevk · 2023-04-24T16:14:21Z

Once this has tests and impl bugs this should be good to go. Thanks @josepharhar!

This is being changed in the HTML spec here: whatwg/html#9142 Change-Id: Ib8aaaf314c2a1de5d082494e5172e029d531c8e8

josepharhar · 2023-05-01T16:56:29Z

Once this has tests and impl bugs this should be good to go.

Done

josepharhar · 2023-05-01T21:00:04Z

@mfreed7 suggested that we should throw when hidePopover is called on a disconnected popover. This PR currently makes disconnected popovers stop throwing when hidePopover is called on them.

Does anyone have thoughts on this? If not I'll make that case start throwing again.

mfreed7 · 2023-05-05T18:41:22Z

@mfreed7 suggested that we should throw when hidePopover is called on a disconnected popover. This PR currently makes disconnected popovers stop throwing when hidePopover is called on them.

Does anyone have thoughts on this? If not I'll make that case start throwing again.

...my rationale was simply that this is a different case than calling showPopover when it's already showing. Calling showPopover/hidePopover in "invalid" cases should still throw. These include elements not containing the popover attribute, disconnected elements, etc.

annevk · 2023-05-08T15:37:19Z

I don't care strongly, though I suspect it'll be easier for people to use when there are less cases that throw, including what one might think of as erroneous cases. I'm wondering what web developers would do when they encounter the exception. Add a conditional or solve their ordering problem? If they would usually do the former, they are better served by us not throwing.

Either way, this will need some rebasing.

josepharhar · 2023-05-08T16:24:11Z

I resolved the conflicts, which included the current document vs expected document check, which I included in the list of conditions after the no-exception early out.

mfreed7 · 2023-05-08T22:20:15Z

I don't care strongly, though I suspect it'll be easier for people to use when there are less cases that throw, including what one might think of as erroneous cases. I'm wondering what web developers would do when they encounter the exception. Add a conditional or solve their ordering problem? If they would usually do the former, they are better served by us not throwing.

One quick data point: nobody evaluating the popover API that I worked with encountered this issue, or at least never relayed it to me, so hopefully it's fairly corner case.

My intuition is that we'll most help them by throwing. If you call showPopover() while the element is disconnected, it won't do anything. So calling attention to this seems like the right thing to do, rather than silently succeeding and doing nothing. If the problem was an ordering problem, e.g. popover.showPopover(); document.body.appendChild(popover); then the only fix is to solve the ordering problem. If the problem was a state problem, e.g. function justTryShowing() {popover.showPopover} then the only fix is a conditional. But without a signal like an exception, it'll be very frustrating to try to figure out why the popover isn't showing. (I've encountered this frustration - it's real.)

https://bugs.webkit.org/show_bug.cgi?id=255879 Reviewed by Tim Nguyen. Implement the spec changes from: whatwg/html#9142 The dialog related changes are tested by dialog-no-throw-requested-state.html. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-no-throw-requested-state.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-attribute-basic.html: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss.html: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-move-documents.html: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/resources/popover-utils.js: (async sendTab): (async sendEscape): (async sendEnter): (assertPopoverVisibility): * LayoutTests/platform/ios/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt: * Source/WebCore/html/HTMLDialogElement.cpp: (WebCore::HTMLDialogElement::show): (WebCore::HTMLDialogElement::showModal): * Source/WebCore/html/HTMLElement.cpp: (WebCore::checkPopoverValidity): (WebCore::HTMLElement::showPopover): (WebCore::HTMLElement::hidePopoverInternal): Canonical link: https://commits.webkit.org/263957@main

This is being changed in the HTML spec here: whatwg/html#9142 Change-Id: Ib8aaaf314c2a1de5d082494e5172e029d531c8e8

This is being changed in the HTML spec here: whatwg/html#9142 Change-Id: Ib8aaaf314c2a1de5d082494e5172e029d531c8e8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4494823 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1144952}

…e in requested state, a=testonly Automatic update from web-platform-tests Don't throw when popovers and dialogs are in requested state This is being changed in the HTML spec here: whatwg/html#9142 Change-Id: Ib8aaaf314c2a1de5d082494e5172e029d531c8e8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4494823 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1144952} -- wpt-commits: 0297d56d41123bb302ed3fdc84ec4f0fa891c4bf wpt-pr: 39781

…e. r=emilio Update to be in line with spec discussions at whatwg/html#9142 Differential Revision: https://phabricator.services.mozilla.com/D178782

…e. r=emilio Update to be in line with spec discussions at whatwg/html#9142 Differential Revision: https://phabricator.services.mozilla.com/D178782 UltraBlame original commit: 39007d3d4f3eee8382115fb59e595de006ceca70

mbrodesser-Igalia · 2023-06-20T13:03:36Z

@mfreed7 suggested that we should throw when hidePopover is called on a disconnected popover. This PR currently makes disconnected popovers stop throwing when hidePopover is called on them.
Does anyone have thoughts on this? If not I'll make that case start throwing again.

Assuming this is an edge-case, with unclear developer benefit, consistency with showPopover() and togglePopover() seems preferable. An argument against the current inconsistency is, that it must propagate through the spec down to the calls of https://html.spec.whatwg.org/#check-popover-validity. Let's please make this consistent.

@emilio: what's your view on this?

CC @mfreed7

...my rationale was simply that this is a different case than calling showPopover when it's already showing. Calling showPopover/hidePopover in "invalid" cases should still throw. These include elements not containing the popover attribute, disconnected elements, etc.

mbrodesser-Igalia · 2023-06-27T13:36:44Z

CC @josepharhar

josepharhar · 2023-06-29T20:42:02Z

Assuming this is an edge-case, with unclear developer benefit

Let's please make this consistent.

What exactly are you proposing?

consistency with showPopover() and togglePopover() seems preferable

On a disconnected popover:

showPopover will throw
togglePopover will throw after Make togglePopover throw more exceptions #9451
hidePopover does not throw after this PR

mbrodesser-Igalia · 2023-07-03T12:47:52Z

Assuming this is an edge-case, with unclear developer benefit

Let's please make this consistent.

What exactly are you proposing?

To let hidePopover throw on a disconnected popover.

consistency with showPopover() and togglePopover() seems preferable

On a disconnected popover:
* showPopover will throw

* togglePopover will throw after [Make togglePopover throw more exceptions #9451](https://github.com/whatwg/html/pull/9451)

* hidePopover does not throw after this PR

Then those three methods all throw, hence, are consistent.

josepharhar · 2023-07-05T17:45:24Z

Ok, yeah I suppose that making hidePopover throw on a disconnected popover doesnt necessarily go against the original motivation of the issue that this PR fixed, so I'm ok with making it throw again

nt1m · 2023-07-05T19:20:00Z

The current setup is consistent with dialog fwiw, HTMLDialogElement::close() does not throw, but HTMLDialogElement::showModal() does.

annevk · 2023-07-05T20:28:13Z

Yeah, there's a difference between not being able to show and hiding no-opping. This was discussed in one of the many issues.

mbrodesser-Igalia · 2023-07-10T08:29:00Z

The current setup is consistent with dialog fwiw, HTMLDialogElement::close() does not throw, but HTMLDialogElement::showModal() does.

Thanks for pointing that out.

Be aware that hidePopover() called on an element without the popover attribute does throw. See https://html.spec.whatwg.org/#dom-hidepopover. That differs from https://html.spec.whatwg.org/#close-the-dialog.

Consistency within the popover API seems preferable, or?

nt1m · 2023-07-10T09:00:45Z

Be aware that hidePopover() called on an element without the popover attribute does throw. See https://html.spec.whatwg.org/#dom-hidepopover. That differs from https://html.spec.whatwg.org/#close-the-dialog.

You can't really change the tagname for <dialog>, so there isn't a parallel here I think.

mbrodesser-Igalia · 2023-07-10T09:34:06Z

You can't really change the tagname for <dialog>

True.

, so there isn't a parallel here I think.

mbrodesser-Igalia · 2023-07-10T09:37:47Z

Yeah, there's a difference between not being able to show and hiding no-opping. This was discussed in one of the many issues.

Presumably you mean #9142 (comment) and previous comments.

I lack the experience of what web-developers actually prefer. As there are arguments for both behaviors, throwing and not throwing, let's leave the API in the current state.

josepharhar mentioned this pull request Apr 10, 2023

Should popover/dialog show/hide when already shown/hidden throw? #9045

Closed

mfreed7 mentioned this pull request Apr 11, 2023

Use CSS spec for top layer #9093

Merged

4 tasks

nt1m reviewed Apr 12, 2023

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

nt1m added topic: popover The popover attribute and friends topic: dialog The <dialog> element labels Apr 12, 2023

josepharhar added 5 commits April 12, 2023 14:48

fix typo

bbdd1f3

Merge with some upstream commits

9f2de61

fix show() exception

23df735

fix disconnected exception

0800430

move connected check after requested state check

ecdbdfa

annevk approved these changes Apr 24, 2023

View reviewed changes

annevk added the needs tests Moving the issue forward requires someone to write tests label Apr 24, 2023

chromium-wpt-export-bot mentioned this pull request May 1, 2023

Don't throw when popovers and dialogs are in requested state web-platform-tests/wpt#39781

Merged

rwlbuis mentioned this pull request May 2, 2023

[popover] Don't throw when popover/dialog is in requested state WebKit/WebKit#13248

Merged

Merge with upstream commits

2177155

annevk approved these changes May 9, 2023

View reviewed changes

annevk merged commit ba956cb into whatwg:main May 9, 2023

mbrodesser-Igalia mentioned this pull request Jun 13, 2023

Add before removing steps whatwg/dom#1185

Open

4 tasks

YusukeHirao mentioned this pull request Sep 18, 2023

Update Popover API markuplint/markuplint#1152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't throw when popover/dialog is in requested state #9142

Don't throw when popover/dialog is in requested state #9142

josepharhar commented Apr 10, 2023 •

edited by annevk

Loading

annevk left a comment

annevk Apr 24, 2023

annevk Apr 24, 2023

annevk commented Apr 24, 2023

josepharhar commented May 1, 2023

josepharhar commented May 1, 2023

mfreed7 commented May 5, 2023

annevk commented May 8, 2023

josepharhar commented May 8, 2023

mfreed7 commented May 8, 2023

mbrodesser-Igalia commented Jun 20, 2023 •

edited

Loading

mbrodesser-Igalia commented Jun 27, 2023

josepharhar commented Jun 29, 2023

mbrodesser-Igalia commented Jul 3, 2023

josepharhar commented Jul 5, 2023

nt1m commented Jul 5, 2023

annevk commented Jul 5, 2023

mbrodesser-Igalia commented Jul 10, 2023

nt1m commented Jul 10, 2023

mbrodesser-Igalia commented Jul 10, 2023

mbrodesser-Igalia commented Jul 10, 2023

Don't throw when popover/dialog is in requested state #9142

Don't throw when popover/dialog is in requested state #9142

Conversation

josepharhar commented Apr 10, 2023 • edited by annevk Loading

annevk left a comment

Choose a reason for hiding this comment

annevk Apr 24, 2023

Choose a reason for hiding this comment

annevk Apr 24, 2023

Choose a reason for hiding this comment

annevk commented Apr 24, 2023

josepharhar commented May 1, 2023

josepharhar commented May 1, 2023

mfreed7 commented May 5, 2023

annevk commented May 8, 2023

josepharhar commented May 8, 2023

mfreed7 commented May 8, 2023

mbrodesser-Igalia commented Jun 20, 2023 • edited Loading

mbrodesser-Igalia commented Jun 27, 2023

josepharhar commented Jun 29, 2023

mbrodesser-Igalia commented Jul 3, 2023

josepharhar commented Jul 5, 2023

nt1m commented Jul 5, 2023

annevk commented Jul 5, 2023

mbrodesser-Igalia commented Jul 10, 2023

nt1m commented Jul 10, 2023

mbrodesser-Igalia commented Jul 10, 2023

mbrodesser-Igalia commented Jul 10, 2023

josepharhar commented Apr 10, 2023 •

edited by annevk

Loading

mbrodesser-Igalia commented Jun 20, 2023 •

edited

Loading