Bug 1833570 - Change popover invoker type from boolean to Element. r=…

…emilio The invoker type is currently implemented as boolean as suggested at whatwg/html#9168. This issue is now closed and has been fixed at whatwg/html#9171. This patch is to be follow above discussions and implement popover invoker type as element. Differential Revision: https://phabricator.services.mozilla.com/D178287
erichdongubler-mozilla · May 31, 2023 · 2f265d5 · 2f265d5
1 parent 5242b77
commit 2f265d5
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 28 deletions.
diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp
@@ -15074,7 +15074,9 @@ void Document::HidePopover(Element& aPopover, bool aFocusPreviousElement,
     }
   }
 
-  aPopover.SetHasPopoverInvoker(false);
+  auto* data = popoverHTMLEl->GetPopoverData();
+  MOZ_ASSERT(data, "Should have popover data");
+  data->SetInvoker(nullptr);
 
   // Fire beforetoggle event and re-check popover validity.
   if (aFireEvents && !wasHiding) {

diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
@@ -4248,19 +4248,6 @@ void Element::ClearServoData(Document* aDoc) {
   }
 }
 
-bool Element::HasPopoverInvoker() const {
-  auto* popoverData = GetPopoverData();
-  return popoverData && popoverData->HasPopoverInvoker();
-}
-
-void Element::SetHasPopoverInvoker(bool aHasInvoker) {
-  if (aHasInvoker) {
-    EnsurePopoverData().SetHasPopoverInvoker(true);
-  } else if (auto* popoverData = GetPopoverData()) {
-    popoverData->SetHasPopoverInvoker(false);
-  }
-}
-
 bool Element::IsAutoPopover() const {
   const auto* htmlElement = nsGenericHTMLElement::FromNode(this);
   return htmlElement &&
@@ -4305,8 +4292,9 @@ Element* Element::GetTopmostPopoverAncestor() const {
 
   checkAncestor(newPopover->GetFlattenedTreeParentElement());
 
-  // TODO: To handle the button invokers
   // https://github.com/whatwg/html/issues/9160
+  RefPtr<Element> invoker = newPopover->GetPopoverData()->GetInvoker();
+  checkAncestor(invoker);
 
   return topmostPopoverAncestor;
 }

diff --git a/dom/base/Element.h b/dom/base/Element.h
@@ -581,10 +581,6 @@ class Element : public FragmentOrElement {
     return CreatePopoverData();
   }
 
-  // https://html.spec.whatwg.org/multipage/popover.html#popover-invoker
-  bool HasPopoverInvoker() const;
-  void SetHasPopoverInvoker(bool);
-
   bool IsAutoPopover() const;
   bool IsPopoverOpen() const;
 

diff --git a/dom/base/PopoverData.h b/dom/base/PopoverData.h
@@ -7,10 +7,12 @@
 #ifndef mozilla_dom_PopoverData_h
 #define mozilla_dom_PopoverData_h
 
-#include "nsStringFwd.h"
+#include "Element.h"
+#include "nsINode.h"
 #include "nsIRunnable.h"
-#include "nsThreadUtils.h"
 #include "nsIWeakReferenceUtils.h"
+#include "nsStringFwd.h"
+#include "nsThreadUtils.h"
 
 namespace mozilla::dom {
 
@@ -66,10 +68,14 @@ class PopoverData {
     mPreviouslyFocusedElement = aPreviouslyFocusedElement;
   }
 
-  bool HasPopoverInvoker() const { return mHasPopoverInvoker; }
-  void SetHasPopoverInvoker(bool aHasPopoverInvoker) {
-    mHasPopoverInvoker = aHasPopoverInvoker;
+  RefPtr<Element> GetInvoker() const {
+    return do_QueryReferent(mInvokerElement);
+  }
+  void SetInvoker(Element* aInvokerElement) {
+    mInvokerElement =
+        do_GetWeakReference(static_cast<nsINode*>(aInvokerElement));
   }
+
   PopoverToggleEventTask* GetToggleEventTask() const { return mTask; }
   void SetToggleEventTask(PopoverToggleEventTask* aTask) { mTask = aTask; }
   void ClearToggleEventTask() { mTask = nullptr; }
@@ -85,9 +91,12 @@ class PopoverData {
   // See, https://github.com/whatwg/html/issues/9063
   nsWeakPtr mPreviouslyFocusedElement = nullptr;
 
-  // https://html.spec.whatwg.org/multipage/popover.html#popover-invoker, also
-  // see https://github.com/whatwg/html/issues/9168.
-  bool mHasPopoverInvoker = false;
+  // https://html.spec.whatwg.org/#popover-invoker
+  // Since having a popover invoker only makes a difference if the invoker
+  // is in the document (in another open popover to be precise) we can make
+  // this a weak reference, as if the element goes away it's necessarily not
+  // connected to our document.
+  nsWeakPtr mInvokerElement;
   bool mIsHiding = false;
   RefPtr<PopoverToggleEventTask> mTask;
 };

diff --git a/dom/html/nsGenericHTMLElement.cpp b/dom/html/nsGenericHTMLElement.cpp
@@ -2892,7 +2892,7 @@ void nsGenericHTMLFormControlElementWithState::HandlePopoverTargetAction() {
   if (canHide && target->IsPopoverOpen()) {
     target->HidePopover(IgnoreErrors());
   } else if (canShow && !target->IsPopoverOpen()) {
-    target->SetHasPopoverInvoker(true);
+    target->GetPopoverData()->SetInvoker(this);
     target->ShowPopover(IgnoreErrors());
   }
 }