fix: make window.flashFrame(bool) flash continuously on macOS #41391

yuzawa-san · 2024-02-21T13:20:07Z

Description of Change

I was developing a cross-platform (Windows, macOS) application and found that the Windows behavior was consistent with the semantics of flashFrame(bool). i.e. the first call flashFrame(true) will start and it will continue until you call flashFrame(false). However, the macOS implementation is not at parity with this. The first flashFrame(true) call will indeed flash, but only once (given the NSInformationalRequest level). The solution to have it flash continuously until you stop it would be to change it to NSCriticalRequest). If somebody did want to explicitly use NSInformationalRequest they can still use dock.bounce('informational') . The main objective here is to bring the macOS into parity with the Windows, so that a developer would need to only call flashFrame in a platform independent manner. Currently, I was doing:

    if (__DARWIN__) {
      // This is used instead of flashFrame in order to match the OS dialog behavior.
      app.dock.bounce('critical')
    } else {
      this.window.once('focus', () => this.window.flashFrame(false))
      this.window.flashFrame(true)
    }

I did not find any unit tests for this piece of code and I do not know how to assert on the userAttentionRequest given ther underlying API does not expose a way to read the state.
If you feel any docs need updating let me know and I can update them.

Checklist

PR description included and stakeholders cc'd
npm test passes
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: made window.flashFrame(bool) flash continuously on macOS

welcome · 2024-02-21T13:20:10Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

MarshallOfSound

Interested in other thoughts on this but although I understand the goal for alignment I don't think that will be reached and this change feels more break-y than align-y. The docs don't explicitly state that it continues infinitely until stopped although I guess you could claim it's implied.

I'm interested in the behavior on linux as that would help guide whether this change is more acceptable to me 🤔

yuzawa-san · 2024-02-28T02:24:03Z

@MarshallOfSound yes based on the docs (specifically the code example) and the semantics around how they are called it sure appears that there is a call with true to start and a call with false to stop. So if that is not the case, then I suppose those should be updated. But even then the bool argument is odd and still part of the public API.

I do not have a linux or windows environment to test with, but I dug in and I think I found the windows and linux implementation:

electron/shell/browser/native_window_views.cc

Lines 1089 to 1103 in 267c079

    
           void NativeWindowViews::FlashFrame(bool flash) { 
        
           #if BUILDFLAG(IS_WIN) 
        
             // The Chromium's implementation has a bug stopping flash. 
        
             if (!flash) { 
        
               FLASHWINFO fwi; 
        
               fwi.cbSize = sizeof(fwi); 
        
               fwi.hwnd = GetAcceleratedWidget(); 
        
               fwi.dwFlags = FLASHW_STOP; 
        
               fwi.uCount = 0; 
        
               FlashWindowEx(&fwi); 
        
               return; 
        
             } 
        
           #endif 
        
             widget()->FlashFrame(flash); 
        
           }

I think the linux case (and windows case partially) delegates to the chromium ostensibly with widget()->FlashFrame(flash). Digging into the chromium codebase, I think I found at least one of the linux implementations https://chromium.googlesource.com/chromium/src/+/47515abd6b3ae36b989683c5663df06d6bcb919b/chrome/browser/ui/gtk/browser_window_gtk.cc#790
which has a comment May not be respected by all window managers. so maybe there is inconsistent gtk support/behavior on this. The gtk docs do not state an exact behavior.

I found another X11 FlashFrame implementation https://chromium.googlesource.com/chromium/src/+/ae9485d020b2834ecb0a0439c76914d3386af405/ui/platform_window/x11/x11_window.cc#448 which seems to set X11's WMHints urgency flag. I found this https://tronche.com/gui/x/xlib/ICC/client-to-window-manager/wm-hints.html which states:

The UrgencyHint flag, if set in the flags field, indicates that the client deems the window contents to be urgent, requiring the timely response of the user. The window manager will make some effort to draw the user's attention to this window while this flag is set. The client must provide some means by which the user can cause the urgency flag to be cleared (either mitigating the condition that made the window urgent or merely shutting off the alarm) or the window to be withdrawn

Digging further I also found this comment element-hq/element-desktop#715 (comment) unclear if that is correct.

zcbenz

This is kind of a breaking change, but I think the behavior parity worths it, and if an app is affected they can easily switch to use the dock.bounce API.

/cc @electron/wg-api

erickzhao

API LGTM

I think this is a good change as long as we document it in breaking-changes.md

yuzawa-san · 2024-03-06T13:16:49Z

@erickzhao do you want me to update breaking-changes.md? I could, but I don't know if the maintainers do that.

If so, what section would I put it in? 30? 29, since this was tagged with semver-minor? Any specific verbiage you are looking for?

erickzhao · 2024-03-06T18:10:59Z

Hey @yuzawa-san, this is labeled as a no-backport change, so it's currently only targeting main, which is on v31. I think we'd want to change the semver label as well here.

