Skip to content
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

document.hasFocus() can return true for multiple windows being opened #122352

Closed
jrieken opened this issue Apr 27, 2021 · 10 comments
Closed

document.hasFocus() can return true for multiple windows being opened #122352

jrieken opened this issue Apr 27, 2021 · 10 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug confirmed Issue has been confirmed by VS Code Team member electron Issues and items related to Electron insiders-released Patch has been released in VS Code Insiders upstream-issue-fixed The underlying upstream issue has been fixed verified Verification succeeded
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Apr 27, 2021

Testing #122200

  • have multiple windows of VS Code open
  • enable or disable window.nativeTabs
  • 💩 each window shows a model dialog about needing to restart
  • select restart in each window
  • 🐛 restart doesn't happen
Untitled.mov
@bpasero
Copy link
Member

bpasero commented Apr 27, 2021

We probably have this issue for any setting that the relauncher.contribution.ts listens to. Will see if I can just show the dialog in the active window.

@bpasero bpasero added this to the April 2021 milestone Apr 27, 2021
@bpasero bpasero added workbench-window Window management electron Issues and items related to Electron and removed workbench-window Window management labels Apr 27, 2021
@bpasero bpasero removed this from the April 2021 milestone Apr 28, 2021
@bpasero bpasero assigned deepak1556 and unassigned bpasero Apr 28, 2021
@bpasero
Copy link
Member

bpasero commented Apr 28, 2021

Turns out we already check for the window having focus or not:

I cannot reproduce this but if document.hasFocus() returns true for multiple windows being opened, I suspect an Electron issue. I am open to changing the method to check for another property, but having it return sync was one of the main reasons I use document.hasFocus(), we use the same in web btw.

//cc @rzhao271

@bpasero
Copy link
Member

bpasero commented Apr 28, 2021

@jrieken are you able to reliably reproduce? Any special window manager installed?

@bpasero bpasero changed the title Issues when configured window.nativeTabs while having many windows open document.hasFocus() can return true for multiple windows being opened Apr 28, 2021
@jrieken
Copy link
Member Author

jrieken commented Apr 28, 2021

Yeah, it is tricky... So, the first time I set "window.nativeTabs": true it works. Then I comment out this setting and the modal shows. First only for the current window and a bit later (~2sec?) for all windows. Once that happened it will always reproduce

@bpasero bpasero added the confirmed Issue has been confirmed by VS Code Team member label Apr 28, 2021
@bpasero
Copy link
Member

bpasero commented Apr 28, 2021

Oh yeah I can reproduce. Steps:

  • (use a fresh user data dir to start clean)
  • open 3 folders in 3 windows
  • enable native tabs and restart
  • disable native tabs

=> 🐛 each window shows the dialog

@bpasero
Copy link
Member

bpasero commented Apr 28, 2021

Reported as electron/electron#28897

@deepak1556
Copy link
Collaborator

Temporary workarounds:

  1. Start the window with show: false and window.show()
  2. webContents.removeAllListeners('load-url')

@deepak1556 deepak1556 added the upstream-issue-linked This is an upstream issue that has been reported upstream label Apr 28, 2021
@rzhao271 rzhao271 self-assigned this May 4, 2021
@rzhao271
Copy link
Contributor

Ok I finally built electron 12 source on the Mac.
I was about to let it build overnight with goma, but for some reason I got logged out of the server. I wouldn't be surprised if this has to do with power settings on my device.

With this build, I commented out the line that initially focuses the frame.
I then opened @bpasero's Fiddle from electron/electron#28897 with that build, and noticed that the top-most frame that showed up had focus initially true, and the other two frames had focus initially false.

I then tested electron/electron#25287 with the build, and found out that that Fiddle still works perfectly fine, too.

Therefore, I believe we can remove most of https://github.com/electron/electron/pull/25292/files and just let Chromium handle the focusing again.

@rzhao271 rzhao271 added this to the May 2021 milestone May 13, 2021
@rzhao271 rzhao271 added the bug Issue identified by VS Code Team member as probable bug label May 13, 2021
@deepak1556 deepak1556 added upstream-issue-fixed The underlying upstream issue has been fixed and removed upstream-issue-linked This is an upstream issue that has been reported upstream labels May 19, 2021
@rzhao271
Copy link
Contributor

@deepak1556's upstream fix is here: electron/electron#29204, and the backport for that fix is here: electron/electron#29235

Ashray123 pushed a commit to Ashray123/vscode that referenced this issue May 25, 2021
@bpasero bpasero added the verified Verification succeeded label Jun 4, 2021
@bpasero bpasero modified the milestones: May 2021, June 2021 Jun 11, 2021
@bpasero bpasero reopened this Jun 11, 2021
@bpasero bpasero removed the verified Verification succeeded label Jun 11, 2021
@bpasero
Copy link
Member

bpasero commented Jun 11, 2021

Given #125122 and other related issues, we will be going back to Electron 12.0.7 for recovery. Hence reopening this issue, even though technically it is fixed for insiders because we are not going back there.

@jrieken jrieken added the verified Verification succeeded label Jun 30, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug confirmed Issue has been confirmed by VS Code Team member electron Issues and items related to Electron insiders-released Patch has been released in VS Code Insiders upstream-issue-fixed The underlying upstream issue has been fixed verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants
@bpasero @deepak1556 @jrieken @rzhao271 and others