[WIP] fix: avoid blurring window when using Character Viewer pop-up on macOS

[erickzhao]

Description of Change

Context

In PR #18995, I had submitted a fix such that focus and blur events were listening for changes in the Key window rather than the Main window.

However, Issue #19124 showed that since a window would lose its Key status when the macOS Character Viewer was activated, the blur event would make any input on the window lose its focus.

Then, the character selected from the Character Viewer wouldn't be entered into the input.

Change

I did not find much documentation regarding the behavior of NSWindow with respect to the Character Viewer. After playing around with windowDidResignKey, I found that when the Character Viewer appears, the window loses its Key status, but the [NSApp keyWindow] variable still referenced the original window.

Thus, I added a conditional check to only apply the blur event if the resigning window and the new key window weren't the same.

cc @codebytere @zcbenz @javan

Checklist

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

Release Notes

Notes: Fixed bug on macOS that broke input from the Character Viewer popup.

[zcbenz]

I think the root problem of #19124 and #18502 is rwhv->SetActive getting called when it shouldn't.

After checking the call stack of RenderWidgetHost::SetActive I found it is already being called inside content module, and it is not being called by other parts of Chrome, so removing the rwhv->SetActive calls in BrowserWindow should be enough to fix the issues.

(The rwhv->SetActive calls were inherited from ancient Chrome code.)

And we should also check whether removing rwhv->SetActive alone would be able to fix #18502, and if it is, we should also revert #18995 as focus/blur should map to the main window events instead of key window events.

[nornagon]

Looks like Chrome's app shim does the same thing.

Please remove the commented-out debugging code before you merge :)

[erickzhao]

It seems like removing the rwhv->SetActive calls fixes #19124. However, #18502 persists even with those calls removed.

However, both solutions give less than desirable outcomes for firing focus and blur events. On browser and previous versions of Electron, opening the Character Viewer wouldn't fire either event, since the window's Main status wouldn't change.

With the Key change:

1. Removing rwhv->SetActive fires both focus and blur.
2. This PR's original change only fires focus since the blur event is blocked.

I'm not particularly sure what to do in this case. It seems like removing the rwhv calls gives a more reasonable behavior, though.

cc @zcbenz @nornagon

[zcbenz]

I think we got confused with 2 types of focus/blur here: one is the focus events of BrowserWindow, the other is the focus events of DOM Window.

For BrowserWindow, the focus events should be main window events, otherwise things like opening spotlight or status item menu would trigger the focus events. For example with PR, while it hacks windowDidResignKey so blur is emitted correctly for some cases, opening and closing spotlight would still tigger focus on BrowserWindow, and I don't think we can cover all edge cases.

For DOM Window, the focus events, which are controlled by SetActive or StoreFocus/RestoreFocus, should be key window events, as investigated by @erickzhao and @nornagon.

So to correctly fix both issues without breaking focus events of BrowserWindow, I think we should:

1. Still call OnWindowBlur/OnWindowBlur(which emits focus/blur on BrowserWindow) on main window events.
2. Call SetActive on key window events, with check in windowDidResignKey.
3. Call StoreFocus/RestoreFocus on main window events. (Chrome calls them on tab change, which seems to map to the main window events, it needs some testing to confirm.)

We would need to add more methods to NativeWindowObserver to handle key window events separately instead of keeping everything in OnWindowBlur/OnWindowBlur.

[erickzhao]

@zcbenz @codebytere @nornagon do you think that we should first move to revert the original change? PR #18995 was backported to 6, 5, and 4, and it seems like this issue is more immediately pressing for macOS users than the modal focus behavior.

[zcbenz]

Reverting #18995 first sounds good to me.

[erickzhao]

Going to close this and attempt to resolve #18502 in a future PR as per the comments above.