Bug 1036966 - Make accessibility.tabfocus default to 7 on macOS too. r=morgan,settings-reviewers,mac-reviewers,mstange

Stop supporting following the system preference, but keep macOS users
able to switch to just text controls (accessibility.tabfocus=1) in the
settings.

Change the meaning of the "Use the tab key to move focus between form
controls and links" checkbox in the Firefox settings, which was
introduced in bug 1628476 to override the system setting.

The intention, I think was that this checkbox being off resulted in
"follow the system" behavior, but that didn't quite happen due to a bug
in the preferences code (this[1] won't unset the pref, because of
this[2], which means we'll just return 0).

This patch changes it so that the checkbox instead always ignores the
system setting. There will no longer be a Firefox setting (neither in
the UI nor on about:config) that means "follow system setting".

This allows us to somewhat simplify the approach compared to the
previous patch in D196110, and keep the accessibility.tabfocus working
as the source of truth without a migration.

In the future, we can think of migrating accessibility.tabfocus to a
boolean pref, which would allow us to do the cleanups to the preferences
code that D196110 did.

[1]: https://searchfox.org/mozilla-central/rev/f1532761de0b60337e42c6c3f525288a523dabef/browser/components/preferences/main.js#2252
[2]: https://searchfox.org/mozilla-central/rev/f1532761de0b60337e42c6c3f525288a523dabef/toolkit/content/preferencesBindings.js#450,483

Differential Revision: https://phabricator.services.mozilla.com/D208602

browser/components/preferences/main.js

@@ -2247,9 +2247,8 @@ var gMainPane = {
       // 1/2/4 values set via about:config should persist
       return this._storedFullKeyboardNavigation;
     }
-    // When the checkbox is unchecked, this pref shouldn't exist
-    // at all.
-    return undefined;
+    // When the checkbox is unchecked, default to just text controls.
+    return 1;
   },
 
   /**

browser/components/preferences/tests/browser_keyboardfocus.js

@@ -13,40 +13,42 @@ add_task(async function () {
   let checkbox = gBrowser.contentDocument.querySelector(
     "#useFullKeyboardNavigation"
   );
-  Assert.ok(
-    !Services.prefs.getIntPref("accessibility.tabfocus", undefined),
-    "no pref value should exist"
+  Assert.equal(
+    Services.prefs.getIntPref("accessibility.tabfocus"),
+    7,
+    "default should be full keyboard access"
   );
   Assert.ok(
-    !checkbox.checked,
-    "checkbox should be unchecked before clicking on checkbox"
+    checkbox.checked,
+    "checkbox should be checked before clicking on checkbox"
   );
 
   checkbox.click();
 
   Assert.equal(
     Services.prefs.getIntPref("accessibility.tabfocus"),
-    7,
+    1,
     "Prefstore should reflect checkbox's associated numeric value"
   );
   Assert.ok(
-    checkbox.checked,
-    "checkbox should be checked after clicking on checkbox"
+    !checkbox.checked,
+    "checkbox should be unchecked after clicking on checkbox"
   );
 
   checkbox.click();
 
   Assert.ok(
-    !checkbox.checked,
-    "checkbox should be unchecked after clicking on checkbox"
+    checkbox.checked,
+    "checkbox should be checked after clicking on checkbox"
   );
-  Assert.ok(
-    !Services.prefs.getIntPref("accessibility.tabfocus", undefined),
-    "No pref value should exist"
+  Assert.equal(
+    Services.prefs.getIntPref("accessibility.tabfocus"),
+    7,
+    "Should restore default value"
   );
 
   BrowserTestUtils.removeTab(gBrowser.selectedTab);
-  SpecialPowers.pushPrefEnv({ set: [["accessibility.tabfocus", 4]] });
+  await SpecialPowers.pushPrefEnv({ set: [["accessibility.tabfocus", 4]] });
   await launchPreferences();
   checkbox = gBrowser.contentDocument.querySelector(
     "#useFullKeyboardNavigation"
@@ -57,20 +59,20 @@ add_task(async function () {
     "checkbox should stay unchecked after setting non-7 pref value"
   );
   Assert.equal(
-    Services.prefs.getIntPref("accessibility.tabfocus", 0),
+    Services.prefs.getIntPref("accessibility.tabfocus"),
     4,
     "pref should have value in store"
   );
 
   BrowserTestUtils.removeTab(gBrowser.selectedTab);
-  SpecialPowers.pushPrefEnv({ set: [["accessibility.tabfocus", 7]] });
+  await SpecialPowers.pushPrefEnv({ set: [["accessibility.tabfocus", 7]] });
   await launchPreferences();
   checkbox = gBrowser.contentDocument.querySelector(
     "#useFullKeyboardNavigation"
   );
 
   Assert.equal(
-    Services.prefs.getIntPref("accessibility.tabfocus", 0),
+    Services.prefs.getIntPref("accessibility.tabfocus"),
     7,
     "Pref value should update after modification"
   );

dom/base/FragmentOrElement.cpp

@@ -105,8 +105,6 @@
 using namespace mozilla;
 using namespace mozilla::dom;
 
-int32_t nsIContent::sTabFocusModel = eTabFocus_any;
-bool nsIContent::sTabFocusModelAppliesToXUL = false;
 uint64_t nsMutationGuard::sGeneration = 0;
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(nsIContent)

dom/base/TabFocusModel.h

@@ -0,0 +1,32 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef mozilla_TabFocusModel_h
+#define mozilla_TabFocusModel_h
+
+#include "mozilla/StaticPrefs_accessibility.h"
+
+namespace mozilla {
+
+enum class TabFocusableType : uint8_t {
+  TextControls = 1,       // Textboxes and lists always tabbable
+  FormElements = 1 << 1,  // Non-text form elements
+  Links = 1 << 2,         // Links
+  Any = TextControls | FormElements | Links,
+};
+
+class TabFocusModel final {
+ public:
+  static bool AppliesToXUL() {
+    return StaticPrefs::accessibility_tabfocus_applies_to_xul();
+  }
+
+  static bool IsTabFocusable(TabFocusableType aType) {
+    return StaticPrefs::accessibility_tabfocus() & int32_t(aType);
+  }
+};
+
+}  // namespace mozilla
+
+#endif

dom/base/moz.build

@@ -135,6 +135,7 @@ EXPORTS.mozilla += [
     "ScriptableContentIterator.h",
     "ScrollingMetrics.h",
     "SelectionChangeEventDispatcher.h",
+    "TabFocusModel.h",
     "TextInputProcessor.h",
     "UseCounter.h",
 ]

dom/base/nsFocusManager.cpp

@@ -173,7 +173,6 @@ bool nsFocusManager::sTestMode = false;
 uint64_t nsFocusManager::sFocusActionCounter = 0;
 
 static const char* kObservedPrefs[] = {"accessibility.browsewithcaret",
-                                       "accessibility.tabfocus_applies_to_xul",
                                        "focusmanager.testmode", nullptr};
 
 nsFocusManager::nsFocusManager()
@@ -198,10 +197,6 @@ nsFocusManager::~nsFocusManager() {
 nsresult nsFocusManager::Init() {
   sInstance = new nsFocusManager();
 
-  nsIContent::sTabFocusModelAppliesToXUL =
-      Preferences::GetBool("accessibility.tabfocus_applies_to_xul",
-                           nsIContent::sTabFocusModelAppliesToXUL);
-
   sTestMode = Preferences::GetBool("focusmanager.testmode", false);
 
   Preferences::RegisterCallbacks(nsFocusManager::PrefChanged, kObservedPrefs,
@@ -229,10 +224,6 @@ void nsFocusManager::PrefChanged(const char* aPref) {
   nsDependentCString pref(aPref);
   if (pref.EqualsLiteral("accessibility.browsewithcaret")) {
     UpdateCaretForCaretBrowsingMode();
-  } else if (pref.EqualsLiteral("accessibility.tabfocus_applies_to_xul")) {
-    nsIContent::sTabFocusModelAppliesToXUL =
-        Preferences::GetBool("accessibility.tabfocus_applies_to_xul",
-                             nsIContent::sTabFocusModelAppliesToXUL);
   } else if (pref.EqualsLiteral("focusmanager.testmode")) {
     sTestMode = Preferences::GetBool("focusmanager.testmode", false);
   }
@@ -3362,9 +3353,6 @@ nsresult nsFocusManager::DetermineElementToMoveFocus(
     doc = aWindow->GetExtantDoc();
   if (!doc) return NS_OK;
 
-  LookAndFeel::GetInt(LookAndFeel::IntID::TabFocusModel,
-                      &nsIContent::sTabFocusModel);
-
   // True if we are navigating by document (F6/Shift+F6) or false if we are
   // navigating by element (Tab/Shift+Tab).
   const bool forDocumentNavigation =

dom/base/nsIContent.h

@@ -753,21 +753,6 @@ class nsIContent : public nsINode {
   virtual void DumpContent(FILE* out = stdout, int32_t aIndent = 0,
                            bool aDumpAll = true) const = 0;
 #endif
-
-  enum ETabFocusType {
-    eTabFocus_textControlsMask =
-        (1 << 0),  // textboxes and lists always tabbable
-    eTabFocus_formElementsMask = (1 << 1),   // non-text form elements
-    eTabFocus_linksMask = (1 << 2),          // links
-    eTabFocus_any = 1 + (1 << 1) + (1 << 2)  // everything that can be focused
-  };
-
-  // Tab focus model bit field:
-  static int32_t sTabFocusModel;
-
-  // accessibility.tabfocus_applies_to_xul pref - if it is set to true,
-  // the tabfocus bit field applies to xul elements.
-  static bool sTabFocusModelAppliesToXUL;
 };
 
 NON_VIRTUAL_ADDREF_RELEASE(nsIContent)

dom/html/HTMLAnchorElement.cpp

@@ -14,7 +14,7 @@
 #include "nsCOMPtr.h"
 #include "nsContentUtils.h"
 #include "nsGkAtoms.h"
-#include "nsAttrValueOrString.h"
+#include "mozilla/TabFocusModel.h"
 #include "mozilla/dom/Document.h"
 #include "nsPresContext.h"
 #include "nsIURI.h"
@@ -119,7 +119,7 @@ bool HTMLAnchorElement::IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
     }
   }
 
-  if ((sTabFocusModel & eTabFocus_linksMask) == 0) {
+  if (!TabFocusModel::IsTabFocusable(TabFocusableType::Links)) {
     *aTabIndex = -1;
   }
   *aIsFocusable = true;

dom/html/HTMLImageElement.cpp

@@ -6,6 +6,7 @@
 
 #include "mozilla/dom/HTMLImageElement.h"
 #include "mozilla/PresShell.h"
+#include "mozilla/TabFocusModel.h"
 #include "mozilla/dom/BindContext.h"
 #include "mozilla/dom/BindingUtils.h"
 #include "mozilla/dom/HTMLImageElementBinding.h"
@@ -494,7 +495,8 @@ bool HTMLImageElement::IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
 
   if (IsInComposedDoc() && FindImageMap()) {
     // Use tab index on individual map areas.
-    *aTabIndex = (sTabFocusModel & eTabFocus_linksMask) ? 0 : -1;
+    *aTabIndex =
+        TabFocusModel::IsTabFocusable(TabFocusableType::Links) ? 0 : -1;
     // Image map is not focusable itself, but flag as tabbable
     // so that image map areas get walked into.
     *aIsFocusable = false;
@@ -502,7 +504,9 @@ bool HTMLImageElement::IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
   }
 
   // Can be in tab order if tabindex >=0 and form controls are tabbable.
-  *aTabIndex = (sTabFocusModel & eTabFocus_formElementsMask) ? tabIndex : -1;
+  *aTabIndex = TabFocusModel::IsTabFocusable(TabFocusableType::FormElements)
+                   ? tabIndex
+                   : -1;
   *aIsFocusable = IsFormControlDefaultFocusable(aWithMouse) &&
                   (tabIndex >= 0 || GetTabIndexAttrValue().isSome());
 

dom/mathml/MathMLElement.cpp

@@ -7,10 +7,10 @@
 #include "mozilla/dom/MathMLElement.h"
 
 #include "base/compiler_specific.h"
+#include "mozilla/TabFocusModel.h"
 #include "mozilla/dom/BindContext.h"
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/EventListenerManager.h"
-#include "mozilla/FontPropertyTypes.h"
 #include "mozilla/StaticPrefs_mathml.h"
 #include "mozilla/TextUtils.h"
 #include "nsGkAtoms.h"
@@ -20,7 +20,6 @@
 #include "nsStyleConsts.h"
 #include "mozilla/dom/Document.h"
 #include "nsPresContext.h"
-#include "mozAutoDocUpdate.h"
 #include "nsIScriptError.h"
 #include "nsContentUtils.h"
 #include "nsIURI.h"
@@ -637,7 +636,7 @@ Focusable MathMLElement::IsFocusableWithoutStyle(bool aWithMouse) {
     return {};
   }
 
-  if ((sTabFocusModel & eTabFocus_linksMask) == 0) {
+  if (!TabFocusModel::IsTabFocusable(TabFocusableType::Links)) {
     tabIndex = -1;
   }
 

dom/svg/SVGAElement.cpp

@@ -6,11 +6,10 @@
 
 #include "mozilla/dom/SVGAElement.h"
 
-#include "mozilla/Attributes.h"
 #include "mozilla/EventDispatcher.h"
-#include "mozilla/dom/BindContext.h"
 #include "mozilla/dom/DocumentInlines.h"
 #include "mozilla/dom/SVGAElementBinding.h"
+#include "mozilla/TabFocusModel.h"
 #include "nsCOMPtr.h"
 #include "nsContentUtils.h"
 #include "nsGkAtoms.h"
@@ -182,7 +181,7 @@ Focusable SVGAElement::IsFocusableWithoutStyle(bool aWithMouse) {
       return {};
     }
   }
-  if ((sTabFocusModel & eTabFocus_linksMask) == 0) {
+  if (!TabFocusModel::IsTabFocusable(TabFocusableType::Links)) {
     result.mTabIndex = -1;
   }
   return result;

dom/xul/nsXULElement.cpp

@@ -50,6 +50,7 @@
 #include "mozilla/StaticAnalysisFunctions.h"
 #include "mozilla/StaticPrefs_javascript.h"
 #include "mozilla/StaticPtr.h"
+#include "mozilla/TabFocusModel.h"
 #include "mozilla/TaskController.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/URLExtraData.h"
@@ -402,15 +403,12 @@ nsXULElement::XULFocusability nsXULElement::GetXULFocusability(
     result.mForcedFocusable.emplace(true);
     result.mForcedTabIndexIfFocusable.emplace(attrVal.value());
   }
-  if (xulControl && sTabFocusModelAppliesToXUL &&
-      !(sTabFocusModel & eTabFocus_formElementsMask) && IsNonList(mNodeInfo)) {
+  if (xulControl && TabFocusModel::AppliesToXUL() &&
+      !TabFocusModel::IsTabFocusable(TabFocusableType::FormElements) &&
+      IsNonList(mNodeInfo)) {
     // By default, the tab focus model doesn't apply to xul element on any
-    // system but OS X. on OS X we're following it for UI elements (XUL) as
-    // sTabFocusModel is based on "Full Keyboard Access" system setting (see
-    // mac/nsILookAndFeel). both textboxes and list elements (i.e. trees and
-    // list) should always be focusable (textboxes are handled as html:input)
-    // For compatibility, we only do this for controls, otherwise elements
-    // like <browser> cannot take this focus.
+    // system but OS X. For compatibility, we only do this for controls,
+    // otherwise elements like <browser> cannot take this focus.
     result.mForcedTabIndexIfFocusable = Some(-1);
   }
   return result;

modules/libpref/init/StaticPrefList.yaml

@@ -200,6 +200,22 @@
 # Prefs starting with "accessibility."
 #---------------------------------------------------------------------------
 
+# Tab focus model bit field:
+# 1 focuses text controls, 2 focuses other form elements, 4 adds links.
+# Most users will want 1, 3, or 7. On macOS we expose a checkbox to alter
+# between 7 and 3.
+- name: accessibility.tabfocus
+  type: int32_t
+  value: 7
+  mirror: always
+
+# Only on mac tabfocus is expected to handle UI widgets as well as web content.
+# FIXME(emilio): This is weird now that we have a lot of HTML in our pages.
+- name: accessibility.tabfocus_applies_to_xul
+  type: bool
+  value: @IS_XP_MACOSX@
+  mirror: always
+
 - name: accessibility.accesskeycausesactivation
   type: bool
   value: true

modules/libpref/init/all.js

@@ -472,20 +472,6 @@ pref("accessibility.warn_on_browsewithcaret", true);
 
 pref("accessibility.browsewithcaret_shortcut.enabled", true);
 
-#ifndef XP_MACOSX
-  // Tab focus model bit field:
-  // 1 focuses text controls, 2 focuses other form elements, 4 adds links.
-  // Most users will want 1, 3, or 7.
-  // On OS X, we use Full Keyboard Access system preference,
-  // unless accessibility.tabfocus is set by the user.
-  pref("accessibility.tabfocus", 7);
-  pref("accessibility.tabfocus_applies_to_xul", false);
-#else
-  // Only on mac tabfocus is expected to handle UI widgets as well as web
-  // content.
-  pref("accessibility.tabfocus_applies_to_xul", true);
-#endif
-
 // We follow the "Click in the scrollbar to:" system preference on OS X and
 // "gtk-primary-button-warps-slider" property with GTK (since 2.24 / 3.6),
 // unless this preference is explicitly set.

widget/LookAndFeel.h

@@ -92,8 +92,6 @@ class LookAndFeel {
     TreeScrollDelay,
     // the maximum number of lines to be scrolled at ones
     TreeScrollLinesMax,
-    // What type of tab-order to use
-    TabFocusModel,
     // Should menu items blink when they're chosen?
     ChosenMenuItemsShouldBlink,
 

widget/cocoa/nsLookAndFeel.mm

@@ -459,11 +459,6 @@ nsresult nsLookAndFeel::NativeGetInt(IntID aID, int32_t& aResult) {
     case IntID::AlertNotificationOrigin:
       aResult = NS_ALERT_TOP;
       break;
-    case IntID::TabFocusModel:
-      aResult = [NSApp isFullKeyboardAccessEnabled]
-                    ? nsIContent::eTabFocus_any
-                    : nsIContent::eTabFocus_textControlsMask;
-      break;
     case IntID::ScrollToClick: {
       aResult = [[NSUserDefaults standardUserDefaults]
           boolForKey:@"AppleScrollerPagingBehavior"];

widget/headless/HeadlessLookAndFeelGTK.cpp

@@ -84,9 +84,6 @@ nsresult HeadlessLookAndFeel::NativeGetInt(IntID aID, int32_t& aResult) {
     case IntID::TreeScrollLinesMax:
       aResult = 3;
       break;
-    case IntID::TabFocusModel:
-      aResult = nsIContent::eTabFocus_textControlsMask;
-      break;
     case IntID::ChosenMenuItemsShouldBlink:
       aResult = 1;
       break;

widget/nsXPLookAndFeel.cpp

@@ -146,7 +146,6 @@ static const char sIntPrefs[][45] = {
     "ui.treeLazyScrollDelay",
     "ui.treeScrollDelay",
     "ui.treeScrollLinesMax",
-    "accessibility.tabfocus",  // Weird one...
     "ui.chosenMenuItemsShouldBlink",
     "ui.windowsAccentColorInTitlebar",
     "ui.macBigSurTheme",
@@ -527,9 +526,6 @@ void nsXPLookAndFeel::Init() {
   //     for each types.  Then, we could reduce the unnecessary loop from
   //     nsXPLookAndFeel::OnPrefChanged().
   Preferences::RegisterPrefixCallback(OnPrefChanged, "ui.");
-  // We really do just want the accessibility.tabfocus pref, not other prefs
-  // that start with that string.
-  Preferences::RegisterCallback(OnPrefChanged, "accessibility.tabfocus");
 
   for (const auto& pref : kMediaQueryPrefs) {
     Preferences::RegisterCallback(

widget/uikit/nsLookAndFeel.mm

@@ -282,9 +282,6 @@ nsLookAndFeel::NativeGetInt(IntID aID, int32_t& aResult) {
     case IntID::TreeScrollLinesMax:
       aResult = 3;
       break;
-    case IntID::TabFocusModel:
-      aResult = 1;  // default to just textboxes
-      break;
     case IntID::ScrollToClick:
       aResult = 0;
       break;