Allow top layer elements to be nested within popovers #10116

josepharhar · 2024-01-31T16:04:56Z

This patch makes top layer elements, including the dialog element, to be nested inside of an open popover by not closing the popover when the new top layer element is opened. Without this patch, opening a modal dialog inside of a popover will make the page inert and make the dialog invisible. A separate PR for fullscreen will be made: whatwg/fullscreen#237

Fixes #9998

Implemented in chromium here:
https://chromium-review.googlesource.com/c/chromium/src/+/5229300

At least two implementers are interested (and none opposed):
- Chrome
- Firefox
Tests are written and can be reviewed and commented upon at:
- Allow top layer elements to be nested within popovers web-platform-tests/wpt#44146
Implementation bugs are filed:
- Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1520938
- Gecko: https://phabricator.services.mozilla.com/D200686
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=269928
MDN issue is filed: I'm not sure this edge case warrants any text in MDN
The top of this comment includes a clear commit message to use.

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

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

Fixes whatwg#9998 Implemented in chromium here: https://chromium-review.googlesource.com/c/chromium/src/+/5229300

Without this patch, the fullscreening an element inside an open popover will make the fullscreen element display:none. Issue: whatwg/html#9998 Corresponding HTML PR: whatwg/html#10116

mfreed7

LGTM - this seems to match the Chromium impl, which seems to work correctly.

source

keithamus · 2024-02-02T17:00:27Z

Looks like showPopovers() invocation of topmost popover ancestor needs to be given true.

source

…ent. r=smaug Given some markup like: ```html <div id=popover popover> <button id="openDialog">Open Dialog</button> <dialog id=dialog> I'm a dialog! </dialog> </div> <button id="openPopover">Open Popover</button> <button>I do nothing!</button> ``` With some JS like ```js openDialog.onclick = () => { dialog.showModal(); } openPopover.onclick = () => { popover.showPopover(); } ``` Clicking the "Open Popover" button followed by the "Open Dialog" button results in both the Dialog and Popover being hidden, however the dialog will still be open as modal, rendering the whole page inert, nothing is clickable and there seems to be no way to undo this (unless you use a CloseWatcher gesture such as the Esc key - if you have one available on your device). It is expected that the popover should not close the dialog, as it causes the invisible Modal Dialog to make the whole page inert, and it is very difficult for users (and developers) to discover how to undo this action (pressing escape). This was reported in whatwg/html#9998, and WhatWG resolved to fix this, which in whatwg/html#10116. Differential Revision: https://phabricator.services.mozilla.com/D200686

…ent. r=smaug Given some markup like: ```html <div id=popover popover> <button id="openDialog">Open Dialog</button> <dialog id=dialog> I'm a dialog! </dialog> </div> <button id="openPopover">Open Popover</button> <button>I do nothing!</button> ``` With some JS like ```js openDialog.onclick = () => { dialog.showModal(); } openPopover.onclick = () => { popover.showPopover(); } ``` Clicking the "Open Popover" button followed by the "Open Dialog" button results in both the Dialog and Popover being hidden, however the dialog will still be open as modal, rendering the whole page inert, nothing is clickable and there seems to be no way to undo this (unless you use a CloseWatcher gesture such as the Esc key - if you have one available on your device). It is expected that the popover should not close the dialog, as it causes the invisible Modal Dialog to make the whole page inert, and it is very difficult for users (and developers) to discover how to undo this action (pressing escape). This was reported in whatwg/html#9998, and WhatWG resolved to fix this, which in whatwg/html#10116. Differential Revision: https://phabricator.services.mozilla.com/D200686 UltraBlame original commit: 52284e30cea2c52ab073310e4010161702dc92e4

keithamus · 2024-02-08T21:19:30Z

FWIW this is now merged in Gecko: https://phabricator.services.mozilla.com/D200686.

josepharhar · 2024-02-09T18:00:10Z

Looks like showPopovers() invocation of topmost popover ancestor needs to be given true.

done

Chromestatus: https://chromestatus.com/feature/5149353645965312 Spec PRs: whatwg/html#10116 whatwg/fullscreen#237 I2S: {waiting for spec to land} Fixed: 1520938 Change-Id: I04e5f217155658907988ed0a0557944dbffbf6f2

mfreed7

I guess I forgot to hit the "Submit" button on this comment.

source

mfreed7 · 2024-03-04T19:39:11Z

Is anything missing from this PR? I'd like to ship this in Chrome 124, but that branches in a few weeks. I need the spec to land first.

keithamus · 2024-03-04T20:41:13Z

Webkit bug: https://bugs.webkit.org/show_bug.cgi?id=269928

domenic

Changes LGTM, now that we've resolved the thread. I can merge once all the checkboxes in the OP are filled out.

mfreed7 · 2024-03-07T16:57:27Z

Changes LGTM, now that we've resolved the thread. I can merge once all the checkboxes in the OP are filled out.

Nice, thanks! @josepharhar, mind updating the OP checkboxes?

josepharhar · 2024-03-20T14:50:02Z

@domenic I updated it, we can merge now

Chromestatus: https://chromestatus.com/feature/5149353645965312 Spec PRs: whatwg/html#10116 whatwg/fullscreen#237 I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/3vIL56MTgvw/m/W4hcWMblBAAJ Fixed: 1520938 Change-Id: I04e5f217155658907988ed0a0557944dbffbf6f2