MarshallOfSound

API LGTM

Let's mark this as a breaking change though in the doc, no backport

jkleinsc · 2024-04-16T15:00:28Z

@yuzawa-san can you add an entry under Planned Breaking API Changes (31.0)
in the doc https://github.com/electron/electron/blob/main/docs/breaking-changes.md? If we can get that added, I think this PR can get merged in.

This brings the behavior to parity with Windows and Linux. Prior behavior: The first `flashFrame(true)` bounces the dock icon only once (using the [NSInformationalRequest](https://developer.apple.com/documentation/appkit/nsrequestuserattentiontype/nsinformationalrequest) level) and `flashFrame(false)` does nothing. New behavior: Flash continuously until `flashFrame(false)` is called. This uses the [NSCriticalRequest](https://developer.apple.com/documentation/appkit/nsrequestuserattentiontype/nscriticalrequest) level instead. To explicitly use `NSInformationalRequest` to cause a single dock icon bounce, it is still possible to use [`dock.bounce('informational')`](https://www.electronjs.org/docs/latest/api/dock#dockbouncetype-macos).

yuzawa-san · 2024-04-17T12:22:50Z

@jkleinsc @MarshallOfSound I have updated the breaking changes document.

jkleinsc · 2024-04-17T16:42:26Z

Merging as CI failure unrelated to PR change.

welcome · 2024-04-17T16:42:37Z

Congrats on merging your first pull request! 🎉🎉🎉

release-clerk · 2024-04-17T16:42:37Z

Release Notes Persisted

made window.flashFrame(bool) flash continuously on macOS

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Feb 21, 2024

yuzawa-san mentioned this pull request Feb 21, 2024

Follow OS behavior for out of focus modal dialogs desktop/desktop#17220

Merged

MarshallOfSound reviewed Feb 21, 2024

View reviewed changes

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Feb 28, 2024

zcbenz added semver/minor backwards-compatible functionality no-backport labels Mar 4, 2024

electron-cation bot added the api-review/requested 🗳 label Mar 4, 2024

zcbenz approved these changes Mar 4, 2024

View reviewed changes

erickzhao suggested changes Mar 4, 2024

View reviewed changes

codebytere requested a review from MarshallOfSound March 19, 2024 11:51

MarshallOfSound added semver/major incompatible API changes and removed semver/minor backwards-compatible functionality labels Apr 15, 2024

MarshallOfSound approved these changes Apr 15, 2024

View reviewed changes

yuzawa-san force-pushed the fix-mac-flashFrame branch from de714b0 to 2c30509 Compare April 17, 2024 12:21

yuzawa-san requested a review from a team as a code owner April 17, 2024 12:21

jkleinsc approved these changes Apr 17, 2024

View reviewed changes

jkleinsc merged commit bf754a3 into electron:main Apr 17, 2024
12 of 14 checks passed

emil-wire mentioned this pull request Dec 4, 2024

chore: fix the annoying dock bounce on macOS (WPB-14749) wireapp/wire-desktop#8392

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make window.flashFrame(bool) flash continuously on macOS #41391

fix: make window.flashFrame(bool) flash continuously on macOS #41391

yuzawa-san commented Feb 21, 2024 •

edited

Loading

welcome bot commented Feb 21, 2024

MarshallOfSound left a comment

yuzawa-san commented Feb 28, 2024 •

edited

Loading

zcbenz left a comment

erickzhao left a comment

yuzawa-san commented Mar 6, 2024

erickzhao commented Mar 6, 2024

MarshallOfSound left a comment

jkleinsc commented Apr 16, 2024

yuzawa-san commented Apr 17, 2024

jkleinsc commented Apr 17, 2024

welcome bot commented Apr 17, 2024

release-clerk bot commented Apr 17, 2024

fix: make window.flashFrame(bool) flash continuously on macOS #41391

fix: make window.flashFrame(bool) flash continuously on macOS #41391

Conversation

yuzawa-san commented Feb 21, 2024 • edited Loading

Description of Change

Checklist

Release Notes

welcome bot commented Feb 21, 2024

MarshallOfSound left a comment

Choose a reason for hiding this comment

yuzawa-san commented Feb 28, 2024 • edited Loading

zcbenz left a comment

Choose a reason for hiding this comment

erickzhao left a comment

Choose a reason for hiding this comment

yuzawa-san commented Mar 6, 2024

erickzhao commented Mar 6, 2024

MarshallOfSound left a comment

Choose a reason for hiding this comment

jkleinsc commented Apr 16, 2024

yuzawa-san commented Apr 17, 2024

jkleinsc commented Apr 17, 2024

welcome bot commented Apr 17, 2024

release-clerk bot commented Apr 17, 2024

yuzawa-san commented Feb 21, 2024 •

edited

Loading

yuzawa-san commented Feb 28, 2024 •

edited

Loading