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

UltraBlame original commit: 52284e30cea2c52ab073310e4010161702dc92e4
  • Loading branch information
marco-c committed Feb 8, 2024
1 parent c62df26 commit 05c02d4
Show file tree
Hide file tree
Showing 8 changed files with 73 additions and 105 deletions.
8 changes: 8 additions & 0 deletions dom/base/Element.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22810,6 +22810,8 @@ const
Element
*
aInvoker
bool
isPopover
)
const
{
Expand Down Expand Up @@ -22863,6 +22865,11 @@ index
)
;
}
if
(
isPopover
)
{
popoverPositions
.
LookupOrInsert
Expand All @@ -22871,6 +22878,7 @@ newPopover
index
)
;
}
Element
*
topmostPopoverAncestor
Expand Down
2 changes: 2 additions & 0 deletions dom/base/Element.h
Original file line number Diff line number Diff line change
Expand Up @@ -1910,6 +1910,8 @@ const
Element
*
aInvoker
bool
isPopover
)
const
;
Expand Down
62 changes: 60 additions & 2 deletions dom/html/HTMLDialogElement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -275,13 +275,42 @@ StorePreviouslyFocusedElement
(
)
;
RefPtr
<
nsINode
>
hideUntil
=
GetTopmostPopoverAncestor
(
nullptr
false
)
;
if
(
!
hideUntil
)
{
hideUntil
=
OwnerDoc
(
)
;
}
OwnerDoc
(
)
-
>
HideAllPopoversWithoutRunningScript
HideAllPopoversUntil
(
*
hideUntil
false
true
)
;
FocusDialog
Expand Down Expand Up @@ -598,13 +627,42 @@ StorePreviouslyFocusedElement
(
)
;
RefPtr
<
nsINode
>
hideUntil
=
GetTopmostPopoverAncestor
(
nullptr
false
)
;
if
(
!
hideUntil
)
{
hideUntil
=
OwnerDoc
(
)
;
}
OwnerDoc
(
)
-
>
HideAllPopoversWithoutRunningScript
HideAllPopoversUntil
(
*
hideUntil
false
true
)
;
FocusDialog
Expand Down
2 changes: 2 additions & 0 deletions dom/html/HTMLDialogElement.h
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ nsAString
aReturnValue
)
;
MOZ_CAN_RUN_SCRIPT
void
Show
(
Expand All @@ -207,6 +208,7 @@ ErrorResult
aError
)
;
MOZ_CAN_RUN_SCRIPT
void
ShowModal
(
Expand Down
1 change: 1 addition & 0 deletions dom/html/nsGenericHTMLElement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17749,6 +17749,7 @@ ancestor
GetTopmostPopoverAncestor
(
aInvoker
true
)
;
if
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,6 @@ auto
ancestor
with
dialog
]
expected
:
FAIL
[
Single
popover
=
auto
ancestor
with
dialog
anchor
attribute
]
Expand Down Expand Up @@ -89,18 +77,6 @@ manual
ancestor
with
dialog
]
expected
:
FAIL
[
Single
popover
=
manual
ancestor
with
dialog
anchor
attribute
]
Expand Down Expand Up @@ -189,7 +165,6 @@ not
debug
:
NOTRUN
FAIL
[
Nested
popover
Expand Down Expand Up @@ -300,7 +275,6 @@ not
debug
:
NOTRUN
FAIL
[
Nested
popover
Expand Down Expand Up @@ -418,7 +392,6 @@ not
debug
:
NOTRUN
FAIL
[
Top
layer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,6 @@ auto
hint
ancestors
with
dialog
]
expected
:
FAIL
[
Nested
auto
/
hint
ancestors
with
fullscreen
]
expected
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,6 @@ popover
auto
ancestor
with
dialog
]
expected
:
FAIL
[
Single
popover
=
auto
ancestor
with
fullscreen
]
expected
Expand All @@ -42,18 +30,6 @@ popover
manual
ancestor
with
dialog
]
expected
:
FAIL
[
Single
popover
=
manual
ancestor
with
fullscreen
]
expected
Expand All @@ -66,18 +42,6 @@ popover
auto
ancestors
with
dialog
]
expected
:
FAIL
[
Nested
popover
=
auto
ancestors
with
fullscreen
]
expected
Expand All @@ -93,21 +57,6 @@ target
is
outer
with
dialog
]
expected
:
FAIL
[
Nested
popover
=
auto
ancestors
target
is
outer
with
fullscreen
]
expected
Expand All @@ -121,19 +70,6 @@ of
nested
element
with
dialog
]
expected
:
FAIL
[
Top
layer
inside
of
nested
element
with
fullscreen
]
expected
Expand Down

0 comments on commit 05c02d4

Please sign in to comment.