Check dialog is modal flag instead of open attribute in 'check popover validity' algorithm #9344

nt1m · 2023-05-27T08:09:19Z

The following case currently does not fail:

dialog.showModal();
dialog.open = false;
dialog.showPopover();

even though it is a top layer conflict:

showModal pushes to the top layer
.open = false then hides the dialog, while keeping it in top layer
showPopover pushes to the top layer again

The check for the open attribute is supposed to prevent top layer conflicts, since those conflicts can leave the element in a broken state, notably after hiding. For instance, the previous sequence of actions followed by dialog.hidePopover() will leave the is modal flag despite the dialog being completely removed from the top layer.

Also only remove from top layer in HTMLDialogElement.prototype.close() when the is modal flag is
true to prevent that method from removing popovers from the top layer.

Fixes #9335

At least two implementers are interested (and none opposed):
- WebKit
- Chromium ("check popover algorithm" check for open dialogs is weak #9335 (comment))
Tests are written and can be reviewed and commented upon at:
- [popover] Check for modal flag instead of open attribute WebKit/WebKit#14454 (@rwlbuis, can you please make a WPT export?)
Implementation bugs are filed:
- Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1449442
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1835464
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=257412
- Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
- Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …
MDN issue is filed: …

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

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

nt1m · 2023-05-29T22:56:19Z

@josepharhar @mfreed7 One side effect of this PR is that you can now run showPopover() on dialogs that have the open attribute, but aren't modal. This found a small bug in the HTMLDialogElement.prototype.close() algorithm, which was removing from the top layer even when the thing that pushed it into the top layer was showPopover() as opposed to showModal().

The alternative fix would be throwing an exception if you're trying to close a <dialog popover> with HTMLDialogElement.prototype.close(), which I'm also fine doing.

Any opinions?

annevk · 2023-05-30T11:26:12Z

source

-
-   <li><p>If <var>subject</var> is in its <code>Document</code>'s <span>top layer</span>, then <span
-   data-x="list remove">remove</span> it.</p></li>
+    data-x="dom-dialog-returnValue">returnValue</code> attribute to <var>result</var>.</p></li>


Nit: no additional indentation warranted here.

Suggested change

data-x="dom-dialog-returnValue">returnValue</code> attribute to <var>result</var>.</p></li>

data-x="dom-dialog-returnValue">returnValue</code> attribute to <var>result</var>.</p></li>

@annevk Fixed!

annevk · 2023-05-30T11:32:24Z

source

-     <li><p><var>element</var> is a <code>dialog</code> element and has an <code
-     data-x="attr-details-open">open</code> attribute</p></li>
+     <li><p><var>element</var> is a <code>dialog</code> element and its <span>is modal</span> flag
+     is set to true.</li>


This seems reasonable to me, but why wouldn't we check for either the open attribute or the is modal flag here?

We discussed this a bit more and it makes sense to me that we just care about the top layer conflicts here and not care about potential semantic mismatches.

Hmm, can you say more? I was leaning toward your original suggestion, check for either open or modal. It feels weird to "direct upgrade" a non-modal dialog to a popover, but maybe it's not so weird.

It doesn't violate an invariant we're trying to uphold. It's weird, but not really weirder than any of the other things that's possible and doesn't break anything. Whereas top layer conflicts do break things.

If we wanted to make the open attribute more meaningful we'd probably have to do things when folks remove it and such, but it seems a bit too late to start with that now.

mfreed7 · 2023-05-30T17:03:29Z

The alternative fix would be throwing an exception if you're trying to close a <dialog popover> with HTMLDialogElement.prototype.close(), which I'm also fine doing.

I like this approach also. I think generally we should throw for this type of "mixing and matching" where it doesn't seem to make sense.

annevk · 2023-05-30T17:48:24Z

FWIW, I think the currently proposed approach is nicer as it doesn't bake more of popover into the dialog element.

nt1m · 2023-05-30T19:31:14Z

The alternative fix would be throwing an exception if you're trying to close a <dialog popover> with HTMLDialogElement.prototype.close(), which I'm also fine doing.

I like this approach also. I think generally we should throw for this type of "mixing and matching" where it doesn't seem to make sense.

Thinking more, this is tricky, because if you have the sequence:

dialog.show();
dialog.showPopover();
dialog.close();

It wouldn't be unreasonable for dialog.close() to remove the open attribute but keep the dialog in the top layer.

mfreed7 · 2023-05-30T21:29:04Z

Thinking more, this is tricky, because if you have the sequence:
dialog.show();
dialog.showPopover();
dialog.close();
It wouldn't be unreasonable for dialog.close() to remove the open attribute but keep the dialog in the top layer.

I guess I don't have strong opinions, and I think that's because I think we should eventually deprecate non-modal <dialog> which will clean a lot of this up. But the example above feels quite confusing for developers. I can see this ending up as an "HTML quiz" question: "what state is the dialog in at the end of this code?".

Part 2 is:

dialog.show();
dialog.showPopover();
dialog.close();
dialog.show(); // What happens here?

I think the answer to part 2 is that show() throws, right?

nt1m · 2023-05-30T21:35:14Z

Thinking more, this is tricky, because if you have the sequence:
dialog.show();
dialog.showPopover();
dialog.close();
It wouldn't be unreasonable for dialog.close() to remove the open attribute but keep the dialog in the top layer.
I guess I don't have strong opinions, and I think that's because I think we should eventually deprecate non-modal <dialog> which will clean a lot of this up. But the example above feels quite confusing for developers. I can see this ending up on one of the many "HTML quiz" questions: "what state is the dialog in at the end of this code?". Part 2 is:
dialog.show();
dialog.showPopover();
dialog.close();
dialog.show(); // What happens here?
I think the answer to part 2 is that show() throws, right?

Hmm, yeah, it does throw in that situation. I guess symmetry with show() does make sense. The other part that makes this tricky is that dialogs can be closed via non-JS (via <form method=dialog>), so not sure we can throw an exception when that codepath is invoked.

mfreed7 · 2023-05-30T21:40:03Z

Hmm, yeah, it does throw in that situation. I guess symmetry with show() does make sense. The other part that makes this tricky is that dialogs can be closed via non-JS (via <form method=dialog>), so not sure we can throw an exception when that codepath is invoked.

Right - I was wondering about <form method=dialog> also. We can't throw on the closing operation, but we can prevent that situation by throwing at showPopover(), right?

nt1m · 2023-05-30T23:59:18Z

@annevk @rwlbuis Do you have a preference here?

annevk · 2023-06-01T12:42:26Z

I think the current PR still makes sense.

In addition I would suggest that we remove

If this is in the popover showing state, then throw an "InvalidStateError" DOMException.

from dialog's show() as it's not fundamentally incompatible with also being a popover. As we demonstrate in this PR the salient piece of information is really top layer participation and show() doesn't do that so it should not introduce unnecessary coupling.

mfreed7 · 2023-06-01T18:37:01Z

I think the current PR still makes sense.

In addition I would suggest that we remove

If this is in the popover showing state, then throw an "InvalidStateError" DOMException.

from dialog's show() as it's not fundamentally incompatible with also being a popover. As we demonstrate in this PR the salient piece of information is really top layer participation and show() doesn't do that so it should not introduce unnecessary coupling.

What about these questions? #9344 (comment)

annevk · 2023-06-02T12:43:57Z

@mfreed7 there's no question there? If you mean what would happen with the last show() call in the quoted text: if we adopt my suggestion of it not caring about popover that would no longer throw. Which seems good.

mfreed7 · 2023-06-02T17:49:49Z

@mfreed7 there's no question there? If you mean what would happen with the last show() call in the quoted text: if we adopt my suggestion of it not caring about popover that would no longer throw. Which seems good.

Right - that question. Ok, so the proposal is:

dialog.show() just adds the open attribute, which makes it non-display:none, which is a no-op if the dialog is already a showing popover.
dialog.close(), if dialog is non-modal, just removes the open attribute, and re-applies display:none only if the dialog isn't a showing popover.
dialog.showPopover() and dialog.hidePopover() basically ignore the open attribute, and just check whether dialog "is modal". If so, they throw. If not, they do the normal thing, ignoring that the dialog was already showing.

I think that also sounds reasonable. There might be weird cases when trying to use CSS transitions during popover show/hide, but if you're mixing/matching showPopover() and show() like this, you're likely doing something wrong anyway.

So ok by me, I think.

…r validity' algorithm The following case currently does not fail: ``` dialog.showModal(); dialog.open = false; dialog.showPopover(); ``` even though it is a top layer conflict: 1. `showModal` pushes to the top layer 2. `.open = false` then hides the dialog, while keeping it in top layer 3. `showPopover` pushes to the top layer again The check for the open attribute is supposed to prevent top layer conflicts, since those conflicts can leave the element in a broken state, notably after hiding. For instance, the previous sequence of actions followed by `dialog.hidePopover()` will leave the `is modal` flag despite the dialog being completely removed from the top layer. Also only remove from top layer in `HTMLDialogElement.prototype.close()` when the is modal flag is true to prevent that method from removing popovers from the top layer. For `dialog.close()` to be symmetric & consistent with `dialog.show()`, we also stop throwing when `show()` is called on a dialog in the popover showing state. Fixes whatwg#9335

nt1m · 2023-06-05T03:46:15Z

I updated the PR with @annevk's suggestion applied.

annevk

Do we have tests for this change? If OP could point to them that'd be great.

nt1m · 2023-06-05T15:44:09Z

@annevk The WebKit export for WebKit/WebKit#14454 should take care of it.

mfreed7

LGTM, thanks!

https://bugs.webkit.org/show_bug.cgi?id=257412 Reviewed by Tim Nguyen. Check for modal flag instead of open attribute in checkPopoverValidity check: whatwg/html#9344 * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-top-layer-combinations.html: * Source/WebCore/html/HTMLDialogElement.cpp: (WebCore::HTMLDialogElement::show): (WebCore::HTMLDialogElement::close): * Source/WebCore/html/HTMLElement.cpp: (WebCore::checkPopoverValidity): Canonical link: https://commits.webkit.org/264876@main

Adjust test after issue 9344 is merged: whatwg/html#9344

Adjust test after dialog check changed in 'check popover validity': whatwg/html#9344

…40394) Adjust test after dialog check changed in 'check popover validity': whatwg/html#9344

