Reland "Add the beginning of hover-trigger for pop-ups"

This is a reland of commit cdbdb4520d6be65fdb04fc85aea5d12953f1a1da The added test was flaky. I'm already revamping that test in [1], so I'm just going to delete it here and make sure it isn't flaky in [1]. [1] https://chromium-review.googlesource.com/c/chromium/src/+/3760885 Fixed: 1344436 Original change's description: > Add the beginning of hover-trigger for pop-ups > > This CL adds the 'hoverpopup' attribute, to go along with the other > invoking attributes (togglepopup, showpopup, hidepopup). This attribute, > when places on *any* element, triggers a pop-up when hovered. With this > CL, it is always triggered after a fixed 100ms delay, but that will be > made configurable via CSS in subsequent CLs. There is also no "hide" > trigger when any element is de-hovered - again that will be added > later. > > See this spec discussion: > openui/open-ui#526 (comment) > > Bug: 1307772 > Change-Id: I7af88dad9efa015f833843ea76bed41b4aa42c4b > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3759184 > Reviewed-by: David Baron <[email protected]> > Auto-Submit: Mason Freed <[email protected]> > Commit-Queue: Mason Freed <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1024141} Bug: 1307772 Change-Id: I571272780ddbca9905a429172dec7717dcd5ac9f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3763422 Commit-Queue: David Baron <[email protected]> Reviewed-by: David Baron <[email protected]> Auto-Submit: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1024296} NOKEYCHECK=True GitOrigin-RevId: f6f2c4d5964dd9dcc11e20e6298510ff7395ba5b
mjfroman · Jul 14, 2022 · d1b7025 · d1b7025
1 parent ac0f591
commit d1b7025
Show file tree

Hide file tree

Showing 6 changed files with 78 additions and 26 deletions.
diff --git a/blink/renderer/core/dom/element.cc b/blink/renderer/core/dom/element.cc
@@ -3061,6 +3061,60 @@ Element* Element::anchorElement() const {
   return GetTreeScope().getElementById(anchor_id);  // may be null
 }
 
