Skip to content
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

Merged
merged 7 commits into from
Mar 26, 2024

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Jan 31, 2024

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 )

josepharhar added a commit to josepharhar/fullscreen that referenced this pull request Jan 31, 2024
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
Copy link
Contributor

@mfreed7 mfreed7 left a 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.

source Outdated Show resolved Hide resolved
@keithamus
Copy link
Contributor

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

@domenic domenic added normative change topic: popover The popover attribute and friends labels Feb 6, 2024
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic domenic self-assigned this Feb 6, 2024
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 6, 2024
…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
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Feb 7, 2024
…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
@keithamus keithamus requested a review from domenic February 7, 2024 09:44
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Feb 8, 2024
…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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Feb 8, 2024
…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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Feb 8, 2024
…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
Copy link
Contributor

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

@josepharhar
Copy link
Contributor Author

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

done

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 10, 2024
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
Copy link
Contributor

@mfreed7 mfreed7 left a 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.

source Outdated Show resolved Hide resolved
@mfreed7
Copy link
Contributor

mfreed7 commented Mar 4, 2024

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
Copy link
Contributor

Copy link
Member

@domenic domenic left a 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.

@domenic domenic added the topic: dialog The <dialog> element label Mar 5, 2024
@mfreed7
Copy link
Contributor

mfreed7 commented Mar 7, 2024

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
Copy link
Contributor Author

@domenic I updated it, we can merge now

@domenic domenic merged commit ae5a7ec into whatwg:main Mar 26, 2024
2 checks passed
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 26, 2024
aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 27, 2024
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}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 27, 2024
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}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 28, 2024
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}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 8, 2024
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
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Apr 15, 2024
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
domenic pushed a commit to whatwg/fullscreen that referenced this pull request May 7, 2024
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
webkit-commit-queue pushed a commit to keithamus/WebKit that referenced this pull request Jun 10, 2024
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
mnutt pushed a commit to movableink/webkit that referenced this pull request Aug 27, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative change topic: dialog The <dialog> element topic: popover The popover attribute and friends
4 participants