From 9e8f08fcf2d8637444794058b5cc7181fcc8de26 Mon Sep 17 00:00:00 2001 From: Mason Freed Date: Wed, 6 Jul 2022 20:01:16 +0000 Subject: [PATCH] Update the pop-up implementation to match hint/auto resolutions See the set of behaviors described here: https://github.com/openui/open-ui/issues/525#issuecomment-1119093412 which were resolved here: https://github.com/openui/open-ui/issues/525#issuecomment-1125293226 This CL implements those changes in behavior, which mostly deal with how popup=auto and popup=hint interact, and some small changes to how `defaultopen` works. Bug: 1307772 Change-Id: I4d280b60e7c341b4d0f97fe82e60134ff4a6e1fa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3742105 Auto-Submit: Mason Freed Reviewed-by: Joey Arhar Commit-Queue: Mason Freed Cr-Commit-Position: refs/heads/main@{#1021327} NOKEYCHECK=True GitOrigin-RevId: 033ae102a356d906af6f62d3a31588f0b2fc4b18 --- blink/renderer/core/dom/document.cc | 15 +- blink/renderer/core/dom/document.h | 23 +- blink/renderer/core/dom/element.cc | 407 ++++++++++++------ blink/renderer/core/dom/element.h | 18 +- blink/renderer/core/dom/node.cc | 2 +- blink/renderer/core/fullscreen/fullscreen.cc | 8 +- .../renderer/core/html/html_dialog_element.cc | 6 +- .../popup-attribute-basic.tentative.html | 37 +- .../popup-defaultopen-hints.tentative.html | 20 + .../popups/popup-defaultopen.tentative.html | 2 - .../popups/popup-focus.tentative.html | 25 +- .../popups/popup-light-dismiss.tentative.html | 28 +- .../popups/popup-types.tentative.html | 248 +++++++---- .../semantics/popups/resources/popup-utils.js | 4 + 14 files changed, 563 insertions(+), 280 deletions(-) create mode 100644 blink/web_tests/external/wpt/html/semantics/popups/popup-defaultopen-hints.tentative.html diff --git a/blink/renderer/core/dom/document.cc b/blink/renderer/core/dom/document.cc index 63f5b25e6f4..d786c39f959 100644 --- a/blink/renderer/core/dom/document.cc +++ b/blink/renderer/core/dom/document.cc @@ -7260,12 +7260,12 @@ HTMLDialogElement* Document::ActiveModalDialog() const { return nullptr; } -bool Document::PopupOrHintShowing() const { - return !popup_and_hint_stack_.IsEmpty(); -} -bool Document::HintShowing() const { - return !popup_and_hint_stack_.IsEmpty() && - (popup_and_hint_stack_.back()->PopupType() == PopupValueType::kHint); +Element* Document::TopmostPopupAutoOrHint() const { + if (PopupHintShowing()) + return PopupHintShowing(); + if (PopupStack().IsEmpty()) + return nullptr; + return PopupStack().back(); } void Document::exitPointerLock() { @@ -8019,7 +8019,8 @@ void Document::Trace(Visitor* visitor) const { visitor->Trace(lists_invalidated_at_document_); visitor->Trace(node_lists_); visitor->Trace(top_layer_elements_); - visitor->Trace(popup_and_hint_stack_); + visitor->Trace(popup_hint_showing_); + visitor->Trace(popup_stack_); visitor->Trace(popups_waiting_to_hide_); visitor->Trace(load_event_delay_timer_); visitor->Trace(plugin_loading_timer_); diff --git a/blink/renderer/core/dom/document.h b/blink/renderer/core/dom/document.h index 223cca24a3d..cbdfcaf5535 100644 --- a/blink/renderer/core/dom/document.h +++ b/blink/renderer/core/dom/document.h @@ -73,6 +73,7 @@ #include "third_party/blink/renderer/platform/heap/collection_support/heap_hash_map.h" #include "third_party/blink/renderer/platform/heap/collection_support/heap_hash_set.h" #include "third_party/blink/renderer/platform/heap/collection_support/heap_linked_hash_set.h" +#include "third_party/blink/renderer/platform/heap/collection_support/heap_vector.h" #include "third_party/blink/renderer/platform/heap_observer_set.h" #include "third_party/blink/renderer/platform/instrumentation/use_counter.h" #include "third_party/blink/renderer/platform/scheduler/public/post_cancellable_task.h" @@ -1515,14 +1516,15 @@ class CORE_EXPORT Document : public ContainerNode, HTMLDialogElement* ActiveModalDialog() const; - HeapVector>& PopupAndHintStack() { - return popup_and_hint_stack_; - } + Element* PopupHintShowing() const { return popup_hint_showing_; } + void SetPopupHintShowing(Element* element) { popup_hint_showing_ = element; } + HeapVector>& PopupStack() { return popup_stack_; } + const HeapVector>& PopupStack() const { return popup_stack_; } + bool PopupAutoShowing() const { return !popup_stack_.IsEmpty(); } + Element* TopmostPopupAutoOrHint() const; HeapHashSet>& PopupsWaitingToHide() { return popups_waiting_to_hide_; } - bool PopupOrHintShowing() const; - bool HintShowing() const; // A non-null template_document_host_ implies that |this| was created by // EnsureTemplateDocument(). @@ -2318,12 +2320,11 @@ class CORE_EXPORT Document : public ContainerNode, // stack and is thus the one that will be visually on top. HeapVector> top_layer_elements_; - // The stack of currently-displayed Popup (and Hint) elements, which are - // elements that have either `popup=popup` or `popup=hint`. Elements in the - // stack go from earliest (bottom-most) to latest (top-most). If there is a - // hint in the stack, it is at the top. - HeapVector> popup_and_hint_stack_; - + // The stack of currently-displayed `popup=auto` elements. Elements in the + // stack go from earliest (bottom-most) to latest (top-most). + HeapVector> popup_stack_; + // The `popup=hint` that is currently showing, if any. + Member popup_hint_showing_; // A set of popups for which hidePopUp() has been called, but animations are // still running. HeapHashSet> popups_waiting_to_hide_; diff --git a/blink/renderer/core/dom/element.cc b/blink/renderer/core/dom/element.cc index 2b2bcd6d5a1..e14031904dc 100644 --- a/blink/renderer/core/dom/element.cc +++ b/blink/renderer/core/dom/element.cc @@ -133,6 +133,7 @@ #include "third_party/blink/renderer/core/html/custom/custom_element_registry.h" #include "third_party/blink/renderer/core/html/forms/html_button_element.h" #include "third_party/blink/renderer/core/html/forms/html_field_set_element.h" +#include "third_party/blink/renderer/core/html/forms/html_form_control_element.h" #include "third_party/blink/renderer/core/html/forms/html_form_controls_collection.h" #include "third_party/blink/renderer/core/html/forms/html_options_collection.h" #include "third_party/blink/renderer/core/html/forms/html_select_element.h" @@ -2508,32 +2509,56 @@ void Element::showPopUp(ExceptionState& exception_state) { } bool should_restore_focus = false; + auto& document = GetDocument(); if (PopupType() == PopupValueType::kAuto || PopupType() == PopupValueType::kHint) { - if (GetDocument().HintShowing()) { - GetDocument().PopupAndHintStack().back()->HidePopUpInternal( - HidePopupFocusBehavior::kNone, - HidePopupForcingLevel::kHideAfterAnimations); + if (PopupType() == PopupValueType::kHint) { + // If the new pop-up is popup=hint, hide other hints first. + if (document.PopupHintShowing()) { + document.PopupHintShowing()->HidePopUpInternal( + HidePopupFocusBehavior::kNone, + HidePopupForcingLevel::kHideAfterAnimations); + } + // Then hide open pop-ups that aren't ancestors of this hint. + if (const Element* hint_ancestor = NearestOpenAncestralPopup(this)) { + HideAllPopupsUntil(hint_ancestor, document, + HidePopupFocusBehavior::kNone, + HidePopupForcingLevel::kHideAfterAnimations, + HidePopupIndependence::kHideUnrelated); + } + } else { + // If the new pop-up is a popup=auto, hide any pop-up above this in the + // stack, and hide any hint pop-ups. Because this pop-up isn't yet in the + // stack, we call NearestOpenAncestralPopup to find this pop-up's + // ancestor, if any. + const Element* auto_ancestor = NearestOpenAncestralPopup(this); + HideAllPopupsUntil(auto_ancestor, document, HidePopupFocusBehavior::kNone, + HidePopupForcingLevel::kHideAfterAnimations, + HidePopupIndependence::kHideUnrelated); } - if (PopupType() == PopupValueType::kAuto) { - // Only hide other popups up to this popup's ancestral popup. - HideAllPopupsUntil(NearestOpenAncestralPopup(this), GetDocument(), - HidePopupFocusBehavior::kNone, - HidePopupForcingLevel::kHideAfterAnimations); - } - // Add this popup to the popup stack. - auto& stack = GetDocument().PopupAndHintStack(); - DCHECK(!stack.Contains(this)); + + // The 'hide' event handlers could have changed this popup, e.g. by changing + // its type, removing it from the document, or calling showPopUp(). + if (!HasValidPopupAttribute() || !isConnected() || popupOpen()) + return; + // We only restore focus for popup/hint, and only for the first popup in - // the stack. - should_restore_focus = stack.IsEmpty(); - stack.push_back(this); + // the stack. If there's nothing showing, restore focus. + should_restore_focus = !document.TopmostPopupAutoOrHint(); + if (PopupType() == PopupValueType::kAuto) { + // Add this popup to the popup stack. + auto& stack = document.PopupStack(); + DCHECK(!stack.Contains(this)); + stack.push_back(this); + } else { + document.SetPopupHintShowing(this); + } } GetPopupData()->setAnimationFinishedListener(nullptr); GetPopupData()->setPreviouslyFocusedElement( - should_restore_focus ? GetDocument().FocusedElement() : nullptr); - GetDocument().AddToTopLayer(this); + should_restore_focus ? document.FocusedElement() : nullptr); + document.AddToTopLayer(this); // Remove display:none styling: GetPopupData()->setVisibilityState(PopupVisibilityState::kTransitioning); PseudoStateChanged(CSSSelector::kPseudoPopupHidden); @@ -2541,7 +2566,7 @@ void Element::showPopUp(ExceptionState& exception_state) { // Force a style update. This ensures that base property values are set prior // to `:top-layer` matching, so that transitions can start on the change to // top layer. - GetDocument().UpdateStyleAndLayoutTreeForNode(this); + document.UpdateStyleAndLayoutTreeForNode(this); EnsureComputedStyle(); // Make the popup match :top-layer: @@ -2557,22 +2582,65 @@ void Element::showPopUp(ExceptionState& exception_state) { } // static +// All pop-ups up to, but not including, |endpoint|, will be hidden. If there +// are "unrelated" pop-ups open, such as a stack of popup=auto pop-ups and +// |endpoint| is a popup=hint, then the popup_independence argument controls +// whether those unrelated popup=auto pop-ups are hidden. void Element::HideAllPopupsUntil(const Element* endpoint, Document& document, HidePopupFocusBehavior focus_behavior, - HidePopupForcingLevel forcing_level) { + HidePopupForcingLevel forcing_level, + HidePopupIndependence popup_independence) { DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled()); + DCHECK(!endpoint || endpoint->HasValidPopupAttribute()); + // If we're forcing a popup to hide immediately, first hide any other popups // that have already started the hide process. if (forcing_level == HidePopupForcingLevel::kHideImmediately) { auto popups_to_hide = document.PopupsWaitingToHide(); for (auto popup : popups_to_hide) popup->PopupHideFinishIfNeeded(); + DCHECK(document.PopupsWaitingToHide().IsEmpty()); } - while (!document.PopupAndHintStack().IsEmpty() && - document.PopupAndHintStack().back() != endpoint) { - document.PopupAndHintStack().back()->HidePopUpInternal(focus_behavior, - forcing_level); + + if (endpoint && endpoint->PopupType() == PopupValueType::kHint) { + if (popup_independence == HidePopupIndependence::kHideUnrelated) { + if (document.PopupHintShowing() != endpoint) { + document.PopupHintShowing()->HidePopUpInternal(focus_behavior, + forcing_level); + } + while (!document.PopupStack().IsEmpty() && + document.PopupStack().back() != endpoint) { + document.PopupStack().back()->HidePopUpInternal(focus_behavior, + forcing_level); + } + } + } else { + DCHECK(!endpoint || endpoint->PopupType() == PopupValueType::kAuto); + const Element* hint_ancestor = nullptr; + if (document.PopupHintShowing()) { + // If there is a hint showing that is a descendent of something on the + // stack, then the hint should be hidden before that ancestor is hidden, + // regardless of popup_independence. + hint_ancestor = NearestOpenAncestralPopup(document.PopupHintShowing()); + if (!hint_ancestor && + popup_independence == HidePopupIndependence::kHideUnrelated) { + document.PopupHintShowing()->HidePopUpInternal(focus_behavior, + forcing_level); + } + } + // Then hide everything in the popup=auto stack up to the specified + // endpoint. + while (!document.PopupStack().IsEmpty()) { + if (document.PopupStack().back() == hint_ancestor) { + document.PopupHintShowing()->HidePopUpInternal(focus_behavior, + forcing_level); + } + if (document.PopupStack().back() == endpoint) + break; + document.PopupStack().back()->HidePopUpInternal(focus_behavior, + forcing_level); + } } } @@ -2583,10 +2651,14 @@ void Element::hidePopUp(ExceptionState& exception_state) { DOMExceptionCode::kNotSupportedError, "Not supported on elements that do not have a valid value for the " "'popup' attribute"); - } else if (!popupOpen()) { + } else if (GetPopupData()->visibilityState() != + PopupVisibilityState::kShowing) { + // Important to check that visibility is not kShowing (rather than + // popupOpen()), because a hide transition might have been started on this + // pop-up already, and we don't want to allow a double-hide. return exception_state.ThrowDOMException( DOMExceptionCode::kInvalidStateError, - "Invalid on already-hidden popup elements"); + "Invalid on popup elements that aren't already showing"); } HidePopUpInternal(HidePopupFocusBehavior::kFocusPreviousElement, HidePopupForcingLevel::kHideAfterAnimations); @@ -2610,22 +2682,34 @@ void Element::HidePopUpInternal(HidePopupFocusBehavior focus_behavior, HidePopupForcingLevel forcing_level) { DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled()); DCHECK(HasValidPopupAttribute()); + auto& document = GetDocument(); if (PopupType() == PopupValueType::kAuto || PopupType() == PopupValueType::kHint) { // Hide any popups/hints above us in the stack. - HideAllPopupsUntil(this, GetDocument(), focus_behavior, forcing_level); + HideAllPopupsUntil(this, document, focus_behavior, forcing_level, + HidePopupIndependence::kLeaveUnrelated); + + // The 'hide' event handlers could have changed this popup, e.g. by changing + // its type, removing it from the document, or calling hidePopUp(). + if (!HasValidPopupAttribute() || !isConnected() || + GetPopupData()->visibilityState() != PopupVisibilityState::kShowing) { + DCHECK(!GetDocument().PopupStack().Contains(this)); + return; + } + // Then remove this popup/hint from the stack, if present. If the popup // is already hidden, it won't be in the stack. - auto& stack = GetDocument().PopupAndHintStack(); - if (!stack.IsEmpty() && stack.back() == this) + if (PopupType() == PopupValueType::kAuto) { + auto& stack = document.PopupStack(); + DCHECK(!stack.IsEmpty()); + DCHECK_EQ(stack.back(), this); stack.pop_back(); - DCHECK(stack.IsEmpty() || !stack.Contains(this)); - } - if (!isConnected() || !popupOpen()) { - DCHECK(!GetDocument().PopupAndHintStack().Contains(this)); - return; + } else { + DCHECK_EQ(document.TopmostPopupAutoOrHint(), this); + document.SetPopupHintShowing(nullptr); + } } - GetDocument().PopupsWaitingToHide().insert(this); + document.PopupsWaitingToHide().insert(this); bool force_hide = forcing_level == HidePopupForcingLevel::kHideImmediately; HeapVector> previous_animations; @@ -2772,102 +2856,132 @@ Element* Element::GetPopupFocusableArea(bool autofocus_only) const { return nullptr; } -// static -const Element* Element::NearestOpenAncestralPopup(Node* start_node) { - DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled()); - if (!start_node) +namespace { +const Element* NearestOpenAncestralPopupRecursive( + const Node* node, + const HeapHashMap, int>& popup_positions, + const HeapHashMap, Member>& + anchors_to_popups, + int upper_bound, + HashSet>& seen) { + if (!node || seen.Contains(node)) return nullptr; - // We need to walk up from the start node to see if there is a parent popup, - // or the anchor for a popup, or an invoking element (which has one of the - // togglepopup/showpopup/hidepopup attribute). There can be multiple popups - // for a single anchor element, and an anchor for one popup can also be an - // invoker for a different popup. We need to stop on the highest such popup in - // the popup stack. Additionally, if start_node is inside an element that has - // an invoking attribute (e.g. togglepopup) but wasn't *used* to invoke that - // popup, we still need to stop on that popup, so that a click on that - // invoking element doesn't immediately light-dismiss its target. - - // |anchors_and_invokers| is a map from anchors/invokers to their popups, for - // all open popups. - HeapHashMap, Member> - anchors_and_invokers; - // |popup_position| is a map from popups to their position in the stack. - HeapHashMap, int> popup_position; - int indx = 0; - Document& document = start_node->GetDocument(); - for (auto popup : document.PopupAndHintStack()) { - DCHECK(popup->PopupType() == PopupValueType::kAuto || - popup->PopupType() == PopupValueType::kHint); - popup_position.Set(popup, indx++); - if (const auto* anchor = popup->anchorElement()) - anchors_and_invokers.Set(anchor, popup); - if (const auto* invoker = popup->GetPopupData()->invoker()) - anchors_and_invokers.Set(invoker, popup); - } - - // This keeps track of the highest-in-stack popup we've seen. - const Element* highest_popup = nullptr; - auto update_highest = [popup_position, &highest_popup](const Element* popup) { - DCHECK(popup->HasValidPopupAttribute()); - DCHECK(popup_position.Contains(popup)); - if (!highest_popup || - popup_position.at(popup) > popup_position.at(highest_popup)) { - highest_popup = popup; + seen.insert(node); + + const Element* ancestor = nullptr; + int position = -1; + auto update = [&ancestor, &position, &popup_positions, + upper_bound](const Element* popup) { + if (popup && popup->popupOpen()) { + DCHECK(popup_positions.Contains(popup)); + int new_position = popup_positions.at(popup); + if (new_position > position && new_position < upper_bound) { + ancestor = popup; + position = new_position; + } } }; + auto recurse_and_update = [&update, &popup_positions, upper_bound, + &anchors_to_popups, &seen](const Node* node) { + update(NearestOpenAncestralPopupRecursive( + node, popup_positions, anchors_to_popups, upper_bound, seen)); + }; - // Walk up from the start_node. Four things can happen: - // 1. We encounter a showing popup. - // 2. We encounter an element that invoked a showing popup. - // 3. We encounter the anchor element for a showing popup. - // 4. We encounter an invoking element that points to a showing popup, but - // which didn't invoke it. - // Keep track of the highest (on the popup stack) popup of any of these. - for (Node* current_node = start_node; current_node; - current_node = FlatTreeTraversal::Parent(*current_node)) { - auto* current_element = DynamicTo(current_node); - if (!current_element) - continue; - if (current_element->HasValidPopupAttribute() && - current_element->popupOpen() && - (current_element->PopupType() == PopupValueType::kAuto || - current_element->PopupType() == PopupValueType::kHint)) { - // Case #1: a showing popup. - update_highest(current_element); - } else if (anchors_and_invokers.Contains(current_element)) { - // Case #2 or 3: An anchor or trigger for a showing popup. - update_highest(anchors_and_invokers.at(current_element)); - } else if (auto* button = DynamicTo(current_element)) { - if (auto invoked_popup = button->togglePopupElement().element; - invoked_popup && popup_position.Contains(invoked_popup)) { - // Case #4: An invoking element pointing to a showing popup. - update_highest(invoked_popup); - } + if (auto* element = DynamicTo(node)) { + // Update for this element. + update(element); + // Recursively look up the tree from this element's anchors and invokers. + if (popup_positions.Contains(element)) { + recurse_and_update(element->anchorElement()); + recurse_and_update(element->GetPopupData()->invoker()); + } + // Include invokers that weren't used to invoke the popup. This is necessary + // to catch invoking elements that should not light dismiss a pop-up, even + // if they weren't used to show it. + if (auto* form_control = DynamicTo(element)) { + recurse_and_update(form_control->togglePopupElement().element); + } + // Include the anchor elements for all showing pop-ups. + if (anchors_to_popups.Contains(element)) { + recurse_and_update(anchors_to_popups.at(element)); } } + // Also walk up the flat tree from this node. + recurse_and_update(FlatTreeTraversal::Parent(*node)); - // If the starting element is a closed popup, we need to check for ancestors - // of *its* anchor and invoking element also. This happens when we're showing - // a new popup and try to close existing popups - we don't want to hide popups - // containing this popup's invoker or anchor. - if (const auto* start_element = DynamicTo(start_node)) { - // If this popup is open, we've already handled it above. Any other - // ancestors we would find here would necessarily be lower in the stack than - // this popup. - if (start_element->HasValidPopupAttribute() && - !start_element->popupOpen()) { - if (auto* anchor_ancestor = - NearestOpenAncestralPopup(start_element->anchorElement())) { - update_highest(anchor_ancestor); - } - if (auto* invoker_ancestor = NearestOpenAncestralPopup( - start_element->GetPopupData()->invoker())) { - update_highest(invoker_ancestor); + return ancestor; +} +} // namespace + +// static +// This function will return the pop-up that is highest in the pop-up stack that +// is an ancestral pop-up of the provided node. Pop-up ancestors are created by +// DOM flat tree parents, or through either anchor or invoker relationships. +// Anchor relationships are formed by the anchor attribute on a pop-up, pointing +// to another node in the tree. Invoker relationships are formed by invoking +// elements, which are HTMLFormControlElements having togglepopup, showpopup, or +// hidepopup attributes pointing to a pop-up element. There can be multiple +// pop-ups that point to a single anchor element, and there can be multiple +// invoking elements for a single pop-up. Additionally, an anchor for one pop-up +// can be an invoker for a different pop-up. For these reasons, this function +// needs to do a recursive tree walk up from the provided node, plus all +// associated anchors and invokers, returning the highest (on the stack) pop-up +// that is found. If the inclusive parameter is true, the highest pop-up found +// during the tree-walk is included in the search. If it is false, the |node| +// parameter must be a pop-up, and the highest pop-up *below* that starting pop- +// up will be returned. +const Element* Element::NearestOpenAncestralPopup(const Node* node, + bool inclusive) { + DCHECK(node); + // popup_positions is a map from all showing (or about-to-show) pop-ups to + // their position in the pop-up stack. + HeapHashMap, int> popup_positions; + // anchors_to_popups is a map from the anchor elements of all showing pop-ups + // back to the pop-up itself. + HeapHashMap, Member> anchors_to_popups; + int indx = 0; + for (auto popup : node->GetDocument().PopupStack()) { + popup_positions.Set(popup, indx++); + if (popup->anchorElement()) + anchors_to_popups.Set(popup->anchorElement(), popup); + } + auto* hint_showing = node->GetDocument().PopupHintShowing(); + if (hint_showing) { + popup_positions.Set(hint_showing, indx++); + if (hint_showing->anchorElement()) { + anchors_to_popups.Set(hint_showing->anchorElement(), hint_showing); + } + } + auto* element = DynamicTo(node); + bool new_element = + element && element->HasValidPopupAttribute() && !element->popupOpen(); + if (new_element) { + DCHECK(!inclusive); + popup_positions.Set(element, indx++); + } + // upper_bound is one above the maximum pop-up stack height to accept. It is + // typically the position of the provided element. + int upper_bound = + popup_positions.Contains(element) ? popup_positions.at(element) : INT_MAX; + if (hint_showing && new_element) { + upper_bound = popup_positions.at(hint_showing); // Do not include the hint + } + if (inclusive) { + // For inclusive mode, we need to walk up the tree until we find an open + // pop-up, and then modify the upper bound to include that pop-up, if found. + for (const Node* current_node = node; current_node; + current_node = FlatTreeTraversal::Parent(*current_node)) { + if (auto* current_element = DynamicTo(current_node); + current_element && current_element->HasValidPopupAttribute() && + current_element->popupOpen()) { + upper_bound = popup_positions.at(current_element) + 1; // Include it. + break; } } } - - return highest_popup; + HashSet> seen; + return NearestOpenAncestralPopupRecursive( + node, popup_positions, anchors_to_popups, upper_bound, seen); } // static @@ -2884,7 +2998,7 @@ void Element::HandlePopupLightDismiss(const Event& event) { if (!target_node) return; auto& document = target_node->GetDocument(); - DCHECK(document.PopupOrHintShowing()); + DCHECK(document.TopmostPopupAutoOrHint()); const AtomicString& event_type = event.type(); if (event_type == event_type_names::kMousedown) { // - Hide everything up to the clicked element. We do this on mousedown, @@ -2892,22 +3006,30 @@ void Element::HandlePopupLightDismiss(const Event& event) { // 1. This mirrors typical platform popups, which dismiss on mousedown. // 2. This allows a mouse-drag that starts on a popup and finishes off // the popup, without light-dismissing the popup. - HideAllPopupsUntil(NearestOpenAncestralPopup(target_node), document, - HidePopupFocusBehavior::kNone, - HidePopupForcingLevel::kHideAfterAnimations); + + // For a clicked node, hide all pop-ups outside the clicked pop-up tree, + // including unrelated pop-ups. + HideAllPopupsUntil( + NearestOpenAncestralPopup(target_node, /*inclusive*/ true), document, + HidePopupFocusBehavior::kNone, + HidePopupForcingLevel::kHideAfterAnimations, + HidePopupIndependence::kHideUnrelated); } else if (event_type == event_type_names::kKeydown) { const KeyboardEvent* key_event = DynamicTo(event); if (key_event && key_event->key() == "Escape") { // Escape key just pops the topmost popup or hint off the stack. - document.PopupAndHintStack().back()->HidePopUpInternal( + document.TopmostPopupAutoOrHint()->HidePopUpInternal( HidePopupFocusBehavior::kFocusPreviousElement, HidePopupForcingLevel::kHideAfterAnimations); } } else if (event_type == event_type_names::kFocusin) { - // If we focus an element, hide all popups that don't contain that element. - HideAllPopupsUntil(NearestOpenAncestralPopup(target_node), document, - HidePopupFocusBehavior::kNone, - HidePopupForcingLevel::kHideAfterAnimations); + // If we focus an element, hide all pop-ups outside the that element's + // pop-up tree, including unrelated pop-ups. + HideAllPopupsUntil( + NearestOpenAncestralPopup(target_node, /*inclusive*/ true), document, + HidePopupFocusBehavior::kNone, + HidePopupForcingLevel::kHideAfterAnimations, + HidePopupIndependence::kHideUnrelated); } } @@ -2920,9 +3042,10 @@ void Element::InvokePopup(Element* invoker) { } Element* Element::anchorElement() const { - if (!RuntimeEnabledFeatures::HTMLPopupAttributeEnabled()) { + if (!RuntimeEnabledFeatures::HTMLPopupAttributeEnabled()) + return nullptr; + if (!HasValidPopupAttribute()) return nullptr; - } const AtomicString& anchor_id = FastGetAttribute(html_names::kAnchorAttr); if (anchor_id.IsNull()) return nullptr; @@ -3245,18 +3368,20 @@ Node::InsertionNotificationRequest Element::InsertedInto( DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled()); DCHECK(isConnected()); GetPopupData()->setHadDefaultOpenWhenParsed(false); + auto maybe_show_popup = [](Element* popup) { + // The `defaultopen` attribute can only be used on popup=manual and + // popup=auto pop-ups. + if (popup && popup->isConnected() && + (popup->PopupType() == PopupValueType::kManual || + (popup->PopupType() == PopupValueType::kAuto && + !popup->GetDocument().PopupAutoShowing()))) { + popup->showPopUp(ASSERT_NO_EXCEPTION); + } + }; GetDocument() .GetTaskRunner(TaskType::kDOMManipulation) ->PostTask(FROM_HERE, - WTF::Bind( - [](Element* popup) { - if (popup && popup->isConnected() && - (popup->PopupType() == PopupValueType::kManual || - !popup->GetDocument().PopupOrHintShowing())) { - popup->showPopUp(ASSERT_NO_EXCEPTION); - } - }, - WrapWeakPersistent(this))); + WTF::Bind(maybe_show_popup, WrapWeakPersistent(this))); } TreeScope& scope = insertion_point.GetTreeScope(); diff --git a/blink/renderer/core/dom/element.h b/blink/renderer/core/dom/element.h index 138ec976312..7e9b8cca6f3 100644 --- a/blink/renderer/core/dom/element.h +++ b/blink/renderer/core/dom/element.h @@ -186,6 +186,11 @@ enum class HidePopupForcingLevel { kHideImmediately, }; +enum class HidePopupIndependence { + kLeaveUnrelated, + kHideUnrelated, +}; + typedef HeapVector> AttrNodeList; typedef HashMap AttrNameToTrustedType; @@ -574,7 +579,11 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable { void HidePopUpInternal(HidePopupFocusBehavior focus_behavior, HidePopupForcingLevel forcing_level); void PopupHideFinishIfNeeded(); - static const Element* NearestOpenAncestralPopup(Node* start_node); + static const Element* NearestOpenAncestralPopup(const Node* node, + bool inclusive = false); + // Retrieves the element pointed to by this element's 'anchor' content + // attribute, if that element exists, and if this element is a pop-up. + Element* anchorElement() const; static void HandlePopupLightDismiss(const Event& event); void InvokePopup(Element* invoker); void SetPopupFocusOnShow(); @@ -583,7 +592,8 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable { static void HideAllPopupsUntil(const Element*, Document&, HidePopupFocusBehavior, - HidePopupForcingLevel); + HidePopupForcingLevel, + HidePopupIndependence); // TODO(crbug.com/1197720): The popup position should be provided by the new // anchored positioning scheme. @@ -1351,10 +1361,6 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable { kAttachLayoutTree, }; - // Retrieves the element pointed to by this element's 'anchor' content - // attribute, if that element exists. - Element* anchorElement() const; - // Special focus handling for popups. Element* GetPopupFocusableArea(bool autofocus_only) const; diff --git a/blink/renderer/core/dom/node.cc b/blink/renderer/core/dom/node.cc index a72b46ba217..5ab77509a56 100644 --- a/blink/renderer/core/dom/node.cc +++ b/blink/renderer/core/dom/node.cc @@ -2894,7 +2894,7 @@ void Node::NotifyMutationObserversNodeWillDetach() { } void Node::HandleLocalEvents(Event& event) { - if (UNLIKELY(IsDocumentNode() && GetDocument().PopupOrHintShowing())) { + if (UNLIKELY(IsDocumentNode() && GetDocument().TopmostPopupAutoOrHint())) { // Check if this event should "light dismiss" one or more popups. Element::HandlePopupLightDismiss(event); } diff --git a/blink/renderer/core/fullscreen/fullscreen.cc b/blink/renderer/core/fullscreen/fullscreen.cc index 9c54c525d02..3333cb29a1c 100644 --- a/blink/renderer/core/fullscreen/fullscreen.cc +++ b/blink/renderer/core/fullscreen/fullscreen.cc @@ -200,12 +200,12 @@ void GoFullscreen(Element& element, else DCHECK(!HasFullscreenFlag(element)); - // If there are any open popups, close them immediately, unless this - // fullscreen element is a descendant of an open popup. + // If there are any open popups, close them immediately. if (RuntimeEnabledFeatures::HTMLPopupAttributeEnabled()) { - Element::HideAllPopupsUntil(&element, document, + Element::HideAllPopupsUntil(nullptr, document, HidePopupFocusBehavior::kNone, - HidePopupForcingLevel::kHideImmediately); + HidePopupForcingLevel::kHideImmediately, + HidePopupIndependence::kHideUnrelated); } // To fullscreen an |element| within a |document|, set the |element|'s diff --git a/blink/renderer/core/html/html_dialog_element.cc b/blink/renderer/core/html/html_dialog_element.cc index c8bfd1b77bb..2e6c90972c8 100644 --- a/blink/renderer/core/html/html_dialog_element.cc +++ b/blink/renderer/core/html/html_dialog_element.cc @@ -189,7 +189,8 @@ void HTMLDialogElement::show() { if (RuntimeEnabledFeatures::HTMLPopupAttributeEnabled()) { Element::HideAllPopupsUntil(nullptr, GetDocument(), HidePopupFocusBehavior::kNone, - HidePopupForcingLevel::kHideImmediately); + HidePopupForcingLevel::kHideImmediately, + HidePopupIndependence::kHideUnrelated); } // The layout must be updated here because setFocusForDialog calls @@ -249,7 +250,8 @@ void HTMLDialogElement::showModal(ExceptionState& exception_state) { if (RuntimeEnabledFeatures::HTMLPopupAttributeEnabled()) { Element::HideAllPopupsUntil(nullptr, document, HidePopupFocusBehavior::kNone, - HidePopupForcingLevel::kHideImmediately); + HidePopupForcingLevel::kHideImmediately, + HidePopupIndependence::kHideUnrelated); } document.AddToTopLayer(this); diff --git a/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html b/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html index 145651173cf..d6090486aa2 100644 --- a/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html +++ b/blink/web_tests/external/wpt/html/semantics/popups/popup-attribute-basic.tentative.html @@ -20,6 +20,17 @@
Not a pop-up
+
Animated pop-up
+ + diff --git a/blink/web_tests/external/wpt/html/semantics/popups/popup-defaultopen-hints.tentative.html b/blink/web_tests/external/wpt/html/semantics/popups/popup-defaultopen-hints.tentative.html new file mode 100644 index 00000000000..9bc841eb772 --- /dev/null +++ b/blink/web_tests/external/wpt/html/semantics/popups/popup-defaultopen-hints.tentative.html @@ -0,0 +1,20 @@ + + + + + + + + +
This is a popup=hint with defaultopen, which should NOT be open upon load
+ + diff --git a/blink/web_tests/external/wpt/html/semantics/popups/popup-defaultopen.tentative.html b/blink/web_tests/external/wpt/html/semantics/popups/popup-defaultopen.tentative.html index 7a822146234..f673d1d8320 100644 --- a/blink/web_tests/external/wpt/html/semantics/popups/popup-defaultopen.tentative.html +++ b/blink/web_tests/external/wpt/html/semantics/popups/popup-defaultopen.tentative.html @@ -8,7 +8,6 @@
This is a popup, which should be open upon load
This is a second popup with defaultopen, which should NOT be open upon load
-
This is a hint popup with defaultopen, which should NOT be open upon load
Also not visible
This is a manual popup with defaultopen, which should be open upon load
@@ -25,7 +24,6 @@ assert_true(p2.hasAttribute('defaultopen'),'defaultopen should be present/true, even if not opened'); assert_true(p2.defaultOpen,'defaultopen should be present/true, even if not opened'); - assert_false(p2b.matches(':top-layer'),'Only the first popup/hint with defaultopen should be open on load'); assert_true(p4.matches(':top-layer'),'defaultopen should open all manual popups'); assert_true(p5.matches(':top-layer'),'defaultopen should open all manual popups'); diff --git a/blink/web_tests/external/wpt/html/semantics/popups/popup-focus.tentative.html b/blink/web_tests/external/wpt/html/semantics/popups/popup-focus.tentative.html index f4fea330b65..0ab90bbf554 100644 --- a/blink/web_tests/external/wpt/html/semantics/popups/popup-focus.tentative.html +++ b/blink/web_tests/external/wpt/html/semantics/popups/popup-focus.tentative.html @@ -132,10 +132,9 @@ t.add_cleanup(() => priorFocus.remove()); return priorFocus; } - async function finishAnimations(popUp) { - popUp.getAnimations({subtree: true}).forEach(animation => animation.finish()); - await waitForRender(); - assert_false(isElementVisible(popUp)); + async function finishAnimationsAndVerifyHide(popUp) { + await finishAnimations(popUp); + assert_false(isElementVisible(popUp),'After animations are finished, the pop-up should be hidden'); } function activateAndVerify(popUp) { const testName = popUp.getAttribute('data-test'); @@ -155,7 +154,7 @@ assert_equals(document.activeElement, expectedFocusedElement, `${testName} activated by popUp.showPopUp()`); popUp.hidePopUp(); assert_equals(document.activeElement, priorFocus, 'prior element should get focus on hide'); - await finishAnimations(popUp); + await finishAnimationsAndVerifyHide(popUp); // Hit Escape: priorFocus.focus(); @@ -164,7 +163,7 @@ assert_equals(document.activeElement, expectedFocusedElement, `${testName} activated by popUp.showPopUp()`); await sendEscape(); assert_equals(document.activeElement, priorFocus, 'prior element should get focus after Escape'); - await finishAnimations(popUp); + await finishAnimationsAndVerifyHide(popUp); // Change the pop-up type: priorFocus.focus(); @@ -174,7 +173,7 @@ popUp.popUp = 'hint'; assert_false(popUp.matches(':top-layer'), 'Changing the pop-up type should hide the pop-up'); assert_equals(document.activeElement, priorFocus, 'prior element should get focus when the type is changed'); - await finishAnimations(popUp); + await finishAnimationsAndVerifyHide(popUp); popUp.popUp = 'auto'; // Remove from the document: @@ -212,7 +211,7 @@ button.click(); // Button is set to toggle the pop-up assert_false(popUp.matches(':top-layer')); assert_equals(document.activeElement, priorFocus, 'prior element should get focus on button-toggled hide'); - await finishAnimations(popUp); + await finishAnimationsAndVerifyHide(popUp); }, "Popup focus test: " + testName); promise_test(async t => { @@ -229,7 +228,7 @@ await clickOn(button); // This will not light dismiss, but will "toggle" the popUp. assert_false(popUp.matches(':top-layer')); assert_equals(document.activeElement, priorFocus, 'Focus should return to prior focus'); - await finishAnimations(popUp); + await finishAnimationsAndVerifyHide(popUp); // Same thing, but the button is contained within the pop-up button.removeAttribute('togglepopup'); @@ -244,7 +243,7 @@ await clickOn(button); assert_false(popUp.matches(':top-layer'), 'clicking button should hide the pop-up'); assert_equals(document.activeElement, priorFocus, 'Contained button should return focus to the previously focused element'); - await finishAnimations(popUp); + await finishAnimationsAndVerifyHide(popUp); // Same thing, but the button is unrelated (no togglepopup) button = document.createElement('button'); @@ -255,7 +254,7 @@ await clickOn(button); // This will light dismiss the pop-up, focus the prior focus, then focus this button. assert_false(popUp.matches(':top-layer'), 'clicking button should hide the pop-up (via light dismiss)'); assert_equals(document.activeElement, button, 'Focus should go to unrelated button on light dismiss'); - await finishAnimations(popUp); + await finishAnimationsAndVerifyHide(popUp); }, "Popup button click focus test: " + testName); promise_test(async t => { @@ -276,7 +275,7 @@ assert_equals(document.activeElement, newFocus, 'focus should not change when prior focus is removed'); popUp.hidePopUp(); assert_not_equals(document.activeElement, priorFocus, 'focused element has been removed'); - await finishAnimations(popUp); + await finishAnimationsAndVerifyHide(popUp); document.body.appendChild(priorFocus); // Put it back // Move the prior focus inside the (already open) pop-up @@ -292,7 +291,7 @@ assert_true(isElementVisible(popUp),'Animations should keep the pop-up visible'); assert_not_equals(getComputedStyle(popUp).display,'none','Animations should keep the pop-up visible'); assert_equals(document.activeElement, priorFocus, 'focused element gets focused'); - await finishAnimations(popUp); + await finishAnimationsAndVerifyHide(popUp); assert_equals(getComputedStyle(popUp).display,'none','Animations have ended, pop-up should be hidden'); assert_not_equals(document.activeElement, priorFocus, 'focused element is display:none inside the pop-up'); document.body.appendChild(priorFocus); // Put it back diff --git a/blink/web_tests/external/wpt/html/semantics/popups/popup-light-dismiss.tentative.html b/blink/web_tests/external/wpt/html/semantics/popups/popup-light-dismiss.tentative.html index fc900439f83..99a48d7230e 100644 --- a/blink/web_tests/external/wpt/html/semantics/popups/popup-light-dismiss.tentative.html +++ b/blink/web_tests/external/wpt/html/semantics/popups/popup-light-dismiss.tentative.html @@ -425,37 +425,37 @@ diff --git a/blink/web_tests/external/wpt/html/semantics/popups/popup-types.tentative.html b/blink/web_tests/external/wpt/html/semantics/popups/popup-types.tentative.html index 2b82fcd7542..b34df5f7960 100644 --- a/blink/web_tests/external/wpt/html/semantics/popups/popup-types.tentative.html +++ b/blink/web_tests/external/wpt/html/semantics/popups/popup-types.tentative.html @@ -5,84 +5,180 @@ - -
Hint
-
Async
-
Async
+
+
Popup
+
Hint
+
Async
+
Async
+ +
+ +
+
Popup 1 +
Popup 2 +

Anchor

+
Popup 3
+
+
+
Hint anchored to pop-up
+ +
-test(() => { - assert_state_1(false,false,false,false); - hint.showPopUp(); - manual.showPopUp(); - manual2.showPopUp(); - assert_state_1(false,true,true,true); - popup.showPopUp(); - assert_state_1(true,false,true,true); - popup.hidePopUp(); - assert_state_1(false,false,true,true); - manual.hidePopUp(); - manual2.hidePopUp(); - assert_state_1(false,false,false,false); -},'popups close hints but not manuals'); - +
+
Hint +
Nested hint
+
+ +
-
Popup 1 -
Popup 2 -

Anchor

-
Popup 3
+
+
Hint +
Nested auto (note - never visible, since inside display:none subtree)
+ +
+ +
+
Auto +
Nested Auto
+
Nested hint
+
+ +
+ +
+
Auto
+
Non-Nested hint
+
-
Hint anchored to popup
- diff --git a/blink/web_tests/external/wpt/html/semantics/popups/resources/popup-utils.js b/blink/web_tests/external/wpt/html/semantics/popups/resources/popup-utils.js index bbb628b40d2..886a2302cad 100644 --- a/blink/web_tests/external/wpt/html/semantics/popups/resources/popup-utils.js +++ b/blink/web_tests/external/wpt/html/semantics/popups/resources/popup-utils.js @@ -23,3 +23,7 @@ async function sendEscape() { function isElementVisible(el) { return !!(el.offsetWidth || el.offsetHeight || el.getClientRects().length); } +async function finishAnimations(popUp) { + popUp.getAnimations({subtree: true}).forEach(animation => animation.finish()); + await waitForRender(); +}