Bug 1807687 - Simplify Windows keyboard indicator setting handling. r…

…=NeilDeakin The only thing that can explain this is the WM_UPDATEUISTATE state getting out of sync in a way that we think we need to unconditionally show focus indicators for a window. I tried to first make this less error prone (see patch above) but digging more into these messages, I'm pretty sure we just don't need all this code. See: * https://devblogs.microsoft.com/oldnewthing/20130516-00/?p=4343 * https://devblogs.microsoft.com/oldnewthing/20130517-00/?p=4323 In particular, this is intended to be a windows feature to not show keyboard indicators on dialogs until you use the keyboard. But that's how Gecko dialogs behave already due to how :focus-visible behaves as per: https://searchfox.org/mozilla-central/rev/43ee5e789b079e94837a21336e9ce2420658fd19/toolkit/components/prompts/src/CommonDialog.jsm#319 I haven't been able to repro this state, but sounds believable that it could happen after opening a native dialog or so on? The purpose of this code is to implement the 'Underline access keys' in the Keyboard Accessibility control panel of windows. There's an easier way of tracking this, via the SPI_GETKEYBOARDCUES SPI, documented in: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-systemparametersinfoa Hook that into LookAndFeel rather than using custom IPC and so on. Differential Revision: https://phabricator.services.mozilla.com/D165578
dothq · Jan 9, 2023 · 56be508 · 56be508
1 parent 5fa5bdb
commit 56be508
Show file tree

Hide file tree

Showing 25 changed files with 40 additions and 250 deletions.
diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp
@@ -7779,23 +7779,6 @@ void nsContentUtils::CallOnAllRemoteChildren(
   }
 }
 
