Skip to content

Commit

Permalink
Update popover=hint behavior to allow a stack of hints
Browse files Browse the repository at this point in the history
The previous implementation only allowed one popover=hint to be open
at a time. Per conversation at [1], developers feel that it should be
possible to nest popover=hint popovers. This CL implements that
capability.

There are now two popover stacks in Document: PopoverAutoStack and
PopoverHintStack. Since it is possible to nest hints within autos,
the PopoverAutoStack can contain hints. However, there
are a few constraints:
 - The PopoverHintStack only ever contains hints.
 - Once the PopoverAutoStack contains a hint, all subsequent popovers
   in the stack must also be hints.
 - A popover=hint can never be the ancestor of a popover=auto.

The light dismiss behavior is roughly the same as before, with a
slight tweak that simplifies behavior: closing anything in the
PopoverAutoStack will always close everything in the PopoverHintStack.
That was not the case before, but it was a bit of a weird corner case.

Note that I found a crasher (happens in stable, with just auto
popovers) that I added a test for here. The bug for that is
crbug.com/1513282. I'll fix that in a followup.

[1] whatwg/html#9776 (comment)

Bug: 1416284,1513282
Change-Id: Ic064ecf1377bb8abfc812654c85016e6d1cbbdaf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5133909
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1246573}
  • Loading branch information
Mason Freed authored and Chromium LUCI CQ committed Jan 12, 2024
1 parent fa7a0ed commit 7323ed4
Show file tree
Hide file tree
Showing 14 changed files with 450 additions and 318 deletions.
24 changes: 9 additions & 15 deletions third_party/blink/renderer/core/dom/document.cc
Expand Up @@ -8151,22 +8151,16 @@ HTMLDialogElement* Document::ActiveModalDialog() const {
return nullptr;
}

void Document::SetPopoverHintShowing(HTMLElement* element) {
DCHECK(!element || element->HasPopoverAttribute());
DCHECK(RuntimeEnabledFeatures::HTMLPopoverHintEnabled());
popover_hint_showing_ = element;
}

