From bd8cffdd581fe7cc9693fd681a05c0d00a7d3789 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 3 Jan 2021 23:30:51 +0000 Subject: [PATCH] Bug 1351248 - Simplify nsIFrame::IsFocusable API. r=layout-reviewers,mats We always compute the tabindex value, so just return it to the caller all the time. This allows us to use early-returns which makes the code a bit easier to follow. This patch shouldn't change behavior. Differential Revision: https://phabricator.services.mozilla.com/D100423 --- dom/base/nsFocusManager.cpp | 41 ++++----- dom/events/EventStateManager.cpp | 3 +- dom/html/HTMLLegendElement.cpp | 3 +- dom/xul/nsXULPopupListener.cpp | 3 +- layout/base/AccessibleCaretManager.cpp | 3 +- layout/generic/nsIFrame.cpp | 114 ++++++++++++++++--------- layout/generic/nsIFrame.h | 20 +++-- 7 files changed, 110 insertions(+), 77 deletions(-) diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp index e39cef1aab98..bc165f897b2b 100644 --- a/dom/base/nsFocusManager.cpp +++ b/dom/base/nsFocusManager.cpp @@ -2033,8 +2033,7 @@ Element* nsFocusManager::FlushAndCheckIfFocusable(Element* aElement, : nullptr; } - return frame->IsFocusable(nullptr, aFlags & FLAG_BYMOUSE) ? aElement - : nullptr; + return frame->IsFocusable(aFlags & FLAG_BYMOUSE) ? aElement : nullptr; } bool nsFocusManager::Blur(BrowsingContext* aBrowsingContextToClear, @@ -3294,7 +3293,7 @@ nsresult nsFocusManager::DetermineElementToMoveFocus( if (startContent->IsHTMLElement(nsGkAtoms::area)) { startContent->IsFocusable(&tabIndex); } else if (frame) { - frame->IsFocusable(&tabIndex, 0); + tabIndex = frame->IsFocusable().mTabIndex; } else { startContent->IsFocusable(&tabIndex); } @@ -3528,7 +3527,7 @@ nsresult nsFocusManager::DetermineElementToMoveFocus( return NS_OK; } - frame->IsFocusable(&tabIndex, 0); + tabIndex = frame->IsFocusable().mTabIndex; if (tabIndex < 0) { tabIndex = 1; ignoreTabIndex = true; @@ -3747,13 +3746,8 @@ static int32_t HostOrSlotTabIndexValue(const nsIContent* aContent, MOZ_ASSERT(IsHostOrSlot(aContent)); if (aIsFocusable) { - *aIsFocusable = false; nsIFrame* frame = aContent->GetPrimaryFrame(); - if (frame) { - int32_t tabIndex; - frame->IsFocusable(&tabIndex, 0); - *aIsFocusable = tabIndex >= 0; - } + *aIsFocusable = frame && frame->IsFocusable().mTabIndex >= 0; } const nsAttrValue* attrVal = @@ -3777,10 +3771,11 @@ nsIContent* nsFocusManager::GetNextTabbableContentInScope( MOZ_ASSERT(IsHostOrSlot(aOwner), "Scope owner should be host or slot"); if (!aSkipOwner && (aForward && aOwner == aStartContent)) { - int32_t tabIndex = -1; - nsIFrame* frame = aOwner->GetPrimaryFrame(); - if (frame && frame->IsFocusable(&tabIndex, false) && tabIndex >= 0) { - return aOwner; + if (nsIFrame* frame = aOwner->GetPrimaryFrame()) { + auto focusable = frame->IsFocusable(); + if (focusable && focusable.mTabIndex >= 0) { + return aOwner; + } } } @@ -3821,7 +3816,7 @@ nsIContent* nsFocusManager::GetNextTabbableContentInScope( int32_t tabIndex = 0; if (iterContent->IsInNativeAnonymousSubtree() && iterContent->GetPrimaryFrame()) { - iterContent->GetPrimaryFrame()->IsFocusable(&tabIndex); + tabIndex = iterContent->GetPrimaryFrame()->IsFocusable().mTabIndex; } else if (IsHostOrSlot(iterContent)) { tabIndex = HostOrSlotTabIndexValue(iterContent); } else { @@ -3829,7 +3824,7 @@ nsIContent* nsFocusManager::GetNextTabbableContentInScope( if (!frame) { continue; } - frame->IsFocusable(&tabIndex, 0); + tabIndex = frame->IsFocusable().mTabIndex; } if (tabIndex < 0 || !(aIgnoreTabIndex || tabIndex == aCurrentTabIndex)) { continue; @@ -3888,10 +3883,11 @@ nsIContent* nsFocusManager::GetNextTabbableContentInScope( // Return scope owner at last for backward navigation if its tabindex // is non-negative if (!aSkipOwner && !aForward) { - int32_t tabIndex = -1; - nsIFrame* frame = aOwner->GetPrimaryFrame(); - if (frame && frame->IsFocusable(&tabIndex, false) && tabIndex >= 0) { - return aOwner; + if (nsIFrame* frame = aOwner->GetPrimaryFrame()) { + auto focusable = frame->IsFocusable(); + if (focusable && focusable.mTabIndex >= 0) { + return aOwner; + } } } @@ -3913,7 +3909,7 @@ nsIContent* nsFocusManager::GetNextTabbableContentInAncestorScopes( if (IsHostOrSlot(startContent)) { tabIndex = HostOrSlotTabIndexValue(startContent); } else if (nsIFrame* frame = startContent->GetPrimaryFrame()) { - frame->IsFocusable(&tabIndex); + tabIndex = frame->IsFocusable().mTabIndex; } else { startContent->IsFocusable(&tabIndex); } @@ -4201,8 +4197,7 @@ nsresult nsFocusManager::GetNextTabbableContent( // == 0 in normal tab order (last after positive tabindexed items) // > 0 can be tabbed to in the order specified by this value // clang-format on - int32_t tabIndex; - frame->IsFocusable(&tabIndex, 0); + int32_t tabIndex = frame->IsFocusable().mTabIndex; LOGCONTENTNAVIGATION("Next Tabbable %s:", frame->GetContent()); LOGFOCUSNAVIGATION( diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp index 2a217a9337da..c027b792cfdd 100644 --- a/dom/events/EventStateManager.cpp +++ b/dom/events/EventStateManager.cpp @@ -3339,8 +3339,7 @@ nsresult EventStateManager::PostHandleEvent(nsPresContext* aPresContext, break; } - int32_t tabIndexUnused; - if (frame->IsFocusable(&tabIndexUnused, true)) { + if (frame->IsFocusable(/* aWithMouse = */ true)) { break; } } diff --git a/dom/html/HTMLLegendElement.cpp b/dom/html/HTMLLegendElement.cpp index d6afcbe9b07a..93b5200f96e7 100644 --- a/dom/html/HTMLLegendElement.cpp +++ b/dom/html/HTMLLegendElement.cpp @@ -74,8 +74,7 @@ void HTMLLegendElement::Focus(const FocusOptions& aOptions, return; } - int32_t tabIndex; - if (frame->IsFocusable(&tabIndex, false)) { + if (frame->IsFocusable()) { nsGenericHTMLElement::Focus(aOptions, aCallerType, aError); return; } diff --git a/dom/xul/nsXULPopupListener.cpp b/dom/xul/nsXULPopupListener.cpp index d0f2a1056f20..1092dd045016 100644 --- a/dom/xul/nsXULPopupListener.cpp +++ b/dom/xul/nsXULPopupListener.cpp @@ -201,8 +201,7 @@ nsresult nsXULPopupListener::FireFocusOnTargetContent( nsIFrame* currFrame = targetFrame; // Look for the nearest enclosing focusable frame. while (currFrame) { - int32_t tabIndexUnused; - if (currFrame->IsFocusable(&tabIndexUnused, true) && + if (currFrame->IsFocusable(/* aWithMouse = */ true) && currFrame->GetContent()->IsElement()) { newFocusElement = currFrame->GetContent()->AsElement(); break; diff --git a/layout/base/AccessibleCaretManager.cpp b/layout/base/AccessibleCaretManager.cpp index 34cea2093df7..e169f498f7f2 100644 --- a/layout/base/AccessibleCaretManager.cpp +++ b/layout/base/AccessibleCaretManager.cpp @@ -30,6 +30,7 @@ #include "nsGenericHTMLElement.h" #include "nsIHapticFeedback.h" #include "nsIScrollableFrame.h" +#include "nsLayoutUtils.h" #include "nsServiceManagerUtils.h" namespace mozilla { @@ -862,7 +863,7 @@ nsIFrame* AccessibleCaretManager::GetFocusableFrame(nsIFrame* aFrame) const { // Look for the nearest enclosing focusable frame. nsIFrame* focusableFrame = aFrame; while (focusableFrame) { - if (focusableFrame->IsFocusable(nullptr, true)) { + if (focusableFrame->IsFocusable(/* aWithMouse = */ true)) { break; } focusableFrame = focusableFrame->GetParent(); diff --git a/layout/generic/nsIFrame.cpp b/layout/generic/nsIFrame.cpp index 6f2dd30a4b76..f8e8eaffe965 100644 --- a/layout/generic/nsIFrame.cpp +++ b/layout/generic/nsIFrame.cpp @@ -10086,58 +10086,88 @@ void nsIFrame::GetFirstLeaf(nsIFrame** aFrame) { } } -/* virtual */ -bool nsIFrame::IsFocusable(int32_t* aTabIndex, bool aWithMouse) { - int32_t tabIndex = -1; - if (aTabIndex) { - *aTabIndex = -1; // Default for early return is not focusable +bool nsIFrame::IsFocusableDueToScrollFrame() { + if (!IsScrollFrame()) { + return false; } - bool isFocusable = false; + if (!mContent->IsHTMLElement()) { + return false; + } + if (mContent->IsRootOfNativeAnonymousSubtree()) { + return false; + } + if (!mContent->GetParent()) { + return false; + } + if (mContent->AsElement()->HasAttr(nsGkAtoms::tabindex)) { + return false; + } + // Elements with scrollable view are focusable with script & tabbable + // Otherwise you couldn't scroll them with keyboard, which is an accessibility + // issue (e.g. Section 508 rules) However, we don't make them to be focusable + // with the mouse, because the extra focus outlines are considered + // unnecessarily ugly. When clicked on, the selection position within the + // element will be enough to make them keyboard scrollable. + nsIScrollableFrame* scrollFrame = do_QueryFrame(this); + if (!scrollFrame) { + return false; + } + if (scrollFrame->IsForTextControlWithNoScrollbars()) { + return false; + } + if (scrollFrame->GetScrollStyles().IsHiddenInBothDirections()) { + return false; + } + if (scrollFrame->GetScrollRange().IsEqualEdges(nsRect(0, 0, 0, 0))) { + return false; + } + return true; +} +nsIFrame::Focusable nsIFrame::IsFocusable(bool aWithMouse) { // cannot focus content in print preview mode. Only the root can be focused, // but that's handled elsewhere. if (PresContext()->Type() == nsPresContext::eContext_PrintPreview) { - return false; + return {}; } - if (mContent && mContent->IsElement() && IsVisibleConsideringAncestors() && - Style()->GetPseudoType() != PseudoStyleType::anonymousFlexItem && - Style()->GetPseudoType() != PseudoStyleType::anonymousGridItem && - StyleUI()->mInert != StyleInert::Inert) { - const nsStyleUI* ui = StyleUI(); - if (ui->mUserFocus != StyleUserFocus::Ignore && - ui->mUserFocus != StyleUserFocus::None) { - // Pass in default tabindex of -1 for nonfocusable and 0 for focusable - tabIndex = 0; - } - isFocusable = mContent->IsFocusable(&tabIndex, aWithMouse); - if (!isFocusable && !aWithMouse && IsScrollFrame() && - mContent->IsHTMLElement() && - !mContent->IsRootOfNativeAnonymousSubtree() && mContent->GetParent() && - !mContent->AsElement()->HasAttr(kNameSpaceID_None, - nsGkAtoms::tabindex)) { - // Elements with scrollable view are focusable with script & tabbable - // Otherwise you couldn't scroll them with keyboard, which is - // an accessibility issue (e.g. Section 508 rules) - // However, we don't make them to be focusable with the mouse, - // because the extra focus outlines are considered unnecessarily ugly. - // When clicked on, the selection position within the element - // will be enough to make them keyboard scrollable. - nsIScrollableFrame* scrollFrame = do_QueryFrame(this); - if (scrollFrame && !scrollFrame->IsForTextControlWithNoScrollbars() && - !scrollFrame->GetScrollStyles().IsHiddenInBothDirections() && - !scrollFrame->GetScrollRange().IsEqualEdges(nsRect(0, 0, 0, 0))) { - // Scroll bars will be used for overflow - isFocusable = true; - tabIndex = 0; - } - } + if (!mContent || !mContent->IsElement()) { + return {}; } - if (aTabIndex) { - *aTabIndex = tabIndex; + if (!IsVisibleConsideringAncestors()) { + return {}; } - return isFocusable; + + const nsStyleUI* ui = StyleUI(); + if (ui->mInert == StyleInert::Inert) { + return {}; + } + + PseudoStyleType pseudo = Style()->GetPseudoType(); + if (pseudo == PseudoStyleType::anonymousFlexItem || + pseudo == PseudoStyleType::anonymousGridItem) { + return {}; + } + + int32_t tabIndex = -1; + if (ui->mUserFocus != StyleUserFocus::Ignore && + ui->mUserFocus != StyleUserFocus::None) { + // Pass in default tabindex of -1 for nonfocusable and 0 for focusable + tabIndex = 0; + } + + if (mContent->IsFocusable(&tabIndex, aWithMouse)) { + // If the content is focusable, then we're done. + return {true, tabIndex}; + } + + // If we're focusing with the mouse we never focus scroll areas. + if (!aWithMouse && IsFocusableDueToScrollFrame()) { + return {true, 0}; + } + + return {false, tabIndex}; } /** diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h index f67107450482..28c556f3ae52 100644 --- a/layout/generic/nsIFrame.h +++ b/layout/generic/nsIFrame.h @@ -4200,6 +4200,17 @@ class nsIFrame : public nsQueryFrame { const nsStyleEffects* aEffects, const nsSize& aSize) const; + 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; } + }; + /** * Check if this frame is focusable and in the current tab order. * Tabbable is indicated by a nonnegative tabindex & is a subset of focusable. @@ -4211,14 +4222,10 @@ class nsIFrame : public nsQueryFrame { * Also, depending on the pref accessibility.tabfocus some widgets may be * focusable but removed from the tab order. This is the default on * Mac OS X, where fewer items are focusable. - * @param [in, 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 * @param [in, optional] aWithMouse, is this focus query for mouse clicking * @return whether the frame is focusable via mouse, kbd or script. */ - bool IsFocusable(int32_t* aTabIndex = nullptr, bool aWithMouse = false); + [[nodiscard]] Focusable IsFocusable(bool aWithMouse = false); // BOX LAYOUT METHODS // These methods have been migrated from nsIBox and are in the process of @@ -4321,6 +4328,9 @@ class nsIFrame : public nsQueryFrame { static nsIFrame* GetParentXULBox(const nsIFrame* aFrame); protected: + // Helper for IsFocusable. + bool IsFocusableDueToScrollFrame(); + /** * Returns true if this box clips its children, e.g., if this box is an * scrollbox.