Chromestatus: https://chromestatus.com/feature/5149353645965312 Spec PRs: whatwg/html#10116 whatwg/fullscreen#237 I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/3vIL56MTgvw/m/W4hcWMblBAAJ Fixed: 1520938 Change-Id: I04e5f217155658907988ed0a0557944dbffbf6f2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5255819 Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1279321}

Chromestatus: https://chromestatus.com/feature/5149353645965312 Spec PRs: whatwg/html#10116 whatwg/fullscreen#237 I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/3vIL56MTgvw/m/W4hcWMblBAAJ Fixed: 1520938 Change-Id: I04e5f217155658907988ed0a0557944dbffbf6f2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5255819 Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1279321}

Automatic update from web-platform-tests Ship NestedTopLayerSupport Chromestatus: https://chromestatus.com/feature/5149353645965312 Spec PRs: whatwg/html#10116 whatwg/fullscreen#237 I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/3vIL56MTgvw/m/W4hcWMblBAAJ Fixed: 1520938 Change-Id: I04e5f217155658907988ed0a0557944dbffbf6f2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5255819 Commit-Queue: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1279321} -- wpt-commits: 5317488d13e2990de35a96e8fc3bf74fa7a2987b wpt-pr: 44339

Without this change, fullscreening an element inside an open popover will make the fullscreen element display:none. Issue: whatwg/html#9998 Corresponding HTML PR: whatwg/html#10116

https://bugs.webkit.org/show_bug.cgi?id=269928 Reviewed by Tim Nguyen. This implements the changes to the spec as defined by whatwg/html#10116 * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-top-layer-nesting-anchor.tentative-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-top-layer-nesting-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-top-layer-nesting-hints.tentative-expected.txt: * Source/WebCore/dom/Element.cpp: (WebCore::Element::topmostPopoverAncestor): * Source/WebCore/dom/Element.h: * Source/WebCore/dom/FullscreenManager.cpp: (WebCore::FullscreenManager::willEnterFullscreen): * Source/WebCore/html/HTMLDialogElement.cpp: (WebCore::HTMLDialogElement::show): (WebCore::HTMLDialogElement::showModal): * Source/WebCore/html/HTMLElement.cpp: (WebCore::HTMLElement::showPopover): (WebCore::topmostPopoverAncestor): Deleted. Canonical link: https://commits.webkit.org/279874@main

Allow top layer elements to be nested within popovers

573442f

Fixes whatwg#9998 Implemented in chromium here: https://chromium-review.googlesource.com/c/chromium/src/+/5229300

josepharhar mentioned this pull request Jan 31, 2024

Opening a dialog as modal, nested inside a popover, causes the whole page to go inert with limited ways for the user to address it. #9998

Closed

josepharhar mentioned this pull request Jan 31, 2024

Don't close ancestor popovers when fullscreening whatwg/fullscreen#237

Merged

5 tasks

mfreed7 approved these changes Feb 1, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

domenic added normative change topic: popover The popover attribute and friends labels Feb 6, 2024

domenic requested changes Feb 6, 2024

View reviewed changes

domenic self-assigned this Feb 6, 2024

josepharhar added 3 commits February 6, 2024 11:56

fix null hideUntil

726f187

update note for ancestor popover

fefa64e

capitalization

001b4bc

keithamus requested a review from domenic February 7, 2024 09:44

josepharhar added 2 commits February 9, 2024 10:05

remove hide all popovers

86e134a

Merge branch 'main' into toplayernested

d13eb54

chromium-wpt-export-bot mentioned this pull request Feb 10, 2024

Ship NestedTopLayerSupport web-platform-tests/wpt#44339

Merged

mfreed7 approved these changes Mar 4, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

domenic approved these changes Mar 5, 2024

View reviewed changes

domenic added the topic: dialog The <dialog> element label Mar 5, 2024

Merge branch 'main' into toplayernested

cf68482

domenic merged commit ae5a7ec into whatwg:main Mar 26, 2024
2 checks passed

foolip mentioned this pull request Apr 5, 2024

Add commandfor & command attributes to HTMLButtonElement #9841

Open

5 tasks

Ahmad-S792 mentioned this pull request Jun 3, 2024

Allow top layer elements to be nested within popovers WebKit/WebKit#24958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow top layer elements to be nested within popovers #10116

Allow top layer elements to be nested within popovers #10116

josepharhar commented Jan 31, 2024 •

edited by pr-preview bot

Loading

mfreed7 left a comment

keithamus commented Feb 2, 2024

keithamus commented Feb 8, 2024

josepharhar commented Feb 9, 2024

mfreed7 left a comment

mfreed7 commented Mar 4, 2024

keithamus commented Mar 4, 2024

domenic left a comment

mfreed7 commented Mar 7, 2024

josepharhar commented Mar 20, 2024

Allow top layer elements to be nested within popovers #10116

Allow top layer elements to be nested within popovers #10116

Conversation

josepharhar commented Jan 31, 2024 • edited by pr-preview bot Loading

mfreed7 left a comment

Choose a reason for hiding this comment

keithamus commented Feb 2, 2024

keithamus commented Feb 8, 2024

josepharhar commented Feb 9, 2024

mfreed7 left a comment

Choose a reason for hiding this comment

mfreed7 commented Mar 4, 2024

keithamus commented Mar 4, 2024

domenic left a comment

Choose a reason for hiding this comment

mfreed7 commented Mar 7, 2024

josepharhar commented Mar 20, 2024

josepharhar commented Jan 31, 2024 •

edited by pr-preview bot

Loading