From f1b1884de44a6aa7dda3b08a0d9bff0b9f7a9e88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 15 May 2020 20:12:14 +0000 Subject: [PATCH] Bug 1638127 - Remove nsGlobalWindowInner::mShowFocusRingForContent. r=edgar Instead move the check to the focus manager, more similar to how focus-visible works. Now nsGlobalWindowInner::ShouldShowFocusRing means "Should we show focus ring for anything in this window", that is: Have we keyboard-navigated in this window, or do we have a pref that says that we should always show focus rings. Fix some callers appropriately (some of them that were not properly accounting for the element being focused in the first place...). Differential Revision: https://phabricator.services.mozilla.com/D75504 --- dom/base/nsFocusManager.cpp | 35 +++++++++++++++++-------- dom/base/nsGlobalWindowInner.cpp | 25 +----------------- dom/base/nsGlobalWindowInner.h | 4 --- dom/canvas/CanvasRenderingContext2D.cpp | 20 +++++--------- layout/forms/nsComboboxControlFrame.cpp | 18 +++++-------- 5 files changed, 38 insertions(+), 64 deletions(-) diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp index 2a7c79efe6a2..18ee85fc2607 100644 --- a/dom/base/nsFocusManager.cpp +++ b/dom/base/nsFocusManager.cpp @@ -1114,12 +1114,6 @@ nsFocusManager::ParentActivated(mozIDOMWindowProxy* aWindow, bool aActive) { static bool ShouldMatchFocusVisible(const Element& aElement, int32_t aFocusFlags) { - if (StaticPrefs::browser_display_show_focus_rings()) { - // FIXME: Spec is ambiguous about whether we should take platform - // conventions into account. This branch does make us account for them. - return true; - } - switch (nsFocusManager::GetFocusMoveActionCause(aFocusFlags)) { case InputContextAction::CAUSE_UNKNOWN: case InputContextAction::CAUSE_KEY: @@ -1142,6 +1136,22 @@ static bool ShouldMatchFocusVisible(const Element& aElement, return false; } +// On Windows, focus rings are only shown when the FLAG_SHOWRING flag is used. +static bool ShouldShowFocusRingForElement(Element& aElement, int32_t aFlags) { + if (aFlags & nsIFocusManager::FLAG_SHOWRING) { + return true; + } +#ifndef XP_WIN + if (aFlags & nsIFocusManager::FLAG_BYMOUSE) { + return !nsContentUtils::ContentIsLink(&aElement) && + !aElement.IsAnyOfHTMLElements(nsGkAtoms::video, nsGkAtoms::audio); + } + return true; +#else + return false; +#endif +} + /* static */ void nsFocusManager::NotifyFocusStateChange(nsIContent* aContent, nsIContent* aContentToFocus, @@ -1149,7 +1159,8 @@ void nsFocusManager::NotifyFocusStateChange(nsIContent* aContent, int32_t aFlags, bool aGettingFocus) { MOZ_ASSERT_IF(aContentToFocus, !aGettingFocus); - if (!aContent->IsElement()) { + auto* element = Element::FromNode(aContent); + if (!element) { return; } @@ -1161,18 +1172,20 @@ void nsFocusManager::NotifyFocusStateChange(nsIContent* aContent, if (aGettingFocus) { EventStates eventStateToAdd = NS_EVENT_STATE_FOCUS; - if (aWindowShouldShowFocusRing) { + if (aWindowShouldShowFocusRing || + ShouldShowFocusRingForElement(*element, aFlags)) { eventStateToAdd |= NS_EVENT_STATE_FOCUSRING; } - if (ShouldMatchFocusVisible(*aContent->AsElement(), aFlags)) { + if (aWindowShouldShowFocusRing || + ShouldMatchFocusVisible(*element, aFlags)) { eventStateToAdd |= NS_EVENT_STATE_FOCUS_VISIBLE; } - aContent->AsElement()->AddStates(eventStateToAdd); + element->AddStates(eventStateToAdd); } else { EventStates eventStateToRemove = NS_EVENT_STATE_FOCUS | NS_EVENT_STATE_FOCUSRING | NS_EVENT_STATE_FOCUS_VISIBLE; - aContent->AsElement()->RemoveStates(eventStateToRemove); + element->RemoveStates(eventStateToRemove); } for (nsIContent* content = aContent; content && content != commonAncestor; diff --git a/dom/base/nsGlobalWindowInner.cpp b/dom/base/nsGlobalWindowInner.cpp index 9c29c14e50dc..a61328e061cf 100644 --- a/dom/base/nsGlobalWindowInner.cpp +++ b/dom/base/nsGlobalWindowInner.cpp @@ -831,7 +831,6 @@ nsGlobalWindowInner::nsGlobalWindowInner(nsGlobalWindowOuter* aOuterWindow, mCleanMessageManager(false), mNeedsFocus(true), mHasFocus(false), - mShowFocusRingForContent(false), mFocusByKeyOccurred(false), mDidFireDocElemInserted(false), mHasGamepad(false), @@ -4087,16 +4086,6 @@ void nsGlobalWindowInner::StopVRActivity() { } } -#ifndef XP_WIN // This guard should match the guard at the callsite. -static bool ShouldShowFocusRingIfFocusedByMouse(nsIContent* aNode) { - if (!aNode) { - return true; - } - return !nsContentUtils::ContentIsLink(aNode) && - !aNode->IsAnyOfHTMLElements(nsGkAtoms::video, nsGkAtoms::audio); -} -#endif - void nsGlobalWindowInner::SetFocusedElement(Element* aElement, uint32_t aFocusMethod, bool aNeedsFocus) { @@ -4114,7 +4103,6 @@ void nsGlobalWindowInner::SetFocusedElement(Element* aElement, UpdateCanvasFocus(false, aElement); mFocusedElement = aElement; mFocusMethod = aFocusMethod & FOCUSMETHOD_MASK; - mShowFocusRingForContent = false; } if (mFocusedElement) { @@ -4122,17 +4110,6 @@ void nsGlobalWindowInner::SetFocusedElement(Element* aElement, // window. if (mFocusMethod & nsIFocusManager::FLAG_BYKEY) { mFocusByKeyOccurred = true; - } else if ( - // otherwise, we set mShowFocusRingForContent, as we don't want this to - // be permanent for the window. On Windows, focus rings are only shown - // when the FLAG_SHOWRING flag is used. On other platforms, focus rings - // are only visible on some elements. -#ifndef XP_WIN - !(mFocusMethod & nsIFocusManager::FLAG_BYMOUSE) || - ShouldShowFocusRingIfFocusedByMouse(aElement) || -#endif - aFocusMethod & nsIFocusManager::FLAG_SHOWRING) { - mShowFocusRingForContent = true; } } @@ -4142,7 +4119,7 @@ void nsGlobalWindowInner::SetFocusedElement(Element* aElement, uint32_t nsGlobalWindowInner::GetFocusMethod() { return mFocusMethod; } bool nsGlobalWindowInner::ShouldShowFocusRing() { - if (mShowFocusRingForContent || mFocusByKeyOccurred) { + if (mFocusByKeyOccurred) { return true; } diff --git a/dom/base/nsGlobalWindowInner.h b/dom/base/nsGlobalWindowInner.h index c852c3cb9eb0..70cdf7fdb9de 100644 --- a/dom/base/nsGlobalWindowInner.h +++ b/dom/base/nsGlobalWindowInner.h @@ -1259,10 +1259,6 @@ class nsGlobalWindowInner final : public mozilla::dom::EventTarget, bool mNeedsFocus : 1; bool mHasFocus : 1; - // when true, show focus rings for the current focused content only. - // This will be reset when another element is focused - bool mShowFocusRingForContent : 1; - // true if tab navigation has occurred for this window. Focus rings // should be displayed. bool mFocusByKeyOccurred : 1; diff --git a/dom/canvas/CanvasRenderingContext2D.cpp b/dom/canvas/CanvasRenderingContext2D.cpp index ffced13b06cc..55b5ca056e63 100644 --- a/dom/canvas/CanvasRenderingContext2D.cpp +++ b/dom/canvas/CanvasRenderingContext2D.cpp @@ -2917,26 +2917,18 @@ void CanvasRenderingContext2D::DrawFocusIfNeeded( } } -bool CanvasRenderingContext2D::DrawCustomFocusRing( - mozilla::dom::Element& aElement) { - EnsureUserSpacePath(); +bool CanvasRenderingContext2D::DrawCustomFocusRing(Element& aElement) { + if (!aElement.State().HasState(NS_EVENT_STATE_FOCUSRING)) { + return false; + } HTMLCanvasElement* canvas = GetCanvas(); - if (!canvas || !aElement.IsInclusiveDescendantOf(canvas)) { return false; } - if (nsFocusManager* fm = nsFocusManager::GetFocusManager()) { - // check that the element is focused - if (&aElement == fm->GetFocusedElement()) { - if (nsPIDOMWindowOuter* window = aElement.OwnerDoc()->GetWindow()) { - return window->ShouldShowFocusRing(); - } - } - } - - return false; + EnsureUserSpacePath(); + return true; } void CanvasRenderingContext2D::Clip(const CanvasWindingRule& aWinding) { diff --git a/layout/forms/nsComboboxControlFrame.cpp b/layout/forms/nsComboboxControlFrame.cpp index be25c46f0fab..759bcb354d96 100644 --- a/layout/forms/nsComboboxControlFrame.cpp +++ b/layout/forms/nsComboboxControlFrame.cpp @@ -1499,17 +1499,13 @@ void nsComboboxControlFrame::BuildDisplayList(nsDisplayListBuilder* aBuilder, } // draw a focus indicator only when focus rings should be drawn - if (Document* doc = mContent->GetComposedDoc()) { - nsPIDOMWindowOuter* window = doc->GetWindow(); - if (window && window->ShouldShowFocusRing()) { - nsPresContext* pc = PresContext(); - const nsStyleDisplay* disp = StyleDisplay(); - if (IsThemed(disp) && - pc->Theme()->ThemeWantsButtonInnerFocusRing(disp->mAppearance) && - mDisplayFrame && IsVisibleForPainting()) { - aLists.Content()->AppendNewToTop(aBuilder, - this); - } + if (mContent->AsElement()->State().HasState(NS_EVENT_STATE_FOCUSRING)) { + nsPresContext* pc = PresContext(); + const nsStyleDisplay* disp = StyleDisplay(); + if (IsThemed(disp) && + pc->Theme()->ThemeWantsButtonInnerFocusRing(disp->mAppearance) && + mDisplayFrame && IsVisibleForPainting()) { + aLists.Content()->AppendNewToTop(aBuilder, this); } }