-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Follow OS behavior for out of focus modal dialogs #17220
Conversation
bd42300
to
1ce3e4b
Compare
👋 @yuzawa-san Thanks for wanting to contribute! This seems like a pattern worth investigating. Could you see if you can dig up some references regarding when to use dock bounce from official Apple documentation so that we can make sure we're following official platform guidance? Also, is there a similar pattern we should be using on Windows for consistency? Something like
I haven't tried this myself but this appears to happen even when the Windows is in focus, is that right? Is that the intended behavior? It seems like it could be perceived as annoying, is there any documentation or guidance you can refer to here? |
This comment was marked as spam.
This comment was marked as spam.
@niik Thanks for the comment. I was able to dig up this piece in the macOS docs for requestUserAttention:
I was also able to run this AppleScript in the mac Script Editor (similar to electron's delay 5
display alert "hello" as informational
delay 5
display alert "hello" as warning
delay 5
display alert "hello" as critical to validate the system's alert functionality. So you are correct, there is (at least in Ventura) no beep when the application is focus. All cause the dock to bounce at critical level as the docs indicate, so I'll fix that up real soon to match the system dialog behavior. Ideally, I would use flashFrame on mac as well, but that uses the non-critical level https://github.com/electron/electron/blob/0b0707145b157343c42266d2586ed9413a1d54f5/shell/browser/native_window_mac.mm#L1004C57-L1004C79 I may open a bug with electron since that would mean the Windows behavior does not match the Mac behavior (flashing until you stop vs just flashing once). |
1ce3e4b
to
6102d7d
Compare
6102d7d
to
7193778
Compare
7193778
to
3ac0e86
Compare
3ac0e86
to
59543a3
Compare
59543a3
to
5570db1
Compare
@niik is there anything else in particular that you are looking for in order to consider this PR? |
Hey @yuzawa-san , thank you for your work 💖
Then, about this other thing you mentioned…
That would be nice if you haven't done it already, but there seems to be a reasonable case for |
5570db1
to
0281868
Compare
@sergiou87 I was able to find this for windows https://learn.microsoft.com/en-us/windows/win32/uxguide/winenv-taskbar#taskbar-button-flashing and https://learn.microsoft.com/en-us/windows/win32/uxguide/mess-warn however its for windows 7, they seem to have newer docs but they are not as in depth as the older ones (which they say are still generally correct in the disclaimer at the tops of the old pages). Here are the salient pieces:
They also repeat "no sound" in the dialog docs:
Given Windows seems very anti-sound, I have updated this PR to only beep on macOS (where that is the modal dialog behavior). I created this PR in electron electron/electron#41391 so we'll see where that goes. Ideally, the code can be a little bit simpler if that gets merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! That is very interesting, I haven't tested it yet but it looks like the app behavior will match those guidelines. For now, I will leave you with a few change suggestions 😄
app/src/ui/main-process-proxy.ts
Outdated
@@ -155,6 +155,9 @@ export const getCurrentWindowZoomFactor = invokeProxy( | |||
0 | |||
) | |||
|
|||
/** Tell the main process to that a modal dialog has opened */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** Tell the main process to that a modal dialog has opened */ | |
/** Tell the main process that a modal dialog has opened */ |
app/src/ui/main-process-proxy.ts
Outdated
@@ -155,6 +155,9 @@ export const getCurrentWindowZoomFactor = invokeProxy( | |||
0 | |||
) | |||
|
|||
/** Tell the main process to that a modal dialog has opened */ | |||
export const dialogOpened = sendProxy('dialog-opened', 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the convention from other event-related stuff, I think the function name should be sendDialogDidOpen
and the event dialog-did-open
:
export const dialogOpened = sendProxy('dialog-opened', 0) | |
export const sendDialogDidOpen = sendProxy('dialog-did-open', 0) |
app/src/main-process/app-window.ts
Outdated
if (__DARWIN__) { | ||
shell.beep() | ||
// 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) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment explaining this stuff? Basically, why beeping on macOS and using bounce('critical')
vs flashFrame
on Windows (feel free to link any of the UX docs you provided in this PR if needed)
0281868
to
f038823
Compare
@sergiou87 I have updated the PR with your requested changes. I have also renamed the PR title as well to better reflect what we're trying to accomplish here. |
f038823
to
ade5146
Compare
@sergiou87 electron/electron#41391 was just merged (presumably for v31), so I was able to clean up the macOS logic. It may not bounce continuously until this project gets to that version, but when then upgrade happens this code won't need to be updated. |
Great work, thank you @yuzawa-san !!! 🙇♂️ I'm personally ok with having this difference in behavior until we update to Electron v31 (probably at some point this year or in early 2025), but I will check with the team next week and let you know. |
When the application window is not in focus, but a modal dialog is opened, we can play the system beep then bounce the dock (mac) or flash the frame (windows, etc) in order to indicate to the user that something is seeking their attention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply @yuzawa-san
We are ok with this workaround, even though Electron 31 is around the corner and in a few months we will be able to get rid of it. I'm gonna approve it and merge it. Thank you for your patience! 💖
Some other thing we wanted to ask you is if you'd be willing to push for another API change/addition in Electron that also takes care of the shell.beep()
part. It feels kind of wrong that that kind of "system behavior" is handled by each app instead of being handled by Electron, so maybe some kind of this.window.requestUserAttention('critical')
and both bounces/flashes and beeps, would be ideal.
Thanks for your contribution! 🙇♂️
Description
desktop/app/src/ui/dialog/dialog.tsx
Line 167 in 79992ce
The dock bounce being "critical" (bouncing until the app comes back into focus) is a function of whether the dialog type is error. Warning dialogs will beep but only bounce once. Information dialogs won't beep or bounce.The beep and bounce will occur for any modal dialog opened out of view in order to match the macOS system dialog behavior.Screenshots
Here is a video of me starting in the application, then triggering a push which has a push hook that takes several seconds and will fail. Then I change focus to another application. Then the hook fails, there is a beep (i could not record audio) and the dock bounce which continues until I focus on the application again.
dialog-beep-bounce.mp4
Release notes
Notes: Beep and bounce dock for out of focus modal dialogs