Skip to content

Commit

Permalink
Bug 1699154 - Tweak focusring heuristics for script focus. r=edgar
Browse files Browse the repository at this point in the history 
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:

  w3c/csswg-drafts#5885
  web-platform-tests/wpt#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
  • Loading branch information
@emilio
emilio committed Mar 18, 2021
1 parent 7f55e68 commit b892d58
Show file tree
Hide file tree
Showing 10 changed files with 40 additions and 63 deletions.
35 changes: 9 additions & 26 deletions dom/base/nsFocusManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<nsFocusManager::BlurredElementInfo>& 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;
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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) {
Expand Down
1 change: 0 additions & 1 deletion dom/base/nsFocusManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,6 @@ class nsFocusManager final : public nsIFocusManager,

struct BlurredElementInfo {
const mozilla::OwningNonNull<mozilla::dom::Element> mElement;
const bool mHadRing;

explicit BlurredElementInfo(mozilla::dom::Element&);
~BlurredElementInfo();
Expand Down
10 changes: 5 additions & 5 deletions dom/base/nsGlobalWindowInner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4336,8 +4336,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;
Expand All @@ -4347,7 +4346,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);
Expand All @@ -4356,13 +4354,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;
}
}

Expand Down
4 changes: 2 additions & 2 deletions dom/base/nsGlobalWindowInner.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
7 changes: 3 additions & 4 deletions dom/base/nsGlobalWindowOuter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6710,10 +6710,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() {
Expand Down
4 changes: 2 additions & 2 deletions dom/base/nsGlobalWindowOuter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -895,8 +895,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;

Expand Down
25 changes: 9 additions & 16 deletions dom/base/nsPIDOMWindow.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -681,7 +676,7 @@ class nsPIDOMWindowInner : public mozIDOMWindow {
// notification.
bool mHasNotifiedGlobalCreated;

bool mFocusedElementShowedOutlines = false;
bool mUnknownFocusMethodShouldShowOutline = true;

uint32_t mMarkedCCGeneration;

Expand Down Expand Up @@ -955,14 +950,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.
Expand Down
4 changes: 2 additions & 2 deletions dom/base/nsPIDOMWindowInlines.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
11 changes: 7 additions & 4 deletions dom/tests/mochitest/general/test_focusrings.xhtml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -103,10 +103,10 @@ function testMacFocusesFormControl()

var htmlElements = [
"<button id='elem'>Button</button>",
"<input id='elem' class='canfocus'>",
"<input id='elem' type='password' class='canfocus'>",
"<input id='elem' type='button'>",
"<input id='elem' type='checkbox'>",
"<input id='elem' class='canfocus'>",
"<input id='elem' type='password' class='canfocus'>",
"<textarea id='elem' class='canfocus'></textarea>",
"<select id='elem' class='canfocus'><option>One</select>",
"<select id='elem' rows='5' class='canfocus'><option>One</select>",
Expand Down Expand Up @@ -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]);
Expand Down
2 changes: 1 addition & 1 deletion layout/reftests/bugs/reftest.list
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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-28) == 1174332-1.html 1174332-1-ref.html # bug 1312658
== 1179078-1.html 1179078-1-ref.html
== 1179288-1.html 1179288-1-ref.html
== 1190635-1.html 1190635-1-ref.html
Expand Down

0 comments on commit b892d58

Please sign in to comment.