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 <charles@charleskerr.com>

* test: temporary commit to torture-test the new change with 1000 iterations

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* test: keep eventually-becomes-consistent test but do not loop 1000 times

Co-authored-by: Charles Kerr <charles@charleskerr.com>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>

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) {

shell/browser/api/electron_api_base_window.h

@@ -92,7 +92,7 @@ class BaseWindow : public gin_helper::TrackableObject<BaseWindow>,
   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;

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(

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);

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 {

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() {}

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<std::string_view>(
@@ -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 {

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 });