Skip to content

Commit

Permalink
Bug 1878708 - Dialogs HideAllPopoversUntil nearest popover, not docum…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
keithamus committed Feb 5, 2024
1 parent 5f5f222 commit 5b10455
Show file tree
Hide file tree
Showing 8 changed files with 23 additions and 35 deletions.
8 changes: 6 additions & 2 deletions dom/base/Element.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4330,15 +4330,19 @@ bool Element::IsPopoverOpen() const {
return htmlElement && htmlElement->PopoverOpen();
}

Element* Element::GetTopmostPopoverAncestor(const Element* aInvoker) const {
Element* Element::GetTopmostPopoverAncestor(const Element* aInvoker,
bool isPopover) const {
const Element* newPopover = this;

nsTHashMap<nsPtrHashKey<const Element>, size_t> popoverPositions;
size_t index = 0;
for (Element* popover : OwnerDoc()->AutoPopoverList()) {
popoverPositions.LookupOrInsert(popover, index++);
}
popoverPositions.LookupOrInsert(newPopover, index);

if (isPopover) {
popoverPositions.LookupOrInsert(newPopover, index);
}

Element* topmostPopoverAncestor = nullptr;

Expand Down
3 changes: 2 additions & 1 deletion dom/base/Element.h
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,8 @@ class Element : public FragmentOrElement {
/**
* https://html.spec.whatwg.org/multipage/popover.html#topmost-popover-ancestor
*/
Element* GetTopmostPopoverAncestor(const Element* aInvoker) const;
Element* GetTopmostPopoverAncestor(const Element* aInvoker,
bool isPopover) const;

ElementAnimationData* GetAnimationData() const {
if (!MayHaveAnimations()) {
Expand Down
14 changes: 12 additions & 2 deletions dom/html/HTMLDialogElement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,12 @@ void HTMLDialogElement::Show(ErrorResult& aError) {

StorePreviouslyFocusedElement();

OwnerDoc()->HideAllPopoversWithoutRunningScript();
RefPtr<nsINode> hideUntil = GetTopmostPopoverAncestor(nullptr, false);
if (!hideUntil) {
hideUntil = OwnerDoc();
}

OwnerDoc()->HideAllPopoversUntil(*hideUntil, false, true);
FocusDialog();
}

Expand Down Expand Up @@ -130,7 +135,12 @@ void HTMLDialogElement::ShowModal(ErrorResult& aError) {

StorePreviouslyFocusedElement();

OwnerDoc()->HideAllPopoversWithoutRunningScript();
RefPtr<nsINode> hideUntil = GetTopmostPopoverAncestor(nullptr, false);
if (!hideUntil) {
hideUntil = OwnerDoc();
}

OwnerDoc()->HideAllPopoversUntil(*hideUntil, false, true);
FocusDialog();

aError.SuppressException();
Expand Down
4 changes: 2 additions & 2 deletions dom/html/HTMLDialogElement.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ class HTMLDialogElement final : public nsGenericHTMLElement {
void UnbindFromTree(bool aNullParent = true) override;

void Close(const mozilla::dom::Optional<nsAString>& aReturnValue);
void Show(ErrorResult& aError);
void ShowModal(ErrorResult& aError);
MOZ_CAN_RUN_SCRIPT void Show(ErrorResult& aError);
MOZ_CAN_RUN_SCRIPT void ShowModal(ErrorResult& aError);

bool IsInTopLayer() const;
void QueueCancelDialog();
Expand Down
2 changes: 1 addition & 1 deletion dom/html/nsGenericHTMLElement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3418,7 +3418,7 @@ void nsGenericHTMLElement::ShowPopoverInternal(Element* aInvoker,
nsWeakPtr originallyFocusedElement;
if (IsAutoPopover()) {
auto originalState = GetPopoverAttributeState();
RefPtr<nsINode> ancestor = GetTopmostPopoverAncestor(aInvoker);
RefPtr<nsINode> ancestor = GetTopmostPopoverAncestor(aInvoker, true);
if (!ancestor) {
ancestor = document;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
[popover-top-layer-nesting-anchor.tentative.html]
expected:
if (os == "mac") and not debug: TIMEOUT
[Single popover=auto ancestor with dialog]
expected: FAIL

[Single popover=auto ancestor with dialog, anchor attribute]
expected: FAIL

Expand All @@ -13,9 +10,6 @@
[Single popover=auto ancestor with fullscreen, anchor attribute]
expected: FAIL

[Single popover=manual ancestor with dialog]
expected: FAIL

[Single popover=manual ancestor with dialog, anchor attribute]
expected: FAIL

Expand All @@ -32,7 +26,6 @@
[Nested popover=auto ancestors with dialog]
expected:
if (os == "mac") and not debug: NOTRUN
FAIL

[Nested popover=auto ancestors with dialog, anchor attribute]
expected:
Expand All @@ -52,7 +45,6 @@
[Nested popover=auto ancestors, target is outer with dialog]
expected:
if (os == "mac") and not debug: NOTRUN
FAIL

[Nested popover=auto ancestors, target is outer with dialog, anchor attribute]
expected:
Expand All @@ -72,7 +64,6 @@
[Top layer inside of nested element with dialog]
expected:
if (os == "mac") and not debug: NOTRUN
FAIL

[Top layer inside of nested element with dialog, anchor attribute]
expected:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
[popover-top-layer-nesting-hints.tentative.html]
[Nested auto/hint ancestors with dialog]
expected: FAIL

[Nested auto/hint ancestors with fullscreen]
expected: FAIL

Expand Down
Original file line number Diff line number Diff line change
@@ -1,30 +1,15 @@
[popover-top-layer-nesting.tentative.html]
[Single popover=auto ancestor with dialog]
expected: FAIL

[Single popover=auto ancestor with fullscreen]
expected: FAIL

[Single popover=manual ancestor with dialog]
expected: FAIL

[Single popover=manual ancestor with fullscreen]
expected: FAIL

[Nested popover=auto ancestors with dialog]
expected: FAIL

[Nested popover=auto ancestors with fullscreen]
expected: FAIL

[Nested popover=auto ancestors, target is outer with dialog]
expected: FAIL

[Nested popover=auto ancestors, target is outer with fullscreen]
expected: FAIL

[Top layer inside of nested element with dialog]
expected: FAIL

[Top layer inside of nested element with fullscreen]
expected: FAIL

0 comments on commit 5b10455

Please sign in to comment.