-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow top layer elements to be nested within popovers #10116
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - this seems to match the Chromium impl, which seems to work correctly.
Looks like |
…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
…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
…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
…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
FWIW this is now merged in Gecko: https://phabricator.services.mozilla.com/D200686. |
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I forgot to hit the "Submit" button on this comment.
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. |
Webkit bug: https://bugs.webkit.org/show_bug.cgi?id=269928 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
@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 <[email protected]> Reviewed-by: Joey Arhar <[email protected]> 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 <[email protected]> Reviewed-by: Joey Arhar <[email protected]> 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 <[email protected]> Reviewed-by: Joey Arhar <[email protected]> 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 <[email protected]> Reviewed-by: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1279321} -- wpt-commits: 5317488d13e2990de35a96e8fc3bf74fa7a2987b wpt-pr: 44339
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 <[email protected]> Reviewed-by: Joey Arhar <[email protected]> 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
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
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
(See WHATWG Working Mode: Changes for more details.)
/interactive-elements.html ( diff )
/popover.html ( diff )