+void Element::MaybeTriggerHoverPopup(Element* popup_element) {
+  DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled());
+  if (!popup_element || !popup_element->HasValidPopupAttribute())
+    return;
+  // Remove this element from hoverPopupTasks always.
+  popup_element->GetPopupData()->hoverPopupTasks().erase(this);
+  // Only trigger the pop-up if the hoverpopup attribute still points to the
+  // same pop-up, and the pop-up is in the tree and still not showing.
+  if (popup_element->IsInTreeScope() && !popup_element->popupOpen() &&
+      popup_element == GetTreeScope().getElementById(
+                           FastGetAttribute(html_names::kHoverpopupAttr))) {
+    popup_element->showPopUp(ASSERT_NO_EXCEPTION);
+  }
+}
+
+void Element::HandlePopupHovered(bool hovered) {
+  if (!RuntimeEnabledFeatures::HTMLPopupAttributeEnabled())
+    return;
+  if (!FastHasAttribute(html_names::kHoverpopupAttr) || !IsInTreeScope())
+    return;
+  Element* popup_element = GetTreeScope().getElementById(
+      FastGetAttribute(html_names::kHoverpopupAttr));
+  if (!popup_element || !popup_element->HasValidPopupAttribute())
+    return;
+  if (hovered) {
+    auto& hover_tasks = popup_element->GetPopupData()->hoverPopupTasks();
+    DCHECK(!hover_tasks.Contains(this));
+
+    // TODO(masonf): Use CSS timeout value instead of this hard-coded timeout.
+    constexpr base::TimeDelta kDelayBeforeShow = base::Milliseconds(100);
+
+    // When we enter an element, we'll post a delayed task for the pop-up we're
+    // targeting. It's possible that multiple nested elements have hoverpopup
+    // attributes pointing to the same pop-up, and in that case, we want to
+    // trigger on the first of them that reaches its timeout threshold.
+    hover_tasks.insert(
+        this,
+        PostDelayedCancellableTask(
+            *GetExecutionContext()->GetTaskRunner(TaskType::kInternalDefault),
+            FROM_HERE,
+            WTF::Bind(&Element::MaybeTriggerHoverPopup,
+                      WrapWeakPersistent(this),
+                      WrapWeakPersistent(popup_element)),
+            kDelayBeforeShow));
+  } else {
+    // If we have a task still waiting, cancel it.
+    popup_element->GetPopupData()->hoverPopupTasks().Take(this).Cancel();
+    // TODO(masonf): Still need to implement the code to hide this pop-up after
+    // a configurable delay. That needs to work even if the pop-up wasn't
+    // triggered by a hoverpopup attribute. E.g. a regular pop-up that gets
+    // hidden after it has not been hovered for n seconds.
+  }
+}
+
 void Element::SetNeedsRepositioningForSelectMenu(bool flag) {
   DCHECK(RuntimeEnabledFeatures::HTMLSelectMenuElementEnabled());
   DCHECK(RuntimeEnabledFeatures::HTMLPopupAttributeEnabled());
@@ -8244,6 +8298,7 @@ void Element::SetHovered(bool hovered) {
     return;
 
   GetDocument().UserActionElements().SetHovered(this, hovered);
+  HandlePopupHovered(hovered);
 
   const ComputedStyle* style = GetComputedStyle();
   if (!style || style->AffectedByHover()) {

diff --git a/blink/renderer/core/dom/element.h b/blink/renderer/core/dom/element.h
@@ -594,6 +594,8 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
                                  HidePopupFocusBehavior,
                                  HidePopupForcingLevel,
                                  HidePopupIndependence);
+  void MaybeTriggerHoverPopup(Element* popup_element);
+  void HandlePopupHovered(bool hovered);
 
   // TODO(crbug.com/1197720): The popup position should be provided by the new
   // anchored positioning scheme.

diff --git a/blink/renderer/core/dom/popup_data.h b/blink/renderer/core/dom/popup_data.h
@@ -81,6 +81,10 @@ class PopupData final : public GarbageCollected<PopupData> {
     animation_finished_listener_ = listener;
   }
 
+  HeapHashMap<WeakMember<Element>, TaskHandle>& hoverPopupTasks() {
+    return hover_popup_tasks_;
+  }
+
   HTMLSelectMenuElement* ownerSelectMenuElement() const {
     return owner_select_menu_element_;
   }
@@ -92,6 +96,7 @@ class PopupData final : public GarbageCollected<PopupData> {
     visitor->Trace(invoker_);
     visitor->Trace(previously_focused_element_);
     visitor->Trace(animation_finished_listener_);
+    visitor->Trace(hover_popup_tasks_);
     visitor->Trace(owner_select_menu_element_);
   }
 
@@ -105,6 +110,9 @@ class PopupData final : public GarbageCollected<PopupData> {
   // We hold a strong reference to the animation finished listener, so that we
   // can confirm that the listeners get removed before cleanup.
   Member<PopupAnimationFinishedEventListener> animation_finished_listener_;
+  // Map from triggering elements to a TaskHandle for the task that will invoke
+  // the pop-up.
+  HeapHashMap<WeakMember<Element>, TaskHandle> hover_popup_tasks_;
 
   // TODO(crbug.com/1197720): The popup position should be provided by the new
   // anchored positioning scheme.

diff --git a/blink/renderer/core/html/html_attribute_names.json5 b/blink/renderer/core/html/html_attribute_names.json5
@@ -102,6 +102,7 @@
     "hidden",
     "hidepopup",
     "high",
+    "hoverpopup",
     "href",
     "hreflang",
     "hreftranslate",

diff --git a/blink/renderer/core/html/html_element.cc b/blink/renderer/core/html/html_element.cc
@@ -54,6 +54,7 @@
 #include "third_party/blink/renderer/core/editing/editing_utilities.h"
 #include "third_party/blink/renderer/core/editing/serializers/serialization.h"
 #include "third_party/blink/renderer/core/editing/spellcheck/spell_checker.h"
+#include "third_party/blink/renderer/core/event_type_names.h"
 #include "third_party/blink/renderer/core/events/keyboard_event.h"
 #include "third_party/blink/renderer/core/frame/csp/content_security_policy.h"
 #include "third_party/blink/renderer/core/frame/local_dom_window.h"
@@ -442,20 +443,23 @@ AttributeTriggers* HTMLElement::TriggersForAttributeName(
   static AttributeTriggers attribute_triggers[] = {
       {html_names::kDirAttr, kNoWebFeature, kNoEvent,
        &HTMLElement::OnDirAttrChanged},
-      {html_names::kFocusgroupAttr, kNoWebFeature, kNoEvent,
-       &HTMLElement::OnFocusgroupAttrChanged},
       {html_names::kFormAttr, kNoWebFeature, kNoEvent,
        &HTMLElement::OnFormAttrChanged},
       {html_names::kLangAttr, kNoWebFeature, kNoEvent,
        &HTMLElement::OnLangAttrChanged},
       {html_names::kNonceAttr, kNoWebFeature, kNoEvent,
        &HTMLElement::OnNonceAttrChanged},
+
+      {html_names::kFocusgroupAttr, kNoWebFeature, kNoEvent,
+       &HTMLElement::ReparseAttribute},
       {html_names::kTabindexAttr, kNoWebFeature, kNoEvent,
-       &HTMLElement::OnTabIndexAttrChanged},
+       &HTMLElement::ReparseAttribute},
       {xml_names::kLangAttr, kNoWebFeature, kNoEvent,
-       &HTMLElement::OnXMLLangAttrChanged},
+       &HTMLElement::ReparseAttribute},
       {html_names::kPopupAttr, kNoWebFeature, kNoEvent,
-       &HTMLElement::OnPopupAttrChanged},
+       &HTMLElement::ReparseAttribute},
+      {html_names::kHoverpopupAttr, kNoWebFeature, kNoEvent,
+       &HTMLElement::ReparseAttribute},
 
       {html_names::kOnabortAttr, kNoWebFeature, event_type_names::kAbort,
        nullptr},
@@ -2021,8 +2025,7 @@ void HTMLElement::OnDirAttrChanged(const AttributeModificationParams& params) {
   }
 }
 
-void HTMLElement::OnFocusgroupAttrChanged(
-    const AttributeModificationParams& params) {
+void HTMLElement::ReparseAttribute(const AttributeModificationParams& params) {
   Element::ParseAttribute(params);
 }
 
@@ -2041,21 +2044,6 @@ void HTMLElement::OnNonceAttrChanged(
     setNonce(params.new_value);
 }
 
-void HTMLElement::OnTabIndexAttrChanged(
-    const AttributeModificationParams& params) {
-  Element::ParseAttribute(params);
-}
-
-void HTMLElement::OnXMLLangAttrChanged(
-    const AttributeModificationParams& params) {
-  Element::ParseAttribute(params);
-}
-
-void HTMLElement::OnPopupAttrChanged(
-    const AttributeModificationParams& params) {
-  Element::ParseAttribute(params);
-}
-
 ElementInternals* HTMLElement::attachInternals(
     ExceptionState& exception_state) {
   // 1. If this's is value is not null, then throw a "NotSupportedError"

diff --git a/blink/renderer/core/html/html_element.h b/blink/renderer/core/html/html_element.h
@@ -264,13 +264,11 @@ class CORE_EXPORT HTMLElement : public Element {
       const QualifiedName& attr_name);
 
   void OnDirAttrChanged(const AttributeModificationParams&);
-  void OnFocusgroupAttrChanged(const AttributeModificationParams&);
   void OnFormAttrChanged(const AttributeModificationParams&);
   void OnLangAttrChanged(const AttributeModificationParams&);
   void OnNonceAttrChanged(const AttributeModificationParams&);
-  void OnTabIndexAttrChanged(const AttributeModificationParams&);
-  void OnXMLLangAttrChanged(const AttributeModificationParams&);
-  void OnPopupAttrChanged(const AttributeModificationParams&);
+
+  void ReparseAttribute(const AttributeModificationParams&);
 };
 
 template <typename T>