…a=testonly Automatic update from web-platform-tests Adjust test after dialog check changed in 'check popover validity' (#40394) Adjust test after dialog check changed in 'check popover validity': whatwg/html#9344 -- wpt-commits: 37d0fc507b5bd89960107d13500ea2e167f1d3e8 wpt-pr: 40394

…a=testonly Automatic update from web-platform-tests Adjust test after dialog check changed in 'check popover validity' (#40394) Adjust test after dialog check changed in 'check popover validity': whatwg/html#9344 -- wpt-commits: 37d0fc507b5bd89960107d13500ea2e167f1d3e8 wpt-pr: 40394 UltraBlame original commit: 4f4e5646bb7993ea1dbf9008632c73392dec0461

https://bugs.webkit.org/show_bug.cgi?id=257412 Reviewed by Tim Nguyen. Check for modal flag instead of open attribute in checkPopoverValidity check: whatwg/html#9344 * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-top-layer-combinations.html: * Source/WebCore/html/HTMLDialogElement.cpp: (WebCore::HTMLDialogElement::show): (WebCore::HTMLDialogElement::close): * Source/WebCore/html/HTMLElement.cpp: (WebCore::checkPopoverValidity): Canonical link: https://commits.webkit.org/264876@main

- Replaces the dialog open attribute check in "check popover validity" with an open attribute + isModal check - Removes the exception in dialog.show() which looks at the popover state - Fixes a bug this uncovered in ScheduleForTopLayerRemoval which allowed on element to be listed in the removals multiple times HTML spec PR: whatwg/html#9344 Fixed: 1449442 Change-Id: I74e6189070a1a416d486cbc607f92f964a106d66 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4616893 Commit-Queue: Joey Arhar <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1183314}

nt1m requested review from annevk, josepharhar and mfreed7 May 27, 2023 08:11

nt1m force-pushed the is-modal-check-popover-validity branch from 24e669f to 35c8661 Compare May 27, 2023 08:12

nt1m mentioned this pull request May 27, 2023

"check popover algorithm" check for open dialogs is weak #9335

Closed

rwlbuis mentioned this pull request May 29, 2023

[popover] Check for modal flag instead of open attribute WebKit/WebKit#14454

Merged

nt1m force-pushed the is-modal-check-popover-validity branch from 35c8661 to aaf0fa6 Compare May 29, 2023 22:51

annevk reviewed May 30, 2023

View reviewed changes

annevk added normative change topic: popover The popover attribute and friends labels May 30, 2023

nt1m force-pushed the is-modal-check-popover-validity branch from aaf0fa6 to 1d7a1a1 Compare May 30, 2023 16:31

nt1m force-pushed the is-modal-check-popover-validity branch from 1d7a1a1 to b600242 Compare June 5, 2023 03:45

annevk approved these changes Jun 5, 2023

View reviewed changes

mfreed7 approved these changes Jun 5, 2023

View reviewed changes

annevk merged commit 278116c into whatwg:main Jun 6, 2023

rwlbuis added a commit to web-platform-tests/wpt that referenced this pull request Jun 6, 2023

Adjust test after issue 9344 is merged

f3c96dc

Adjust test after issue 9344 is merged: whatwg/html#9344

rwlbuis mentioned this pull request Jun 6, 2023

Adjust test after issue 9344 is merged web-platform-tests/wpt#40394

Merged

rwlbuis added a commit to web-platform-tests/wpt that referenced this pull request Jun 6, 2023

Adjust test after dialog check changed in 'check popover validity'

91dc843

Adjust test after dialog check changed in 'check popover validity': whatwg/html#9344

rwlbuis added a commit to web-platform-tests/wpt that referenced this pull request Jun 6, 2023

Adjust test after dialog check changed in 'check popover validity' (#…

37d0fc5

…40394) Adjust test after dialog check changed in 'check popover validity': whatwg/html#9344

nt1m deleted the is-modal-check-popover-validity branch June 12, 2023 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check dialog is modal flag instead of open attribute in 'check popover validity' algorithm #9344

Check dialog is modal flag instead of open attribute in 'check popover validity' algorithm #9344

nt1m commented May 27, 2023 •

edited by pr-preview bot

Loading

nt1m commented May 29, 2023

annevk May 30, 2023

nt1m May 30, 2023

nt1m May 30, 2023

annevk May 30, 2023

annevk May 30, 2023

mfreed7 May 30, 2023

annevk May 30, 2023

mfreed7 commented May 30, 2023

annevk commented May 30, 2023

nt1m commented May 30, 2023

mfreed7 commented May 30, 2023 •

edited

Loading

nt1m commented May 30, 2023

mfreed7 commented May 30, 2023

nt1m commented May 30, 2023

annevk commented Jun 1, 2023

mfreed7 commented Jun 1, 2023

annevk commented Jun 2, 2023

mfreed7 commented Jun 2, 2023

nt1m commented Jun 5, 2023

annevk left a comment

nt1m commented Jun 5, 2023

mfreed7 left a comment

	data-x="dom-dialog-returnValue">returnValue</code> attribute to <var>result</var>.</p></li>
	data-x="dom-dialog-returnValue">returnValue</code> attribute to <var>result</var>.</p></li>

Check dialog is modal flag instead of open attribute in 'check popover validity' algorithm #9344

Check dialog is modal flag instead of open attribute in 'check popover validity' algorithm #9344

Conversation

nt1m commented May 27, 2023 • edited by pr-preview bot Loading

nt1m commented May 29, 2023

annevk May 30, 2023

Choose a reason for hiding this comment

nt1m May 30, 2023

Choose a reason for hiding this comment

nt1m May 30, 2023

Choose a reason for hiding this comment

annevk May 30, 2023

Choose a reason for hiding this comment

annevk May 30, 2023

Choose a reason for hiding this comment

mfreed7 May 30, 2023

Choose a reason for hiding this comment

annevk May 30, 2023

Choose a reason for hiding this comment

mfreed7 commented May 30, 2023

annevk commented May 30, 2023

nt1m commented May 30, 2023

mfreed7 commented May 30, 2023 • edited Loading

nt1m commented May 30, 2023

mfreed7 commented May 30, 2023

nt1m commented May 30, 2023

annevk commented Jun 1, 2023

mfreed7 commented Jun 1, 2023

annevk commented Jun 2, 2023

mfreed7 commented Jun 2, 2023

nt1m commented Jun 5, 2023

annevk left a comment

Choose a reason for hiding this comment

nt1m commented Jun 5, 2023

mfreed7 left a comment

Choose a reason for hiding this comment

nt1m commented May 27, 2023 •

edited by pr-preview bot

Loading

mfreed7 commented May 30, 2023 •

edited

Loading