Bug 1868552 - Refactor nsIContent::IsFocusable for clarity. r=masayuki

Make it be output-only, not having that confusing in-out tab-index parameter that is special for XUL to become focusable with -moz-user-focus: normal. Instead, do that explicitly in nsIFrame::IsFocusable(). Also, call it IsFocusableWithoutStyle(), since that's what it is. Differential Revision: https://phabricator.services.mozilla.com/D195644
dothq · Dec 8, 2023 · 633cce1 · 633cce1
1 parent cc33896
commit 633cce1
Show file tree

Hide file tree

Showing 21 changed files with 141 additions and 247 deletions.
diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp
@@ -6151,8 +6151,9 @@ nsresult Document::EditingStateChanged() {
           getter_AddRefs(focusedWindow));
       if (focusedContent) {
         nsIFrame* focusedFrame = focusedContent->GetPrimaryFrame();
-        bool clearFocus = focusedFrame ? !focusedFrame->IsFocusable()
-                                       : !focusedContent->IsFocusable();
+        bool clearFocus = focusedFrame
+                              ? !focusedFrame->IsFocusable()
+                              : !focusedContent->IsFocusableWithoutStyle();
         if (clearFocus) {
           if (RefPtr<nsFocusManager> fm = nsFocusManager::GetFocusManager()) {
             fm->ClearFocus(window);

diff --git a/dom/base/FragmentOrElement.cpp b/dom/base/FragmentOrElement.cpp
@@ -1032,16 +1032,6 @@ void nsIContent::GetEventTargetParent(EventChainPreVisitor& aVisitor) {
   }
 }
 
-bool nsIContent::IsFocusable(int32_t* aTabIndex, bool aWithMouse) {
-  bool focusable = IsFocusableInternal(aTabIndex, aWithMouse);
-  // Ensure that the return value and aTabIndex are consistent in the case
-  // we're in userfocusignored context.
-  if (focusable || (aTabIndex && *aTabIndex != -1)) {
-    return focusable;
-  }
-  return false;
-}
-
 Element* nsIContent::GetAutofocusDelegate(bool aWithMouse) const {
   for (nsINode* node = GetFirstChild(); node; node = node->GetNextNode(this)) {
     auto* descendant = Element::FromNode(*node);
@@ -1068,7 +1058,7 @@ Element* nsIContent::GetFocusDelegate(bool aWithMouse) const {
     whereToLook = root;
   }
 
-  auto IsFocusable = [&](Element* aElement) -> nsIFrame::Focusable {
+  auto IsFocusable = [&](Element* aElement) -> Focusable {
     nsIFrame* frame = aElement->GetPrimaryFrame();
 
     if (!frame) {
@@ -1094,7 +1084,7 @@ Element* nsIContent::GetFocusDelegate(bool aWithMouse) const {
         return el;
       }
     } else if (!potentialFocus) {
-      if (nsIFrame::Focusable focusable = IsFocusable(el)) {
+      if (Focusable focusable = IsFocusable(el)) {
         if (IsHTMLElement(nsGkAtoms::dialog)) {
           if (focusable.mTabIndex >= 0) {
             // If focusTarget is a dialog element and descendant is sequentially
@@ -1134,11 +1124,9 @@ Element* nsIContent::GetFocusDelegate(bool aWithMouse) const {
   return potentialFocus;
 }
 
-bool nsIContent::IsFocusableInternal(int32_t* aTabIndex, bool aWithMouse) {
-  if (aTabIndex) {
-    *aTabIndex = -1;  // Default, not tabbable
-  }
-  return false;
+Focusable nsIContent::IsFocusableWithoutStyle(bool aWithMouse) {
+  // Default, not tabbable
+  return {};
 }
 
 void nsIContent::SetAssignedSlot(HTMLSlotElement* aSlot) {

diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp
@@ -3377,13 +3377,9 @@ nsresult nsFocusManager::DetermineElementToMoveFocus(
   int32_t tabIndex = forward ? 1 : 0;
   if (startContent) {
     nsIFrame* frame = startContent->GetPrimaryFrame();
-    if (startContent->IsHTMLElement(nsGkAtoms::area)) {
-      startContent->IsFocusable(&tabIndex);
-    } else if (frame) {
-      tabIndex = frame->IsFocusable().mTabIndex;
-    } else {
-      startContent->IsFocusable(&tabIndex);
-    }
+    tabIndex = (frame && !startContent->IsHTMLElement(nsGkAtoms::area))
+                   ? frame->IsFocusable().mTabIndex
+                   : startContent->IsFocusableWithoutStyle().mTabIndex;
 
     // if the current element isn't tabbable, ignore the tabindex and just
     // look for the next element. The root content won't have a tabindex
@@ -4038,7 +4034,7 @@ nsIContent* nsFocusManager::GetNextTabbableContentInAncestorScopes(
     } else if (nsIFrame* frame = startContent->GetPrimaryFrame()) {
       tabIndex = frame->IsFocusable().mTabIndex;
     } else {
-      startContent->IsFocusable(&tabIndex);
+      tabIndex = startContent->IsFocusableWithoutStyle().mTabIndex;
     }
     nsIContent* contentToFocus = GetNextTabbableContentInScope(
         owner, startContent, aOriginalStartContent, aForward, tabIndex,
@@ -4214,7 +4210,7 @@ nsresult nsFocusManager::GetNextTabbableContent(
       if (iterStartContent == aRootContent) {
         if (!aForward) {
           frameTraversal->Last();
-        } else if (aRootContent->IsFocusable()) {
+        } else if (aRootContent->IsFocusableWithoutStyle()) {
           frameTraversal->Next();
         }
         frame = frameTraversal->CurrentItem();
@@ -4282,11 +4278,12 @@ nsresult nsFocusManager::GetNextTabbableContent(
                 return rv;
               }
             }
-          } else {
-            if (invokerContent && invokerContent->IsFocusable()) {
-              invokerContent.forget(aResultContent);
-              return NS_OK;
-            }
+          } else if (invokerContent &&
+                     invokerContent->IsFocusableWithoutStyle()) {
+            // FIXME(emilio): The check above should probably use
+            // nsIFrame::IsFocusable, not IsFocusableWithoutStyle.
+            invokerContent.forget(aResultContent);
+            return NS_OK;
           }
         }
       }
@@ -4619,11 +4616,11 @@ nsIContent* nsFocusManager::GetNextTabbableMapArea(bool aForward,
     // First see if the the start content is in this map
     Maybe<uint32_t> indexOfStartContent =
         mapContent->ComputeIndexOf(aStartContent);
-    int32_t tabIndex;
     nsIContent* scanStartContent;
+    Focusable focusable;
     if (indexOfStartContent.isNothing() ||
-        (aStartContent->IsFocusable(&tabIndex) &&
-         tabIndex != aCurrentTabIndex)) {
+        ((focusable = aStartContent->IsFocusableWithoutStyle()) &&
+         focusable.mTabIndex != aCurrentTabIndex)) {
       // If aStartContent is in this map we must start iterating past it.
       // We skip the case where aStartContent has tabindex == aStartContent
       // since the next tab ordered element might be before it
@@ -4638,7 +4635,8 @@ nsIContent* nsFocusManager::GetNextTabbableMapArea(bool aForward,
     for (nsCOMPtr<nsIContent> areaContent = scanStartContent; areaContent;
          areaContent = aForward ? areaContent->GetNextSibling()
                                 : areaContent->GetPreviousSibling()) {
-      if (areaContent->IsFocusable(&tabIndex) && tabIndex == aCurrentTabIndex) {
+      focusable = areaContent->IsFocusableWithoutStyle();
+      if (focusable && focusable.mTabIndex == aCurrentTabIndex) {
         return areaContent;
       }
     }
@@ -5459,7 +5457,8 @@ Element* nsFocusManager::GetTheFocusableArea(Element* aTarget,
     // HTML areas do not have their own frame, and the img frame we get from
     // GetPrimaryFrame() is not relevant as to whether it is focusable or
     // not, so we have to do all the relevant checks manually for them.
-    return frame->IsVisibleConsideringAncestors() && aTarget->IsFocusable()
+    return frame->IsVisibleConsideringAncestors() &&
+                   aTarget->IsFocusableWithoutStyle()
                ? aTarget
                : nullptr;
   }

diff --git a/dom/base/nsIContent.h b/dom/base/nsIContent.h
@@ -30,6 +30,16 @@ struct IMEState;
 }  // namespace widget
 }  // namespace mozilla
 
+struct Focusable {
+  bool mFocusable = false;
+  // The computed tab index:
+  //         < 0 if not tabbable
+  //         == 0 if in normal tab order
+  //         > 0 can be tabbed to in the order specified by this value
+  int32_t mTabIndex = -1;
+  explicit operator bool() const { return mFocusable; }
+};
+
 // IID for the nsIContent interface
 // Must be kept in sync with xpcom/rust/xpcom/src/interfaces/nonidl.rs
 #define NS_ICONTENT_IID                              \
@@ -280,17 +290,9 @@ class nsIContent : public nsINode {
    * Also, depending on either the accessibility.tabfocus pref or
    * a system setting (nowadays: Full keyboard access, mac only)
    * some widgets may be focusable but removed from the tab order.
-   * @param  [inout, optional] aTabIndex the computed tab index
-   *         In: default tabindex for element (-1 nonfocusable, == 0 focusable)
-   *         Out: computed tabindex
-   * @param  [optional] aTabIndex the computed tab index
-   *         < 0 if not tabbable
-   *         == 0 if in normal tab order
-   *         > 0 can be tabbed to in the order specified by this value
    * @return whether the content is focusable via mouse, kbd or script.
    */
-  bool IsFocusable(int32_t* aTabIndex = nullptr, bool aWithMouse = false);
-  virtual bool IsFocusableInternal(int32_t* aTabIndex, bool aWithMouse);
+  virtual Focusable IsFocusableWithoutStyle(bool aWithMouse = false);
 
   // https://html.spec.whatwg.org/multipage/interaction.html#focus-delegate
   mozilla::dom::Element* GetFocusDelegate(bool aWithMouse) const;

diff --git a/dom/base/nsStyledElement.cpp b/dom/base/nsStyledElement.cpp
@@ -211,7 +211,7 @@ nsresult nsStyledElement::BindToTree(BindContext& aContext, nsINode& aParent) {
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (HasAttr(nsGkAtoms::autofocus) && aContext.AllowsAutoFocus() &&
-      (!IsSVGElement() || IsFocusable())) {
+      (!IsSVGElement() || IsFocusableWithoutStyle())) {
     aContext.OwnerDoc().ElementWithAutoFocusInserted(this);
   }
 

diff --git a/dom/events/IMEStateManager.cpp b/dom/events/IMEStateManager.cpp
@@ -1564,24 +1564,23 @@ MOZ_CAN_RUN_SCRIPT static bool IsNextFocusableElementTextControl(
   if (!nextElement) {
     return false;
   }
-  bool focusable = false;
-  nextElement->IsHTMLFocusable(false, &focusable, nullptr);
-  if (!focusable) {
+
+  // FIXME: Should probably use nsIFrame::IsFocusable if possible, to account
+  // for things like visibility: hidden or so.
+  if (!nextElement->IsFocusableWithoutStyle(false)) {
     return false;
   }
 
   // Check readonly attribute.
   if (nextElement->IsHTMLElement(nsGkAtoms::textarea)) {
-    HTMLTextAreaElement* textAreaElement =
-        HTMLTextAreaElement::FromNodeOrNull(nextElement);
+    auto* textAreaElement = HTMLTextAreaElement::FromNode(nextElement);
     return !textAreaElement->ReadOnly();
   }
 
   // If neither textarea nor input, what element type?
   MOZ_DIAGNOSTIC_ASSERT(nextElement->IsHTMLElement(nsGkAtoms::input));
 
-  HTMLInputElement* inputElement =
-      HTMLInputElement::FromNodeOrNull(nextElement);
+  auto* inputElement = HTMLInputElement::FromNode(nextElement);
   return !inputElement->ReadOnly();
 }
 

diff --git a/dom/html/HTMLAnchorElement.cpp b/dom/html/HTMLAnchorElement.cpp
@@ -98,19 +98,16 @@ bool HTMLAnchorElement::IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
 
   // cannot focus links if there is no link handler
   if (!OwnerDoc()->LinkHandlingEnabled()) {
+    *aTabIndex = -1;
     *aIsFocusable = false;
     return false;
   }
 
   // Links that are in an editable region should never be focusable, even if
   // they are in a contenteditable="false" region.
   if (nsContentUtils::IsNodeInEditableRegion(this)) {
-    if (aTabIndex) {
-      *aTabIndex = -1;
-    }
-
+    *aTabIndex = -1;
     *aIsFocusable = false;
-
     return true;
   }
 
@@ -119,22 +116,16 @@ bool HTMLAnchorElement::IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
     if (!IsLink()) {
       // Not tabbable or focusable without href (bug 17605), unless
       // forced to be via presence of nonnegative tabindex attribute
-      if (aTabIndex) {
-        *aTabIndex = -1;
-      }
-
+      *aTabIndex = -1;
       *aIsFocusable = false;
-
       return false;
     }
   }
 
-  if (aTabIndex && (sTabFocusModel & eTabFocus_linksMask) == 0) {
+  if ((sTabFocusModel & eTabFocus_linksMask) == 0) {
     *aTabIndex = -1;
   }
-
   *aIsFocusable = true;
-
   return false;
 }
 

diff --git a/dom/html/HTMLImageElement.cpp b/dom/html/HTMLImageElement.cpp
@@ -492,22 +492,16 @@ bool HTMLImageElement::IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
   int32_t tabIndex = TabIndex();
 
   if (IsInComposedDoc() && FindImageMap()) {
-    if (aTabIndex) {
-      // Use tab index on individual map areas
-      *aTabIndex = (sTabFocusModel & eTabFocus_linksMask) ? 0 : -1;
-    }
+    // Use tab index on individual map areas.
+    *aTabIndex = (sTabFocusModel & eTabFocus_linksMask) ? 0 : -1;
     // Image map is not focusable itself, but flag as tabbable
     // so that image map areas get walked into.
     *aIsFocusable = false;
-
     return false;
   }
 
-  if (aTabIndex) {
-    // Can be in tab order if tabindex >=0 and form controls are tabbable.
-    *aTabIndex = (sTabFocusModel & eTabFocus_formElementsMask) ? tabIndex : -1;
-  }
-
+  // Can be in tab order if tabindex >=0 and form controls are tabbable.
+  *aTabIndex = (sTabFocusModel & eTabFocus_formElementsMask) ? tabIndex : -1;
   *aIsFocusable = IsFormControlDefaultFocusable(aWithMouse) &&
                   (tabIndex >= 0 || GetTabIndexAttrValue().isSome());
 

diff --git a/dom/html/nsGenericHTMLElement.cpp b/dom/html/nsGenericHTMLElement.cpp
@@ -2286,30 +2286,26 @@ void nsGenericHTMLElement::Click(CallerType aCallerType) {
 
 bool nsGenericHTMLElement::IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
                                            int32_t* aTabIndex) {
+  MOZ_ASSERT(aIsFocusable);
+  MOZ_ASSERT(aTabIndex);
   if (ShadowRoot* root = GetShadowRoot()) {
     if (root->DelegatesFocus()) {
       *aIsFocusable = false;
       return true;
     }
   }
 
-  Document* doc = GetComposedDoc();
-  if (!doc || IsInDesignMode()) {
+  if (!IsInComposedDoc() || IsInDesignMode()) {
     // In designMode documents we only allow focusing the document.
-    if (aTabIndex) {
-      *aTabIndex = -1;
-    }
-
+    *aTabIndex = -1;
     *aIsFocusable = false;
-
     return true;
   }
 
-  int32_t tabIndex = TabIndex();
+  *aTabIndex = TabIndex();
   bool disabled = false;
   bool disallowOverridingFocusability = true;
   Maybe<int32_t> attrVal = GetTabIndexAttrValue();
-
   if (IsEditingHost()) {
     // Editable roots should always be focusable.
     disallowOverridingFocusability = true;
@@ -2319,26 +2315,21 @@ bool nsGenericHTMLElement::IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
     if (attrVal.isNothing()) {
       // The default value for tabindex should be 0 for editable
       // contentEditable roots.
-      tabIndex = 0;
+      *aTabIndex = 0;
     }
   } else {
     disallowOverridingFocusability = false;
 
     // Just check for disabled attribute on form controls
     disabled = IsDisabled();
     if (disabled) {
-      tabIndex = -1;
+      *aTabIndex = -1;
     }
   }
 
-  if (aTabIndex) {
-    *aTabIndex = tabIndex;
-  }
-
   // If a tabindex is specified at all, or the default tabindex is 0, we're
-  // focusable
-  *aIsFocusable = (tabIndex >= 0 || (!disabled && attrVal.isSome()));
-
+  // focusable.
+  *aIsFocusable = (*aTabIndex >= 0 || (!disabled && attrVal.isSome()));
   return disallowOverridingFocusability;
 }
 

diff --git a/dom/html/nsGenericHTMLElement.h b/dom/html/nsGenericHTMLElement.h
@@ -334,10 +334,10 @@ class nsGenericHTMLElement : public nsGenericHTMLElementBase {
   nsresult BindToTree(BindContext&, nsINode& aParent) override;
   void UnbindFromTree(bool aNullParent = true) override;
 
-  bool IsFocusableInternal(int32_t* aTabIndex, bool aWithMouse) override {
-    bool isFocusable = false;
-    IsHTMLFocusable(aWithMouse, &isFocusable, aTabIndex);
-    return isFocusable;
+  Focusable IsFocusableWithoutStyle(bool aWithMouse) override {
+    Focusable result;
+    IsHTMLFocusable(aWithMouse, &result.mFocusable, &result.mTabIndex);
+    return result;
   }
   /**
    * Returns true if a subclass is not allowed to override the value returned