From 40ee61069dcc360cdb380d848dd44a47004595dd Mon Sep 17 00:00:00 2001 From: Sean Feng Date: Wed, 15 Feb 2023 20:08:37 +0000 Subject: [PATCH] Bug 1801761 - Generate focus actionId in SetFocusInner rather than relying on the caller to provide it r=emilio,hsivonen This fixes an issue where Element::Focus triggered another focus event and caused the initial focus event failed to be executed. Differential Revision: https://phabricator.services.mozilla.com/D163156 --- dom/base/nsFocusManager.cpp | 64 ++++++++++++++++++++----------------- dom/base/nsFocusManager.h | 12 ++++--- 2 files changed, 41 insertions(+), 35 deletions(-) diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp index 90b538473462..10d3d2ec14d3 100644 --- a/dom/base/nsFocusManager.cpp +++ b/dom/base/nsFocusManager.cpp @@ -380,8 +380,7 @@ nsFocusManager::GetActiveBrowsingContext(BrowsingContext** aBrowsingContext) { void nsFocusManager::FocusWindow(nsPIDOMWindowOuter* aWindow, CallerType aCallerType) { if (RefPtr fm = sInstance) { - fm->SetFocusedWindowWithCallerType(aWindow, aCallerType, - sInstance->GenerateFocusActionId()); + fm->SetFocusedWindowWithCallerType(aWindow, aCallerType); } } @@ -402,19 +401,19 @@ nsFocusManager::GetFocusedContentBrowsingContext( } nsresult nsFocusManager::SetFocusedWindowWithCallerType( - mozIDOMWindowProxy* aWindowToFocus, CallerType aCallerType, - uint64_t aActionId) { - LOGFOCUS(("<>", aActionId)); + mozIDOMWindowProxy* aWindowToFocus, CallerType aCallerType) { + LOGFOCUS(("<>")); nsCOMPtr windowToFocus = nsPIDOMWindowOuter::From(aWindowToFocus); NS_ENSURE_TRUE(windowToFocus, NS_ERROR_FAILURE); nsCOMPtr frameElement = windowToFocus->GetFrameElementInternal(); + Maybe actionIdFromSetFocusInner; if (frameElement) { // pass false for aFocusChanged so that the caret does not get updated // and scrolling does not occur. - SetFocusInner(frameElement, 0, false, true, aActionId); + actionIdFromSetFocusInner = SetFocusInner(frameElement, 0, false, true); } else { // this is a top-level window. If the window has a child frame focused, // clear the focus. Otherwise, focus should already be in this frame, or @@ -428,19 +427,21 @@ nsresult nsFocusManager::SetFocusedWindowWithCallerType( } nsCOMPtr rootWindow = windowToFocus->GetPrivateRoot(); + const uint64_t actionId = actionIdFromSetFocusInner.isSome() + ? actionIdFromSetFocusInner.value() + : sInstance->GenerateFocusActionId(); if (rootWindow) { - RaiseWindow(rootWindow, aCallerType, aActionId); + RaiseWindow(rootWindow, aCallerType, actionId); } - LOGFOCUS(("<>", aActionId)); + LOGFOCUS(("<>", actionId)); return NS_OK; } NS_IMETHODIMP nsFocusManager::SetFocusedWindow( mozIDOMWindowProxy* aWindowToFocus) { - return SetFocusedWindowWithCallerType(aWindowToFocus, CallerType::System, - GenerateFocusActionId()); + return SetFocusedWindowWithCallerType(aWindowToFocus, CallerType::System); } NS_IMETHODIMP @@ -470,7 +471,7 @@ nsFocusManager::SetFocus(Element* aElement, uint32_t aFlags) { NS_ENSURE_ARG(aElement); - SetFocusInner(aElement, aFlags, true, true, GenerateFocusActionId()); + SetFocusInner(aElement, aFlags, true, true); LOGFOCUS(("<>")); @@ -543,7 +544,7 @@ nsFocusManager::MoveFocus(mozIDOMWindowProxy* aWindow, Element* aStartElement, // would be a problem because the caret would move to the beginning of the // focused link making it impossible to navigate the caret over a link. SetFocusInner(MOZ_KnownLive(newFocus->AsElement()), aFlags, - aType != MOVEFOCUS_CARET, true, GenerateFocusActionId()); + aType != MOVEFOCUS_CARET, true); *aElement = do_AddRef(newFocus->AsElement()).take(); } else if (aType == MOVEFOCUS_ROOT || aType == MOVEFOCUS_CARET) { // no content was found, so clear the focus for these two types. @@ -1472,14 +1473,15 @@ static bool IsEmeddededInNoautofocusPopup(BrowsingContext& aBc) { .GetXULBoolAttr(nsGkAtoms::noautofocus); } -void nsFocusManager::SetFocusInner(Element* aNewContent, int32_t aFlags, - bool aFocusChanged, bool aAdjustWidget, - uint64_t aActionId) { +Maybe nsFocusManager::SetFocusInner(Element* aNewContent, + int32_t aFlags, + bool aFocusChanged, + bool aAdjustWidget) { // if the element is not focusable, just return and leave the focus as is RefPtr elementToFocus = FlushAndCheckIfFocusable(aNewContent, aFlags); if (!elementToFocus) { - return; + return Nothing(); } const RefPtr focusedBrowsingContext = @@ -1515,7 +1517,7 @@ void nsFocusManager::SetFocusInner(Element* aNewContent, int32_t aFlags, // focused rather than the frame it is in. if (!newWindow || (newBrowsingContext == GetFocusedBrowsingContext() && elementToFocus == mFocusedElement)) { - return; + return Nothing(); } MOZ_ASSERT(newBrowsingContext); @@ -1532,7 +1534,7 @@ void nsFocusManager::SetFocusInner(Element* aNewContent, int32_t aFlags, BrowsingContext* walk = focusedBrowsingContext; while (walk) { if (walk == bc) { - return; + return Nothing(); } walk = walk->GetParent(); } @@ -1549,13 +1551,13 @@ void nsFocusManager::SetFocusInner(Element* aNewContent, int32_t aFlags, bool inUnload; docShell->GetIsInUnload(&inUnload); if (inUnload) { - return; + return Nothing(); } bool beingDestroyed; docShell->IsBeingDestroyed(&beingDestroyed); if (beingDestroyed) { - return; + return Nothing(); } BrowsingContext* bc = docShell->GetBrowsingContext(); @@ -1570,7 +1572,7 @@ void nsFocusManager::SetFocusInner(Element* aNewContent, int32_t aFlags, do { bc = bc->GetParent(); if (bc && bc->IsDiscarded()) { - return; + return Nothing(); } } while (bc && !bc->IsInProcess()); if (bc) { @@ -1616,12 +1618,12 @@ void nsFocusManager::SetFocusInner(Element* aNewContent, int32_t aFlags, } if (!focusedPrincipal || !newPrincipal) { - return; + return Nothing(); } if (!focusedPrincipal->Subsumes(newPrincipal)) { NS_WARNING("Not allowed to focus the new window!"); - return; + return Nothing(); } } @@ -1728,11 +1730,12 @@ void nsFocusManager::SetFocusInner(Element* aNewContent, int32_t aFlags, LOGFOCUS((" Flags: %x Current Window: %p New Window: %p Current Element: %p", aFlags, mFocusedWindow.get(), newWindow.get(), mFocusedElement.get())); + const uint64_t actionId = GenerateFocusActionId(); LOGFOCUS( (" In Active Window: %d Moves to different BrowsingContext: %d " "SendFocus: %d actionid: %" PRIu64, isElementInActiveWindow, focusMovesToDifferentBC, sendFocusEvent, - aActionId)); + actionId)); if (sendFocusEvent) { Maybe blurredInfo; @@ -1785,19 +1788,19 @@ void nsFocusManager::SetFocusInner(Element* aNewContent, int32_t aFlags, ? focusedBrowsingContext.get() : nullptr), commonAncestor, focusMovesToDifferentBC, aAdjustWidget, - remainActive, aActionId, elementToFocus)) { - return; + remainActive, actionId, elementToFocus)) { + return Some(actionId); } } Focus(newWindow, elementToFocus, aFlags, focusMovesToDifferentBC, - aFocusChanged, false, aAdjustWidget, aActionId, blurredInfo); + aFocusChanged, false, aAdjustWidget, actionId, blurredInfo); } else { // otherwise, for inactive windows and when the caller cannot steal the // focus, update the node in the window, and raise the window if desired. if (allowFrameSwitch) { AdjustWindowFocus(newBrowsingContext, true, IsWindowVisible(newWindow), - aActionId); + actionId); } // set the focus node and method as needed @@ -1829,7 +1832,7 @@ void nsFocusManager::SetFocusInner(Element* aNewContent, int32_t aFlags, RaiseWindow(outerWindow, aFlags & FLAG_NONSYSTEMCALLER ? CallerType::NonSystem : CallerType::System, - aActionId); + actionId); } else { mozilla::dom::ContentChild* contentChild = mozilla::dom::ContentChild::GetSingleton(); @@ -1838,11 +1841,12 @@ void nsFocusManager::SetFocusInner(Element* aNewContent, int32_t aFlags, aFlags & FLAG_NONSYSTEMCALLER ? CallerType::NonSystem : CallerType::System, - aActionId); + actionId); } } } } + return Some(actionId); } static already_AddRefed GetParentIgnoreChromeBoundary( diff --git a/dom/base/nsFocusManager.h b/dom/base/nsFocusManager.h index 50a714237370..9a60e73a2a3c 100644 --- a/dom/base/nsFocusManager.h +++ b/dom/base/nsFocusManager.h @@ -214,8 +214,7 @@ class nsFocusManager final : public nsIFocusManager, * Setter for focusedWindow with CallerType */ MOZ_CAN_RUN_SCRIPT nsresult SetFocusedWindowWithCallerType( - mozIDOMWindowProxy* aWindowToFocus, mozilla::dom::CallerType aCallerType, - uint64_t aActionId); + mozIDOMWindowProxy* aWindowToFocus, mozilla::dom::CallerType aCallerType); /** * Given an element, which must be the focused element, activate the remote @@ -314,10 +313,13 @@ class nsFocusManager final : public nsIFocusManager, * * All actual focus changes must use this method to do so. (as opposed * to those that update the focus in an inactive window for instance). + * + * Returns Nothing() if we end up not trying to focus the element, + * otherwise returns the generated action id. */ - MOZ_CAN_RUN_SCRIPT void SetFocusInner(mozilla::dom::Element* aNewContent, - int32_t aFlags, bool aFocusChanged, - bool aAdjustWidget, uint64_t aActionId); + MOZ_CAN_RUN_SCRIPT Maybe SetFocusInner( + mozilla::dom::Element* aNewContent, int32_t aFlags, bool aFocusChanged, + bool aAdjustWidget); /** * Returns true if aPossibleAncestor is the same as aWindow or an