From f258998fcb1ac18acf09c97b93d66bc0aa9e13c4 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:25:00 -0700 Subject: [PATCH] fix: intermittent CI failure is-not-alwaysOnTop (#51135) * fix: intermittent CI failure is-not-alwaysOnTop Ensure that the `always-on-top-changed` event always fires with the right 'alwaysOnTop' boolean, regardless of interaction between SetZOrderLevel() and MoveBehindTaskBarIfNeeded(). We know what the value will be when all of the HWND events settle, so use that value. Co-authored-by: Charles Kerr * test: temporary commit to torture-test the new change with 1000 iterations Co-authored-by: Charles Kerr * test: keep eventually-becomes-consistent test but do not loop 1000 times Co-authored-by: Charles Kerr --------- Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Charles Kerr --- shell/browser/api/electron_api_base_window.cc | 4 +-- shell/browser/api/electron_api_base_window.h | 2 +- shell/browser/native_window.cc | 5 +-- shell/browser/native_window.h | 2 +- shell/browser/native_window_mac.mm | 5 ++- shell/browser/native_window_observer.h | 2 +- shell/browser/native_window_views.cc | 14 ++++---- spec/api-browser-window-spec.ts | 35 ++++++++++++++++--- 8 files changed, 48 insertions(+), 21 deletions(-) diff --git a/shell/browser/api/electron_api_base_window.cc b/shell/browser/api/electron_api_base_window.cc index c574a9e7d2..778b31ffb8 100644 --- a/shell/browser/api/electron_api_base_window.cc +++ b/shell/browser/api/electron_api_base_window.cc @@ -331,8 +331,8 @@ void BaseWindow::OnWindowLeaveHtmlFullScreen() { Emit("leave-html-full-screen"); } -void BaseWindow::OnWindowAlwaysOnTopChanged() { - Emit("always-on-top-changed", IsAlwaysOnTop()); +void BaseWindow::OnWindowAlwaysOnTopChanged(const bool is_always_on_top) { + Emit("always-on-top-changed", is_always_on_top); } void BaseWindow::OnExecuteAppCommand(const std::string_view command_name) { diff --git a/shell/browser/api/electron_api_base_window.h b/shell/browser/api/electron_api_base_window.h index 7687085a4b..b27e04e62f 100644 --- a/shell/browser/api/electron_api_base_window.h +++ b/shell/browser/api/electron_api_base_window.h @@ -92,7 +92,7 @@ class BaseWindow : public gin_helper::TrackableObject, void OnWindowLeaveFullScreen() override; void OnWindowEnterHtmlFullScreen() override; void OnWindowLeaveHtmlFullScreen() override; - void OnWindowAlwaysOnTopChanged() override; + void OnWindowAlwaysOnTopChanged(bool is_always_on_top) override; void OnExecuteAppCommand(std::string_view command_name) override; void OnTouchBarItemResult(const std::string& item_id, const base::DictValue& details) override; diff --git a/shell/browser/native_window.cc b/shell/browser/native_window.cc index 08a3d26f51..e4876dfaae 100644 --- a/shell/browser/native_window.cc +++ b/shell/browser/native_window.cc @@ -630,8 +630,9 @@ void NativeWindow::NotifyWindowLeaveHtmlFullScreen() { observers_.Notify(&NativeWindowObserver::OnWindowLeaveHtmlFullScreen); } -void NativeWindow::NotifyWindowAlwaysOnTopChanged() { - observers_.Notify(&NativeWindowObserver::OnWindowAlwaysOnTopChanged); +void NativeWindow::NotifyWindowAlwaysOnTopChanged(const bool is_always_on_top) { + observers_.Notify(&NativeWindowObserver::OnWindowAlwaysOnTopChanged, + is_always_on_top); } void NativeWindow::NotifyWindowExecuteAppCommand( diff --git a/shell/browser/native_window.h b/shell/browser/native_window.h index 17c1ccd67e..eb26b100bd 100644 --- a/shell/browser/native_window.h +++ b/shell/browser/native_window.h @@ -333,7 +333,7 @@ class NativeWindow : public base::SupportsUserData, virtual void NotifyWindowLeaveFullScreen(); void NotifyWindowEnterHtmlFullScreen(); void NotifyWindowLeaveHtmlFullScreen(); - void NotifyWindowAlwaysOnTopChanged(); + void NotifyWindowAlwaysOnTopChanged(bool is_always_on_top); void NotifyWindowExecuteAppCommand(std::string_view command_name); void NotifyTouchBarItemInteraction(const std::string& item_id, base::DictValue details); diff --git a/shell/browser/native_window_mac.mm b/shell/browser/native_window_mac.mm index d9479535e8..85ed3b4b42 100644 --- a/shell/browser/native_window_mac.mm +++ b/shell/browser/native_window_mac.mm @@ -957,10 +957,9 @@ void NativeWindowMac::SetWindowLevel(int unbounded_level) { // a bug of Cocoa or macOS. SetMaximizable(was_maximizable_); - // This must be notified at the very end or IsAlwaysOnTop - // will not yet have been updated to reflect the new status if (did_z_order_level_change) - NativeWindow::NotifyWindowAlwaysOnTopChanged(); + NativeWindow::NotifyWindowAlwaysOnTopChanged(z_order_level != + ui::ZOrderLevel::kNormal); } ui::ZOrderLevel NativeWindowMac::GetZOrderLevel() const { diff --git a/shell/browser/native_window_observer.h b/shell/browser/native_window_observer.h index 957e4f83e4..82bb2aed9c 100644 --- a/shell/browser/native_window_observer.h +++ b/shell/browser/native_window_observer.h @@ -94,7 +94,7 @@ class NativeWindowObserver : public base::CheckedObserver { virtual void OnWindowLeaveFullScreen() {} virtual void OnWindowEnterHtmlFullScreen() {} virtual void OnWindowLeaveHtmlFullScreen() {} - virtual void OnWindowAlwaysOnTopChanged() {} + virtual void OnWindowAlwaysOnTopChanged(bool is_always_on_top) {} virtual void OnTouchBarItemResult(const std::string& item_id, const base::DictValue& details) {} virtual void OnNewWindowForTab() {} diff --git a/shell/browser/native_window_views.cc b/shell/browser/native_window_views.cc index 0430bb804d..10ed1e9844 100644 --- a/shell/browser/native_window_views.cc +++ b/shell/browser/native_window_views.cc @@ -1133,16 +1133,18 @@ bool NativeWindowViews::IsClosable() const { #endif } -void NativeWindowViews::SetAlwaysOnTop(ui::ZOrderLevel z_order, +void NativeWindowViews::SetAlwaysOnTop(const ui::ZOrderLevel z_order, const std::string& level, - int relativeLevel) { - bool level_changed = z_order != widget()->GetZOrderLevel(); + const int relativeLevel) { + const bool level_changed = z_order != widget()->GetZOrderLevel(); + const bool always_on_top = z_order != ui::ZOrderLevel::kNormal; + widget()->SetZOrderLevel(z_order); #if BUILDFLAG(IS_WIN) // Reset the placement flag. behind_task_bar_ = false; - if (z_order != ui::ZOrderLevel::kNormal) { + if (always_on_top) { // On macOS the window is placed behind the Dock for the following levels. // Re-use the same names on Windows to make it easier for the user. static constexpr auto levels = base::MakeFixedFlatSet( @@ -1152,10 +1154,8 @@ void NativeWindowViews::SetAlwaysOnTop(ui::ZOrderLevel z_order, #endif MoveBehindTaskBarIfNeeded(); - // This must be notified at the very end or IsAlwaysOnTop - // will not yet have been updated to reflect the new status if (level_changed) - NativeWindow::NotifyWindowAlwaysOnTopChanged(); + NativeWindow::NotifyWindowAlwaysOnTopChanged(always_on_top); } ui::ZOrderLevel NativeWindowViews::GetZOrderLevel() const { diff --git a/spec/api-browser-window-spec.ts b/spec/api-browser-window-spec.ts index 6061a952c3..99c36ca13a 100755 --- a/spec/api-browser-window-spec.ts +++ b/spec/api-browser-window-spec.ts @@ -2706,6 +2706,26 @@ describe('BrowserWindow module', () => { describe('BrowserWindow.setAlwaysOnTop(flag, level)', () => { let w: BrowserWindow; + const alwaysOnTopSettleTimeout = 5000; + + const waitForAlwaysOnTop = async (alwaysOnTop: boolean, label: string) => { + try { + await waitUntil(() => w.isAlwaysOnTop() === alwaysOnTop, { + rate: 50, + timeout: alwaysOnTopSettleTimeout + }); + } catch (error) { + throw new Error(`${label}: ${(error as Error).message}`); + } + }; + + const setAlwaysOnTopAndWaitForState = async (alwaysOnTop: boolean, label: string) => { + const alwaysOnTopChanged = once(w, 'always-on-top-changed') as Promise<[any, boolean]>; + w.setAlwaysOnTop(alwaysOnTop); + const [, emittedAlwaysOnTop] = await alwaysOnTopChanged; + expect(emittedAlwaysOnTop).to.equal(alwaysOnTop, `${label}: unexpected event payload`); + await waitForAlwaysOnTop(alwaysOnTop, label); + }; afterEach(closeAllWindows); @@ -2738,13 +2758,20 @@ describe('BrowserWindow module', () => { }); it('causes the right value to be emitted on `always-on-top-changed`', async () => { - const alwaysOnTopChanged = once(w, 'always-on-top-changed') as Promise<[any, boolean]>; expect(w.isAlwaysOnTop()).to.be.false('is alwaysOnTop'); - w.setAlwaysOnTop(true); - const [, alwaysOnTop] = await alwaysOnTopChanged; - expect(alwaysOnTop).to.be.true('is not alwaysOnTop'); + await setAlwaysOnTopAndWaitForState(true, 'single transition'); }); + ifit(process.platform === 'win32')( + 'eventually becomes consistent with the emitted value after enable and disable transitions', + async () => { + expect(w.isAlwaysOnTop()).to.be.false('is alwaysOnTop'); + + await setAlwaysOnTopAndWaitForState(true, 'enable'); + await setAlwaysOnTopAndWaitForState(false, 'disable'); + } + ); + ifit(process.platform === 'darwin')('honors the alwaysOnTop level of a child window', () => { w = new BrowserWindow({ show: false }); const c = new BrowserWindow({ parent: w });