From 52e3823a9c64a5a82b76b9279a6d68eecbcfb7dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 18 Mar 2021 19:53:38 +0000 Subject: [PATCH] Bug 1699154 - Tweak focusring heuristics for script focus. r=edgar What we implemented before this patch was basically what the heuristics in the spec said, which used to be normative: https://drafts.csswg.org/selectors/#the-focus-visible-pseudo That has become non-normative and there's ongoing discussion on what should happen for cases like this in: https://github.com/w3c/csswg-drafts/issues/5885 https://github.com/web-platform-tests/wpt/pull/27806 There seems to be agreement on that WPT issue on cases like this one, so let's make it work. Differential Revision: https://phabricator.services.mozilla.com/D108805 --- dom/base/nsFocusManager.cpp | 35 +++++-------------- dom/base/nsFocusManager.h | 1 - dom/base/nsGlobalWindowInner.cpp | 10 +++--- dom/base/nsGlobalWindowInner.h | 4 +-- dom/base/nsGlobalWindowOuter.cpp | 7 ++-- dom/base/nsGlobalWindowOuter.h | 4 +-- dom/base/nsPIDOMWindow.h | 25 +++++-------- dom/base/nsPIDOMWindowInlines.h | 4 +-- .../mochitest/general/test_focusrings.xhtml | 11 +++--- layout/reftests/bugs/reftest.list | 2 +- 10 files changed, 40 insertions(+), 63 deletions(-) diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp index d032613c73ab..0d3e459b950f 100644 --- a/dom/base/nsFocusManager.cpp +++ b/dom/base/nsFocusManager.cpp @@ -1225,16 +1225,14 @@ nsresult nsFocusManager::FocusPlugin(Element* aPlugin) { } nsFocusManager::BlurredElementInfo::BlurredElementInfo(Element& aElement) - : mElement(aElement), - mHadRing(aElement.State().HasState(NS_EVENT_STATE_FOCUSRING)) {} + : mElement(aElement) {} nsFocusManager::BlurredElementInfo::~BlurredElementInfo() = default; // https://drafts.csswg.org/selectors-4/#the-focus-visible-pseudo -static bool ShouldMatchFocusVisible( - nsPIDOMWindowOuter* aWindow, const Element& aElement, int32_t aFocusFlags, - const Maybe& aBlurredElementInfo, - bool aIsRefocus, bool aRefocusedElementUsedToShowOutline) { +static bool ShouldMatchFocusVisible(nsPIDOMWindowOuter* aWindow, + const Element& aElement, + int32_t aFocusFlags) { // If we were explicitly requested to show the ring, do it. if (aFocusFlags & nsIFocusManager::FLAG_SHOWRING) { return true; @@ -1269,20 +1267,9 @@ static bool ShouldMatchFocusVisible( // :focus). return true; case InputContextAction::CAUSE_UNKNOWN: - // If the active element matches :focus-visible, and a script causes focus - // to move elsewhere, the newly focused element should match - // :focus-visible. - // - // Conversely, if the active element does not match :focus-visible, and a - // script causes focus to move elsewhere, the newly focused element should - // not match :focus-visible. - // - // There's an special-case here. If this is a refocus, we just keep the - // outline as it was before, the focus isn't moving after all. - if (aIsRefocus) { - return aRefocusedElementUsedToShowOutline; - } - return !aBlurredElementInfo || aBlurredElementInfo->mHadRing; + // We render outlines if the last "known" focus method was by key or there + // was no previous known focus method, otherwise we don't. + return aWindow->UnknownFocusMethodShouldShowOutline(); case InputContextAction::CAUSE_MOUSE: case InputContextAction::CAUSE_TOUCH: case InputContextAction::CAUSE_LONGPRESS: @@ -2543,13 +2530,9 @@ void nsFocusManager::Focus( !IsNonFocusableRoot(aElement); const bool isRefocus = focusedNode && focusedNode == aElement; const bool shouldShowFocusRing = - sendFocusEvent && - ShouldMatchFocusVisible( - aWindow, *aElement, aFlags, aBlurredElementInfo, isRefocus, - isRefocus && aWindow->FocusedElementShowedOutline()); + sendFocusEvent && ShouldMatchFocusVisible(aWindow, *aElement, aFlags); - aWindow->SetFocusedElement(aElement, focusMethod, false, - shouldShowFocusRing); + aWindow->SetFocusedElement(aElement, focusMethod, false); // if the focused element changed, scroll it into view if (aElement && aFocusChanged) { diff --git a/dom/base/nsFocusManager.h b/dom/base/nsFocusManager.h index e659caf3018c..874b5da8f983 100644 --- a/dom/base/nsFocusManager.h +++ b/dom/base/nsFocusManager.h @@ -301,7 +301,6 @@ class nsFocusManager final : public nsIFocusManager, struct BlurredElementInfo { const mozilla::OwningNonNull mElement; - const bool mHadRing; explicit BlurredElementInfo(mozilla::dom::Element&); ~BlurredElementInfo(); diff --git a/dom/base/nsGlobalWindowInner.cpp b/dom/base/nsGlobalWindowInner.cpp index c186500b6f4a..aa88dc3ccf6c 100644 --- a/dom/base/nsGlobalWindowInner.cpp +++ b/dom/base/nsGlobalWindowInner.cpp @@ -4341,8 +4341,7 @@ void nsGlobalWindowInner::StopVRActivity() { void nsGlobalWindowInner::SetFocusedElement(Element* aElement, uint32_t aFocusMethod, - bool aNeedsFocus, - bool aWillShowOutline) { + bool aNeedsFocus) { if (aElement && aElement->GetComposedDoc() != mDoc) { NS_WARNING("Trying to set focus to a node from a wrong document"); return; @@ -4352,7 +4351,6 @@ void nsGlobalWindowInner::SetFocusedElement(Element* aElement, NS_ASSERTION(!aElement, "Trying to focus cleaned up window!"); aElement = nullptr; aNeedsFocus = false; - aWillShowOutline = false; } if (mFocusedElement != aElement) { UpdateCanvasFocus(false, aElement); @@ -4361,13 +4359,15 @@ void nsGlobalWindowInner::SetFocusedElement(Element* aElement, mFocusMethod = aFocusMethod & FOCUSMETHOD_MASK; } - mFocusedElementShowedOutlines = aWillShowOutline; - if (mFocusedElement) { // if a node was focused by a keypress, turn on focus rings for the // window. if (mFocusMethod & nsIFocusManager::FLAG_BYKEY) { + mUnknownFocusMethodShouldShowOutline = true; mFocusByKeyOccurred = true; + } else if (nsFocusManager::GetFocusMoveActionCause(mFocusMethod) != + widget::InputContextAction::CAUSE_UNKNOWN) { + mUnknownFocusMethodShouldShowOutline = false; } } diff --git a/dom/base/nsGlobalWindowInner.h b/dom/base/nsGlobalWindowInner.h index 18d87be7fa04..fb3b63267041 100644 --- a/dom/base/nsGlobalWindowInner.h +++ b/dom/base/nsGlobalWindowInner.h @@ -1130,8 +1130,8 @@ class nsGlobalWindowInner final : public mozilla::dom::EventTarget, bool IsInModalState(); void SetFocusedElement(mozilla::dom::Element* aElement, - uint32_t aFocusMethod = 0, bool aNeedsFocus = false, - bool aWillShowOutline = false) override; + uint32_t aFocusMethod = 0, + bool aNeedsFocus = false) override; uint32_t GetFocusMethod() override; diff --git a/dom/base/nsGlobalWindowOuter.cpp b/dom/base/nsGlobalWindowOuter.cpp index bdf6d7f5e695..1c30a4becc99 100644 --- a/dom/base/nsGlobalWindowOuter.cpp +++ b/dom/base/nsGlobalWindowOuter.cpp @@ -6711,10 +6711,9 @@ void nsGlobalWindowOuter::SetChromeEventHandler( void nsGlobalWindowOuter::SetFocusedElement(Element* aElement, uint32_t aFocusMethod, - bool aNeedsFocus, - bool aWillShowOutline) { - FORWARD_TO_INNER_VOID(SetFocusedElement, (aElement, aFocusMethod, aNeedsFocus, - aWillShowOutline)); + bool aNeedsFocus) { + FORWARD_TO_INNER_VOID(SetFocusedElement, + (aElement, aFocusMethod, aNeedsFocus)); } uint32_t nsGlobalWindowOuter::GetFocusMethod() { diff --git a/dom/base/nsGlobalWindowOuter.h b/dom/base/nsGlobalWindowOuter.h index 1cafe74ac07b..4281e4fb9a54 100644 --- a/dom/base/nsGlobalWindowOuter.h +++ b/dom/base/nsGlobalWindowOuter.h @@ -891,8 +891,8 @@ class nsGlobalWindowOuter final : public mozilla::dom::EventTarget, nsIBaseWindow* aWindow); void SetFocusedElement(mozilla::dom::Element* aElement, - uint32_t aFocusMethod = 0, bool aNeedsFocus = false, - bool aWillShowOutline = false) override; + uint32_t aFocusMethod = 0, + bool aNeedsFocus = false) override; uint32_t GetFocusMethod() override; diff --git a/dom/base/nsPIDOMWindow.h b/dom/base/nsPIDOMWindow.h index 76bca8a174d3..cf233a92a927 100644 --- a/dom/base/nsPIDOMWindow.h +++ b/dom/base/nsPIDOMWindow.h @@ -487,15 +487,10 @@ class nsPIDOMWindowInner : public mozIDOMWindow { virtual void SetFocusedElement(mozilla::dom::Element* aElement, uint32_t aFocusMethod = 0, - bool aNeedsFocus = false, - bool aWillShowOutline = false) = 0; - /** - * Get whether the focused element did show outlines when it was focused. - * - * Only for the focus manager. Returns false if there was no focused element. - */ - bool FocusedElementShowedOutline() const { - return mFocusedElementShowedOutlines; + bool aNeedsFocus = false) = 0; + + bool UnknownFocusMethodShouldShowOutline() const { + return mUnknownFocusMethodShouldShowOutline; } /** @@ -681,7 +676,7 @@ class nsPIDOMWindowInner : public mozIDOMWindow { // notification. bool mHasNotifiedGlobalCreated; - bool mFocusedElementShowedOutlines = false; + bool mUnknownFocusMethodShouldShowOutline = true; uint32_t mMarkedCCGeneration; @@ -953,14 +948,12 @@ class nsPIDOMWindowOuter : public mozIDOMWindowProxy { virtual void SetFocusedElement(mozilla::dom::Element* aElement, uint32_t aFocusMethod = 0, - bool aNeedsFocus = false, - bool aWillShowOutline = false) = 0; + bool aNeedsFocus = false) = 0; /** - * Get whether the focused element did show outlines when it was focused. - * - * Only for the focus manager. Returns false if there was no focused element. + * Get whether a focused element focused by unknown reasons (like script + * focus) should match the :focus-visible pseudo-class. */ - bool FocusedElementShowedOutline() const; + bool UnknownFocusMethodShouldShowOutline() const; /** * Retrieves the method that was used to focus the current node. diff --git a/dom/base/nsPIDOMWindowInlines.h b/dom/base/nsPIDOMWindowInlines.h index ee19bf653017..028762abf7cc 100644 --- a/dom/base/nsPIDOMWindowInlines.h +++ b/dom/base/nsPIDOMWindowInlines.h @@ -81,8 +81,8 @@ inline mozilla::dom::Element* nsPIDOMWindowOuter::GetFocusedElement() const { return mInnerWindow ? mInnerWindow->GetFocusedElement() : nullptr; } -inline bool nsPIDOMWindowOuter::FocusedElementShowedOutline() const { - return mInnerWindow && mInnerWindow->FocusedElementShowedOutline(); +inline bool nsPIDOMWindowOuter::UnknownFocusMethodShouldShowOutline() const { + return mInnerWindow && mInnerWindow->UnknownFocusMethodShouldShowOutline(); } #endif diff --git a/dom/tests/mochitest/general/test_focusrings.xhtml b/dom/tests/mochitest/general/test_focusrings.xhtml index 80cec266b392..302e6d952ab9 100644 --- a/dom/tests/mochitest/general/test_focusrings.xhtml +++ b/dom/tests/mochitest/general/test_focusrings.xhtml @@ -46,7 +46,7 @@ function runTest() } // make sure that a focus ring appears on the focused button - if (navigator.platform.includes("Mac")) { + if (isMac) { var focusedButton = $("b3"); ok(compareSnapshots(snapShot(focusedButton), snapShot($("b2")), true)[0], "unfocused shows no ring"); focusedButton.focus(); @@ -103,10 +103,10 @@ function testMacFocusesFormControl() var htmlElements = [ "", - "", - "", "", "", + "", + "", "", "", "", @@ -167,7 +167,10 @@ function testHTMLElements(list, isMac, expectedNoRingsOnWin) if (childdoc.activeElement) childdoc.activeElement.blur(); - ringSize = (elem.localName == "a" ? "0" : (expectedNoRingsOnWin ? 2 : 1)) + "px"; + ringSize = mouseRingSize; + if (isMac && !shouldFocus && elem.localName != "a") { + ringSize = "1px"; + } elem.focus(); is(childdoc.activeElement, elem, "focus() on " + list[e]); diff --git a/layout/reftests/bugs/reftest.list b/layout/reftests/bugs/reftest.list index bda370f96c25..637a268fad76 100644 --- a/layout/reftests/bugs/reftest.list +++ b/layout/reftests/bugs/reftest.list @@ -1909,7 +1909,7 @@ fuzzy-if(webrender,0-128,0-22) == 1155828-1.html 1155828-1-ref.html # bug 164652 fuzzy-if(skiaContent,0-7,0-84) == 1156129-1.html 1156129-1-ref.html pref(dom.use_xbl_scopes_for_remote_xul,true) HTTP(..) == 1157127-1.html 1157127-1-ref.html fuzzy-if(Android,0-6,0-6) == 1169331-1.html 1169331-1-ref.html -fuzzy(0-1,0-74) random-if(Android||gtkWidget) fuzzy-if(winWidget&&!nativeThemePref,0-68,0-28) == 1174332-1.html 1174332-1-ref.html # bug 1312658 +fuzzy(0-3,0-95) random-if(Android||gtkWidget) fuzzy-if(winWidget&&!nativeThemePref,0-68,0-257) == 1174332-1.html 1174332-1-ref.html # bug 1312658, plus on webrender fallback vs. non-fallback codepath causes fuzz == 1179078-1.html 1179078-1-ref.html == 1179288-1.html 1179288-1-ref.html == 1190635-1.html 1190635-1-ref.html