Skip to content

Commit

Permalink
Bug 1765083 - Introduce FocusOptions.focusVisible. r=smaug,pip-reviewers
Browse files Browse the repository at this point in the history
As per:

 * whatwg/html#7830
 * whatwg/html#8087

Replace the internal preventFocusRing with the new flag.

Differential Revision: https://phabricator.services.mozilla.com/D151326
  • Loading branch information
emilio committed Jul 11, 2022
1 parent dcebb1d commit c84f2d6
Show file tree
Hide file tree
Showing 16 changed files with 84 additions and 47 deletions.
2 changes: 1 addition & 1 deletion browser/base/content/certerror/aboutNetError.js
Original file line number Diff line number Diff line change
Expand Up @@ -1176,7 +1176,7 @@ function setFocus(selector, position = "afterbegin") {
// be focused. We use a requestAnimationFrame to queue up the focus to occur
// once the button has its frame.
requestAnimationFrame(() => {
button.focus({ preventFocusRing: true });
button.focus({ focusVisible: false });
});
}
}
Expand Down
2 changes: 1 addition & 1 deletion browser/base/content/pageinfo/pageInfo.js
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ async function loadTab(args) {
document.getElementById("generalTab");
radioGroup.selectedItem = initialTab;
radioGroup.selectedItem.doCommand();
radioGroup.focus({ preventFocusRing: true });
radioGroup.focus({ focusVisible: false });
}