-struct UIStateChangeInfo {
-  UIStateChangeType mShowFocusRings;
-
-  explicit UIStateChangeInfo(UIStateChangeType aShowFocusRings)
-      : mShowFocusRings(aShowFocusRings) {}
-};
-
-void nsContentUtils::SetKeyboardIndicatorsOnRemoteChildren(
-    nsPIDOMWindowOuter* aWindow, UIStateChangeType aShowFocusRings) {
-  UIStateChangeInfo stateInfo(aShowFocusRings);
-  CallOnAllRemoteChildren(aWindow, [&stateInfo](BrowserParent* aBrowserParent) {
-    Unused << aBrowserParent->SendSetKeyboardIndicators(
-        stateInfo.mShowFocusRings);
-    return CallState::Continue;
-  });
-}
-
 bool nsContentUtils::IPCDataTransferItemHasKnownFlavor(
     const IPCDataTransferItem& aItem) {
   // Unknown types are converted to kCustomTypesMime.

diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h
@@ -2842,14 +2842,6 @@ class nsContentUtils {
       const std::function<mozilla::CallState(mozilla::dom::BrowserParent*)>&
           aCallback);
 
-  /*
-   * Call nsPIDOMWindow::SetKeyboardIndicators all all remote children. This is
-   * in here rather than nsGlobalWindow because BrowserParent indirectly
-   * includes Windows headers which aren't allowed there.
-   */
-  static void SetKeyboardIndicatorsOnRemoteChildren(
-      nsPIDOMWindowOuter* aWindow, UIStateChangeType aShowFocusRings);
-
   /**
    * Given an IPCDataTransferImageContainer construct an imgIContainer for the
    * image encoded by the transfer item.

diff --git a/dom/base/nsFrameLoader.cpp b/dom/base/nsFrameLoader.cpp
@@ -3624,27 +3624,21 @@ nsresult nsFrameLoader::GetNewTabContext(MutableTabContext* aTabContext,
 
   MOZ_ASSERT(mPendingBrowsingContext->EverAttached());
 
-  UIStateChangeType showFocusRings = UIStateChangeType_NoChange;
   uint64_t chromeOuterWindowID = 0;
 
-  Document* doc = mOwnerContent->OwnerDoc();
-  if (doc) {
-    nsCOMPtr<nsPIWindowRoot> root = nsContentUtils::GetWindowRoot(doc);
-    if (root) {
-      showFocusRings = root->ShowFocusRings() ? UIStateChangeType_Set
-                                              : UIStateChangeType_Clear;
-
-      nsPIDOMWindowOuter* outerWin = root->GetWindow();
-      if (outerWin) {
-        chromeOuterWindowID = outerWin->WindowID();
-      }
+  nsCOMPtr<nsPIWindowRoot> root =
+      nsContentUtils::GetWindowRoot(mOwnerContent->OwnerDoc());
+  if (root) {
+    nsPIDOMWindowOuter* outerWin = root->GetWindow();
+    if (outerWin) {
+      chromeOuterWindowID = outerWin->WindowID();
     }
   }
 
   uint32_t maxTouchPoints = BrowserParent::GetMaxTouchPoints(mOwnerContent);
 
-  bool tabContextUpdated = aTabContext->SetTabContext(
-      chromeOuterWindowID, showFocusRings, maxTouchPoints);
+  bool tabContextUpdated =
+      aTabContext->SetTabContext(chromeOuterWindowID, maxTouchPoints);
   NS_ENSURE_STATE(tabContextUpdated);
 
   return NS_OK;

diff --git a/dom/base/nsGlobalWindowInner.cpp b/dom/base/nsGlobalWindowInner.cpp
@@ -60,6 +60,7 @@
 #include "mozilla/FlushType.h"
 #include "mozilla/Likely.h"
 #include "mozilla/LinkedList.h"
+#include "mozilla/LookAndFeel.h"
 #include "mozilla/Logging.h"
 #include "mozilla/MacroForEach.h"
 #include "mozilla/Maybe.h"
@@ -4614,9 +4615,13 @@ bool nsGlobalWindowInner::ShouldShowFocusRing() {
       StaticPrefs::browser_display_always_show_rings_after_key_focus()) {
     return true;
   }
-
-  nsCOMPtr<nsPIWindowRoot> root = GetTopWindowRoot();
-  return root && root->ShowFocusRings();
+  if (StaticPrefs::browser_display_show_focus_rings()) {
+    return true;
+  }
+  if (LookAndFeel::GetInt(LookAndFeel::IntID::ShowKeyboardCues)) {
+    return true;
+  }
+  return false;
 }
 
 bool nsGlobalWindowInner::TakeFocus(bool aFocus, uint32_t aFocusMethod) {

diff --git a/dom/base/nsGlobalWindowInner.h b/dom/base/nsGlobalWindowInner.h
@@ -1210,8 +1210,6 @@ class nsGlobalWindowInner final : public mozilla::dom::EventTarget,
 
   virtual void UpdateParentTarget() override;
 
-  void InitializeShowFocusRings();
-
   // Clear the document-dependent slots on our JS wrapper.  Inner windows only.
   void ClearDocumentDependentSlots(JSContext* aCx);
 

diff --git a/dom/base/nsGlobalWindowOuter.cpp b/dom/base/nsGlobalWindowOuter.cpp
@@ -3015,30 +3015,6 @@ nsIPrincipal* nsGlobalWindowOuter::PartitionedPrincipal() {
 // nsGlobalWindowOuter::nsIDOMWindow
 //*****************************************************************************
 
-void nsPIDOMWindowOuter::SetInitialKeyboardIndicators(
-    UIStateChangeType aShowFocusRings) {
-  MOZ_ASSERT(!GetCurrentInnerWindow());
-
-  nsPIDOMWindowOuter* piWin = GetPrivateRoot();
-  if (!piWin) {
-    return;
-  }
-
-  MOZ_ASSERT(piWin == this);
-
-  // only change the flags that have been modified
-  nsCOMPtr<nsPIWindowRoot> windowRoot = do_QueryInterface(mChromeEventHandler);
-  if (!windowRoot) {
-    return;
-  }
-
-  if (aShowFocusRings != UIStateChangeType_NoChange) {
-    windowRoot->SetShowFocusRings(aShowFocusRings == UIStateChangeType_Set);
-  }
-
-  nsContentUtils::SetKeyboardIndicatorsOnRemoteChildren(this, aShowFocusRings);
-}
-
 Element* nsPIDOMWindowOuter::GetFrameElementInternal() const {
   return mFrameElement;
 }
@@ -6701,42 +6677,6 @@ bool nsGlobalWindowOuter::ShouldShowFocusRing() {
   FORWARD_TO_INNER(ShouldShowFocusRing, (), false);
 }
 
-void nsGlobalWindowOuter::SetKeyboardIndicators(
-    UIStateChangeType aShowFocusRings) {
-  nsPIDOMWindowOuter* piWin = GetPrivateRoot();
-  if (!piWin) {
-    return;
-  }
-
-  MOZ_ASSERT(piWin == this);
-
-  bool oldShouldShowFocusRing = ShouldShowFocusRing();
-
-  // only change the flags that have been modified
-  nsCOMPtr<nsPIWindowRoot> windowRoot = do_QueryInterface(mChromeEventHandler);
-  if (!windowRoot) {
-    return;
-  }
-
-  if (aShowFocusRings != UIStateChangeType_NoChange) {
-    windowRoot->SetShowFocusRings(aShowFocusRings == UIStateChangeType_Set);
-  }
-
-  nsContentUtils::SetKeyboardIndicatorsOnRemoteChildren(this, aShowFocusRings);
-
-  bool newShouldShowFocusRing = ShouldShowFocusRing();
-  if (mInnerWindow && nsGlobalWindowInner::Cast(mInnerWindow)->mHasFocus &&
-      mInnerWindow->mFocusedElement &&
-      oldShouldShowFocusRing != newShouldShowFocusRing) {
-    // Update focusedNode's state.
-    if (newShouldShowFocusRing) {
-      mInnerWindow->mFocusedElement->AddStates(ElementState::FOCUSRING);
-    } else {
-      mInnerWindow->mFocusedElement->RemoveStates(ElementState::FOCUSRING);
-    }
-  }
-}
-
 bool nsGlobalWindowOuter::TakeFocus(bool aFocus, uint32_t aFocusMethod) {
   FORWARD_TO_INNER(TakeFocus, (aFocus, aFocusMethod), false);
 }

diff --git a/dom/base/nsGlobalWindowOuter.h b/dom/base/nsGlobalWindowOuter.h
@@ -896,8 +896,6 @@ class nsGlobalWindowOuter final : public mozilla::dom::EventTarget,
 
   bool ShouldShowFocusRing() override;
 
-  void SetKeyboardIndicators(UIStateChangeType aShowFocusRings) override;
-
  public:
   already_AddRefed<nsPIWindowRoot> GetTopWindowRoot() override;
 
@@ -908,8 +906,6 @@ class nsGlobalWindowOuter final : public mozilla::dom::EventTarget,
 
   void UpdateParentTarget() override;
 
-  void InitializeShowFocusRings();
-
  protected:
   // Helper for getComputedStyle and getDefaultComputedStyle
   already_AddRefed<nsICSSDeclaration> GetComputedStyleHelperOuter(

diff --git a/dom/base/nsPIDOMWindow.h b/dom/base/nsPIDOMWindow.h
@@ -76,13 +76,6 @@ class CustomElementRegistry;
 enum class CallerType : uint32_t;
 }  // namespace mozilla::dom
 
-enum UIStateChangeType {
-  UIStateChangeType_NoChange,
-  UIStateChangeType_Set,
-  UIStateChangeType_Clear,
-  UIStateChangeType_Invalid  // used for serialization only
-};
-
 enum class FullscreenReason {
   // Toggling the fullscreen mode requires trusted context.
   ForFullscreenMode,
@@ -792,11 +785,6 @@ class nsPIDOMWindowOuter : public mozIDOMWindowProxy {
 
   bool IsRootOuterWindow() { return mIsRootOuterWindow; }
 
-  /**
-   * Set initial keyboard indicator state for focus rings.
-   */
-  void SetInitialKeyboardIndicators(UIStateChangeType aShowFocusRings);
-
   // Internal getter/setter for the frame element, this version of the
   // getter crosses chrome boundaries whereas the public scriptable
   // one doesn't for security reasons.
@@ -1027,11 +1015,6 @@ class nsPIDOMWindowOuter : public mozIDOMWindowProxy {
    */
   virtual bool ShouldShowFocusRing() = 0;
 
-  /**
-   * Set the keyboard indicator state for accelerators and focus rings.
-   */
-  virtual void SetKeyboardIndicators(UIStateChangeType aShowFocusRings) = 0;
-
   /**
    * Indicates that the page in the window has been hidden. This is used to
    * reset the focus state.

diff --git a/dom/base/nsPIWindowRoot.h b/dom/base/nsPIWindowRoot.h
@@ -67,9 +67,6 @@ class nsPIWindowRoot : public mozilla::dom::EventTarget {
 
   // Enumerate all stored browsers that for which the weak reference is valid.
   virtual void EnumerateBrowsers(BrowserEnumerator aEnumFunc, void* aArg) = 0;
-
-  virtual bool ShowFocusRings() = 0;
-  virtual void SetShowFocusRings(bool aEnable) = 0;
 };
 
 namespace mozilla::dom {

diff --git a/dom/base/nsWindowRoot.cpp b/dom/base/nsWindowRoot.cpp
@@ -39,10 +39,7 @@
 using namespace mozilla;
 using namespace mozilla::dom;
 
-nsWindowRoot::nsWindowRoot(nsPIDOMWindowOuter* aWindow) {
-  mWindow = aWindow;
-  mShowFocusRings = StaticPrefs::browser_display_show_focus_rings();
-}
+nsWindowRoot::nsWindowRoot(nsPIDOMWindowOuter* aWindow) { mWindow = aWindow; }
 
 nsWindowRoot::~nsWindowRoot() {
   if (mListenerManager) {

diff --git a/dom/base/nsWindowRoot.h b/dom/base/nsWindowRoot.h
@@ -70,10 +70,6 @@ class nsWindowRoot final : public nsPIWindowRoot {
   void RemoveBrowser(nsIRemoteTab* aBrowser) override;
   void EnumerateBrowsers(BrowserEnumerator aEnumFunc, void* aArg) override;
 
-  bool ShowFocusRings() override { return mShowFocusRings; }
-
-  void SetShowFocusRings(bool aEnable) override { mShowFocusRings = aEnable; }
-
  protected:
   virtual ~nsWindowRoot();
 
@@ -88,9 +84,6 @@ class nsWindowRoot final : public nsPIWindowRoot {
   RefPtr<mozilla::EventListenerManager> mListenerManager;  // [Strong]
   nsWeakPtr mPopupNode;
 
-  // True if focus rings are enabled for this window hierarchy.
-  bool mShowFocusRings;
-
   nsCOMPtr<mozilla::dom::EventTarget> mParent;
 
   // The BrowserParents that are currently registered with this top-level

diff --git a/dom/ipc/BrowserChild.cpp b/dom/ipc/BrowserChild.cpp
@@ -481,13 +481,6 @@ nsresult BrowserChild::Init(mozIDOMWindowProxy* aParent,
   nsCOMPtr<EventTarget> chromeHandler = window->GetChromeEventHandler();
   docShell->SetChromeEventHandler(chromeHandler);
 
-  if (window->GetCurrentInnerWindow()) {
-    window->SetKeyboardIndicators(ShowFocusRings());
-  } else {
-    // Skip ShouldShowFocusRing check if no inner window is available
-    window->SetInitialKeyboardIndicators(ShowFocusRings());
-  }
-
   // Window scrollbar flags only affect top level remote frames, not fission
   // frames.
   if (mIsTopLevel) {
@@ -1456,14 +1449,6 @@ mozilla::ipc::IPCResult BrowserChild::RecvDeactivate(uint64_t aActionId) {
   return IPC_OK();
 }
 
-mozilla::ipc::IPCResult BrowserChild::RecvSetKeyboardIndicators(
-    const UIStateChangeType& aShowFocusRings) {
-  nsCOMPtr<nsPIDOMWindowOuter> window = do_GetInterface(WebNavigation());
-  NS_ENSURE_TRUE(window, IPC_OK());
-  window->SetKeyboardIndicators(aShowFocusRings);
-  return IPC_OK();
-}
-
 mozilla::ipc::IPCResult BrowserChild::RecvStopIMEStateManagement() {
   IMEStateManager::StopIMEStateManagement();
   return IPC_OK();

diff --git a/dom/ipc/BrowserChild.h b/dom/ipc/BrowserChild.h
@@ -702,9 +702,6 @@ class BrowserChild final : public nsMessageManagerScriptExecutor,
 
   mozilla::ipc::IPCResult RecvScrollbarPreferenceChanged(ScrollbarPreference);
 
-  mozilla::ipc::IPCResult RecvSetKeyboardIndicators(
-      const UIStateChangeType& aShowFocusRings);
-
   mozilla::ipc::IPCResult RecvStopIMEStateManagement();
 
   mozilla::ipc::IPCResult RecvAllowScriptsToClose();

diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
@@ -708,8 +708,6 @@ child:
 
     async DynamicToolbarOffsetChanged(ScreenIntCoord height);
 
-    async SetKeyboardIndicators(UIStateChangeType showFocusRings);
-
     /**
      * StopIMEStateManagement() is called when the process loses focus and
      * should stop managing IME state.

diff --git a/dom/ipc/PTabContext.ipdlh b/dom/ipc/PTabContext.ipdlh
@@ -8,7 +8,6 @@ include "mozilla/dom/TabMessageUtils.h";
 
 include protocol PBrowser;
 
-using UIStateChangeType from "nsPIDOMWindow.h";
 using mozilla::OriginAttributes from "mozilla/ipc/BackgroundUtils.h";
 
 namespace mozilla {
@@ -27,9 +26,6 @@ struct FrameIPCTabContext
 {
   uint64_t chromeOuterWindowID;
 
-  // Keyboard indicator state inherited from the parent.
-  UIStateChangeType showFocusRings;
-
   // Maximum number of touch points on the screen.
   uint32_t maxTouchPoints;
 };