HTMLElement* Document::TopmostPopoverOrHint() const {
if (PopoverHintShowing()) {
DCHECK(RuntimeEnabledFeatures::HTMLPopoverHintEnabled());
return PopoverHintShowing();
if (!PopoverHintStack().empty()) {
CHECK(RuntimeEnabledFeatures::HTMLPopoverHintEnabled());
return PopoverHintStack().back();
}
if (PopoverStack().empty())
return nullptr;
return PopoverStack().back();
if (!PopoverAutoStack().empty()) {
return PopoverAutoStack().back();
}
return nullptr;
}

void Document::SetPopoverPointerdownTarget(const HTMLElement* popover) {
DCHECK(!popover || popover->HasPopoverAttribute());
popover_pointerdown_target_ = popover;
Expand Down Expand Up @@ -8902,8 +8896,8 @@ void Document::Trace(Visitor* visitor) const {
visitor->Trace(node_lists_);
visitor->Trace(top_layer_elements_);
visitor->Trace(top_layer_elements_pending_removal_);
visitor->Trace(popover_stack_);
visitor->Trace(popover_hint_showing_);
visitor->Trace(popover_auto_stack_);
visitor->Trace(popover_hint_stack_);
visitor->Trace(popover_pointerdown_target_);
visitor->Trace(popovers_waiting_to_hide_);
visitor->Trace(all_open_popovers_);
Expand Down
34 changes: 22 additions & 12 deletions third_party/blink/renderer/core/dom/document.h
Expand Up @@ -1577,15 +1577,20 @@ class CORE_EXPORT Document : public ContainerNode,

HTMLDialogElement* ActiveModalDialog() const;

HTMLElement* PopoverHintShowing() const {
return popover_hint_showing_.Get();
const HeapVector<Member<HTMLElement>>& PopoverHintStack() const {
return popover_hint_stack_;
}
void SetPopoverHintShowing(HTMLElement* element);
HeapVector<Member<HTMLElement>>& PopoverStack() { return popover_stack_; }
const HeapVector<Member<HTMLElement>>& PopoverStack() const {
return popover_stack_;
HeapVector<Member<HTMLElement>>& PopoverHintStack() {
return popover_hint_stack_;
}
bool PopoverAutoShowing() const { return !popover_stack_.empty(); }
bool PopoverHintShowing() const { return !popover_hint_stack_.empty(); }
HeapVector<Member<HTMLElement>>& PopoverAutoStack() {
return popover_auto_stack_;
}
const HeapVector<Member<HTMLElement>>& PopoverAutoStack() const {
return popover_auto_stack_;
}
bool PopoverAutoShowing() const { return !popover_auto_stack_.empty(); }
HeapHashSet<Member<HTMLElement>>& AllOpenPopovers() {
return all_open_popovers_;
}
Expand Down Expand Up @@ -2542,11 +2547,16 @@ class CORE_EXPORT Document : public ContainerNode,
};
VectorOf<TopLayerPendingRemoval> top_layer_elements_pending_removal_;

// The stack of currently-displayed `popover=auto` elements. Elements in the
// stack go from earliest (bottom-most) to latest (top-most).
HeapVector<Member<HTMLElement>> popover_stack_;
// The `popover=hint` that is currently showing, if any.
Member<HTMLElement> popover_hint_showing_;
// The stack of currently-displayed popover elements that descend from a root
// `popover=auto` element. Elements in the stack go from earliest
// (bottom-most) to latest (top-most). Note that `popover=hint` elements can
// exist in this stack, but there will never be a `popover=auto` that comes
// after that in the stack.
HeapVector<Member<HTMLElement>> popover_auto_stack_;
// The stack of currently-displayed `popover=hint` elements. Ordering in the
// stack is the same as for `popover_auto_stack_`. This stack will only ever
// contain `popover=hint` elements, and nothing else.
HeapVector<Member<HTMLElement>> popover_hint_stack_;
// The popover (if any) that received the most recent pointerdown event.
Member<const HTMLElement> popover_pointerdown_target_;
// A set of popovers for which hidePopover() has been called, but animations
Expand Down
3 changes: 1 addition & 2 deletions third_party/blink/renderer/core/fullscreen/fullscreen.cc
Expand Up @@ -228,8 +228,7 @@ void GoFullscreen(Element& element,
// If there are any open popovers, close them.
HTMLElement::HideAllPopoversUntil(
nullptr, document, HidePopoverFocusBehavior::kNone,
HidePopoverTransitionBehavior::kFireEventsAndWaitForTransitions,
HidePopoverIndependence::kHideUnrelated);
HidePopoverTransitionBehavior::kFireEventsAndWaitForTransitions);

// To fullscreen an |element| within a |document|, set the |element|'s
// fullscreen flag and add it to |document|'s top layer.
Expand Down
6 changes: 2 additions & 4 deletions third_party/blink/renderer/core/html/html_dialog_element.cc
Expand Up @@ -221,8 +221,7 @@ void HTMLDialogElement::show(ExceptionState& exception_state) {
auto& document = GetDocument();
HTMLElement::HideAllPopoversUntil(
nullptr, document, HidePopoverFocusBehavior::kNone,
HidePopoverTransitionBehavior::kFireEventsAndWaitForTransitions,
HidePopoverIndependence::kHideUnrelated);
HidePopoverTransitionBehavior::kFireEventsAndWaitForTransitions);

if (RuntimeEnabledFeatures::DialogNewFocusBehaviorEnabled()) {
SetFocusForDialog();
Expand Down Expand Up @@ -320,8 +319,7 @@ void HTMLDialogElement::showModal(ExceptionState& exception_state) {
// Showing a <dialog> should hide all open popovers.
HTMLElement::HideAllPopoversUntil(
nullptr, document, HidePopoverFocusBehavior::kNone,
HidePopoverTransitionBehavior::kFireEventsAndWaitForTransitions,
HidePopoverIndependence::kHideUnrelated);
HidePopoverTransitionBehavior::kFireEventsAndWaitForTransitions);

if (RuntimeEnabledFeatures::DialogNewFocusBehaviorEnabled()) {
SetFocusForDialog();
Expand Down

0 comments on commit 7323ed4

Please sign in to comment.