function openCacheEntry(key, cb) {
Expand Down
2 changes: 1 addition & 1 deletion browser/base/content/spotlight.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ async function renderSpotlight(ready) {
// If we just call focus() at some random time, it'll cause a flush,
// which slows things down unnecessarily, so instead we use rAF...
requestAnimationFrame(() => {
primaryBtn.focus({ preventFocusRing: true });
primaryBtn.focus({ focusVisible: false });
});
}
if (secondaryBtn) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ var ContentAreaDownloadsView = {
// And prevent it from showing the focus ring around the richlistbox (Bug 1702694)
document
.getElementById("downloadsListBox")
.focus({ preventFocusRing: true });
.focus({ focusVisible: false });
// Pause the indicator if the browser is active.
if (document.visibilityState === "visible") {
DownloadsCommon.getIndicatorData(
Expand Down
5 changes: 4 additions & 1 deletion browser/components/downloads/content/downloads.js
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,10 @@ var DownloadsPanel = {
if (document.activeElement && this.panel.contains(document.activeElement)) {
return;
}
let focusOptions = { preventFocusRing: !!this._preventFocusRing };
let focusOptions = {};
if (this._preventFocusRing) {
focusOptions.focusRing = false;
}
if (DownloadsView.richListBox.itemCount > 0) {
DownloadsView.richListBox.selectedIndex = 0;
DownloadsView.richListBox.focus(focusOptions);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ window.onload = function() {

initTreeView();

errorTryAgainButton.focus({ preventFocusRing: true });
errorTryAgainButton.focus({ focusVisible: false });
};

function isTreeViewVisible() {
Expand Down
15 changes: 0 additions & 15 deletions dom/base/nsContentUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2114,21 +2114,6 @@ bool nsContentUtils::IsCallerChromeOrElementTransformGettersEnabled(
StaticPrefs::dom_element_transform_getters_enabled();
}

/* static */
bool nsContentUtils::IsCallerChromeOrErrorPage(JSContext* aCx,
JSObject* aObject) {
if (ThreadsafeIsSystemCaller(aCx)) {
return true;
}
nsGlobalWindowInner* win =
xpc::WindowGlobalOrNull(js::UncheckedUnwrap(aObject));
if (!win) {
return false;
}
Document* doc = win->GetExtantDoc();
return doc && IsErrorPage(doc->GetDocumentURI());
}

/* static */
bool nsContentUtils::ShouldResistFingerprinting() {
return StaticPrefs::privacy_resistFingerprinting();
Expand Down
2 changes: 0 additions & 2 deletions dom/base/nsContentUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,6 @@ class nsContentUtils {
static bool IsCallerChromeOrElementTransformGettersEnabled(JSContext* aCx,
JSObject*);

static bool IsCallerChromeOrErrorPage(JSContext*, JSObject*);

// The APIs for checking whether the caller is system (in the sense of system
// principal) should only be used when the JSContext is known to accurately
// represent the caller. In practice, that means you should only use them in
Expand Down
12 changes: 6 additions & 6 deletions dom/base/nsFocusManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1236,6 +1236,10 @@ static bool ShouldMatchFocusVisible(nsPIDOMWindowOuter* aWindow,
return true;
}

if (aFocusFlags & nsIFocusManager::FLAG_NOSHOWRING) {
return false;
}

if (aWindow->ShouldShowFocusRing()) {
// The window decision also trumps any other heuristic.
return true;
Expand All @@ -1257,10 +1261,6 @@ static bool ShouldMatchFocusVisible(nsPIDOMWindowOuter* aWindow,
}
}

if (aFocusFlags & nsIFocusManager::FLAG_NOSHOWRING) {
return false;
}

switch (nsFocusManager::GetFocusMoveActionCause(aFocusFlags)) {
case InputContextAction::CAUSE_KEY:
// If the user interacts with the page via the keyboard, the currently
Expand Down Expand Up @@ -3718,8 +3718,8 @@ uint32_t nsFocusManager::ProgrammaticFocusFlags(const FocusOptions& aOptions) {
if (aOptions.mPreventScroll) {
flags |= FLAG_NOSCROLL;
}
if (aOptions.mPreventFocusRing) {
flags |= FLAG_NOSHOWRING;
if (aOptions.mFocusVisible.WasPassed()) {
flags |= aOptions.mFocusVisible.Value() ? FLAG_SHOWRING : FLAG_NOSHOWRING;
}
if (UserActivation::IsHandlingKeyboardInput()) {
flags |= FLAG_BYKEY;
Expand Down
1 change: 0 additions & 1 deletion dom/html/test/mochitest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,6 @@ skip-if = toolkit == 'android' && !is_fennec # Bug 1525959
[test_element_prototype.html]
[test_embed_attributes_reflection.html]
[test_focusshift_button.html]
[test_focus_preventFocusRing.html]
[test_formData.html]
[test_formSubmission.html]
skip-if = toolkit == 'android' #TIMED_OUT
Expand Down
11 changes: 0 additions & 11 deletions dom/html/test/test_focus_preventFocusRing.html

This file was deleted.

4 changes: 1 addition & 3 deletions dom/webidl/Element.webidl
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,7 @@ interface Element : Node {
// https://html.spec.whatwg.org/#focus-management-apis
dictionary FocusOptions {
boolean preventScroll = false;
// Prevents the focus ring if this is not a text control / editable element.
[Func="nsContentUtils::IsCallerChromeOrErrorPage"]
boolean preventFocusRing = false;
boolean focusVisible;
};

interface mixin HTMLOrForeignElement {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<!doctype html>

<title>focus(options) - focusVisible</title>
<script src=/resources/testharness.js></script>
<script src=/resources/testharnessreport.js></script>
<script src="/resources/testdriver.js"></script>
<script src="/resources/testdriver-actions.js"></script>
<script src="/resources/testdriver-vendor.js"></script>

<style>
div {
height: 10px;
border: 1px solid black;
}
</style>

<button>ABC</button>
<input>
<div id="contenteditable" contenteditable></div>
<div id="tabindex" tabindex=0></div>
<div id="not_focusable"></div>

<div id="reset_focus" tabindex=0></div>

<script>
const reset_focus = document.getElementById("reset_focus");

async function check_focus_visible(element, options, { shouldBeVisible, shouldBeFocused }) {
// Reset focus by clicking on a div, which should not show focus rings.
await test_driver.click(reset_focus);

assert_equals(document.activeElement, reset_focus, "Reset should be focused");
assert_true(reset_focus.matches(":focus"), "Clicking focusable div should match :focus");
assert_false(reset_focus.matches(":focus-visible"), "Clicking focusable div shouldn't match :focus-visible");

element.focus(options);

assert_equals(document.activeElement, shouldBeFocused ? element : reset_focus, "activeElement");
assert_equals(element.matches(":focus"), shouldBeFocused, ":focus");
assert_equals(element.matches(":focus-visible"), shouldBeVisible, ":focus-visible");
}

for (let selector of ["button", "input", "#contenteditable", "#tabindex", "#not_focusable"]) {
promise_test(async function() {
const takesKeyboardInput = selector == "#contenteditable" || selector == "input";
const shouldBeFocused = selector != "#not_focusable";

const element = document.querySelector(selector);

await check_focus_visible(element, {}, {
shouldBeVisible: takesKeyboardInput,
shouldBeFocused,
});

await check_focus_visible(element, { focusVisible: true }, {
shouldBeVisible: shouldBeFocused,
shouldBeFocused,
});
await check_focus_visible(element, { focusVisible: false }, {
shouldBeVisible: false,
shouldBeFocused,
});
}, "FocusOptions.focusVisible: " + selector);
}
</script>
2 changes: 1 addition & 1 deletion toolkit/components/aboutconfig/content/aboutconfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ if (!Preferences.get("browser.aboutConfig.showWarning")) {
document.addEventListener("DOMContentLoaded", function() {
let warningButton = document.getElementById("warningButton");
warningButton.addEventListener("click", onWarningButtonClick);
warningButton.focus({ preventFocusRing: true });
warningButton.focus({ focusVisible: false });
});
}

Expand Down
2 changes: 1 addition & 1 deletion toolkit/components/pictureinpicture/content/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ let Player = {
browser.setAttribute("nodefaultsrc", "true");

let playPauseBtn = document.getElementById("playpause");
playPauseBtn.focus({ preventFocusRing: true });
playPauseBtn.focus({ focusVisible: false });

// Set the specific remoteType and browsingContextGroupID to use for the
// initial about:blank load. The combination of these two properties will
Expand Down
2 changes: 1 addition & 1 deletion toolkit/components/prompts/src/CommonDialog.jsm
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ CommonDialog.prototype = {
if (isOSX && !(this.ui.infoRow && this.ui.infoRow.hidden)) {
this.ui.infoBody.focus();
} else {
button.focus({ preventFocusRing: true });
button.focus({ focusVisible: false });
}
} else if (this.args.promptType == "promptPassword") {
// When the prompt is initialized, focus and select the textbox
Expand Down

0 comments on commit c84f2d6

Please sign in to comment.