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

fix: remove Electron BrowserWindow focus call #29126

Closed
wants to merge 1 commit into from

Conversation

rzhao271
Copy link
Contributor

Description of Change

This change undoes #25292.

It removes the handler that focuses the BrowserWindow upon initial load. On Mac at least, it seems that Chromium handles the focusing just fine.

CC @deepak1556

Checklist

I'll work on finishing up these tasks later but I'll upload this PR for now for review.

Release Notes

Notes: none.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 12, 2021
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm you've tested this change against the original fiddle reporting the issue?

https://gist.github.com/CharlieHess/c507468cd40630d04f58c6514100b997

@rzhao271
Copy link
Contributor Author

rzhao271 commented May 12, 2021

I can confirm that that Fiddle continues to work even after the change.
I was expecting the grey unfocused border to show, but instead, as soon as I clicked the dock icon to pull up the window, I saw the border was yellow.
I tried recording a gif of it with gifcap.dev on Safari, but the webapp errored out.

I would like to test it manually on Windows later, too, and record gifs of my findings there.

@deepak1556 deepak1556 added the semver/patch backwards-compatible bug fixes label May 12, 2021
@MarshallOfSound MarshallOfSound dismissed their stale review May 13, 2021 00:36

concerns alleviated

@rzhao271
Copy link
Contributor Author

rzhao271 commented May 13, 2021

Here's some recordings I made of the fix on Mac:

recording (8)
recording (7)

However, I noticed that webContents.getFocusedWebContents() returns false on Mac, unless I run it in a setTimeout(() => { ... }, 0); call.

On Windows, I'm not sure what the expected behaviour is.
When I set show: false, nothing seems to happen. I have to add in a setTimeout which calls win.show() to get the window to show up. On Mac, I just need to click on the Electron icon in the dock.
When I set show: true and run a variation of Ben's fiddle (https://gist.github.com/rzhao271/65a35a76c4c200ee04adb82d2765555a), none of the windows are initially focused (see below). I'm not sure if this is the expected behaviour on Windows.

recording (9)

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 13, 2021
@rzhao271
Copy link
Contributor Author

An example showing how the original Fiddle behaviour is still as expected (yellow frame showing up = good, grey frame showing up = bad):
recording (8)

An example showing how @bpasero's Fiddle now accurately reports which window is focused on a Mac (only the top-most window's devtools shows true as the first line, the other ones show false):
recording (7)

However, all of the windows now claim to be unfocused on Windows during startup:
recording (9)

Additionally, webContents.getFocusedWebContents() now returns false unless it's run inside of a setTimeout. I think this occurs due to Electron not picking up the fact that Chromium is now responsible for focusing the webContents. I believe @deepak1556 is looking into this case.

@deepak1556
Copy link
Member

I am unable to push changes to this PR, closing in favor of #29204

@deepak1556 deepak1556 closed this May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants