From abd47d40cb06340d158fca4a4b2d7f0e3d04015c Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Mon, 20 Apr 2026 15:25:53 -0700 Subject: [PATCH] fix: intermittent CI failure is-not-alwaysOnTop (#51133) * 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 d4a51d41be..4fe4fd3ad6 100644 --- a/shell/browser/api/electron_api_base_window.cc +++ b/shell/browser/api/electron_api_base_window.cc @@ -321,8 +321,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 83cf03e035..b62d812781 100644 --- a/shell/browser/api/electron_api_base_window.h +++ b/shell/browser/api/electron_api_base_window.h @@ -91,7 +91,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 fbb0974558..c4608a8da1 100644 --- a/shell/browser/native_window.cc +++ b/shell/browser/native_window.cc @@ -596,8 +596,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 a2715b1ba1..22e159cbec 100644 --- a/shell/browser/native_window.h +++ b/shell/browser/native_window.h @@ -332,7 +332,7 @@ class NativeWindow : public views::WidgetDelegate { 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 b8df24768e..8244fe1a2d 100644 --- a/shell/browser/native_window_mac.mm +++ b/shell/browser/native_window_mac.mm @@ -974,10 +974,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 1c52d002d4..c3f9f7fd48 100644 --- a/shell/browser/native_window_views.cc +++ b/shell/browser/native_window_views.cc @@ -1134,16 +1134,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( @@ -1153,10 +1155,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 57d2ebc3ce..d303a98480 100755 --- a/spec/api-browser-window-spec.ts +++ b/spec/api-browser-window-spec.ts @@ -2800,6 +2800,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); @@ -2832,13 +2852,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 });