MacViews triage: to robliao@ for M-68.

** Bulk Edit **

FYI: Starting 04/13 M68 will be in canary, M68 Dev promotion will be on 04/26.

Pls mark the bug as fixed if CL is landed in trunk and nothing else is pending. Thank you.

This seems due to the fact that BridgedNativeWIdget conflates Key Window and Main Window. In this case, the browser window resigns key but does not resign main. In views terms, it means the browser window is still active but does not receive key focus.

The code today equates key and activation together, which is incorrect.

This is solved (cross-platform) for the bookmark bubble using Widget::IsAlwaysRenderAsActive (other platforms have no concept of main vs key, so use this whenever key and activation are not equal).

The issue is probably that tab-modal and window-modal dialogs are not using it..

Windows has a separate notion of Keyboard Focus and Activation too.

For Active:
GetActive / SetActive / WM_ACTIVATE

For Keyboard Focus:
GetFocus / SetFocus / WM_SETFOCUS+WM_KILLFOCUS

The rules for HWND active and keyboard focus I think are what are different.

What platform did you have in mind?

Is "Aura" a platform :). AFAIK it only has "focus"

For Aura, I think that might be due to what aura::Window's are allowed to do.

It seems in both Windows and Mac, windows can be embedded within a parent window (child HWNDs for Windows and panes for Mac). This creates a world where activation and key focus do not target the same window.

I don't know if aura::Windows can do this.

Also...

I'm not sure we want to encode this behavior cross platform in IsAlwaysRenderAsActive(). Both platforms have different expectations for when to render the inactive state in these sorts of experiences.

In Windows, the owner window (different from the parent window), is expected to go inactive when the owned dialog appears and receives activation.

On Mac, the parent window is not expected to give up activation.

Quick Notes:

The stack that conflates the two is here:
frame #0: 0x000000010a729776 libchrome_dll.dylib`BrowserNonClientFrameView::ActivationChanged(this=0x0000000100373720, active=false) at browser_non_client_frame_view.cc:356                                                            
    frame #1: 0x000000013b9170d4 libviews.dylib`views::Widget::OnNativeWidgetActivationChanged(this=0x00000001617be7e0, active=false) at widget.cc:1042                                                                                     
    frame #2: 0x000000013b772754 libviews.dylib`views::BridgedNativeWidget::OnWindowKeyStatusChangedTo(this=0x00000001617be960, is_key=false) at bridged_native_widget.mm:840                                                               
    frame #3: 0x000000013b78a16c libviews.dylib`::-[ViewsNSWindowDelegate windowDidResignKey:](self=0x00000001617b4c40, _cmd="windowDidResignKey:", notification="NSWindowDidResignKeyNotification") at views_nswindow_delegate.mm:121 

I think it's worth trying to plumb calls to views::Widget::OnNativeWidgetActivationChanged _only_ via [ViewsNSWindowDelegate windowDid{Resign,Become}Main:]. Maybe this is what you're already doing :)

This may be a divergence from Aura, which seems to conflate the two. Not too much "real" code outside of ui/views depends on OnNativeWidgetActivationChanged(). And when it does, it's mainly appearance things.

However, the real problem is will be tests... A bunch of things rely on WidgetObserver::OnWidgetActivationChanged() to detect key state changes. WidgetActivationWaiter may be able to be updated to observe views::FocusManager (as well). But perhaps we want new WidgetObserver methods as well.

We'll also need to audit calls to `IsActive`, which is currently 

 bool NativeWidgetMac::IsActive() const {
   return [GetNativeWindow() isKeyWindow];
 }

E.g. Widget::SetInitialFocus(..) probably needs to keep using `isKeyWindow`.

Also note on Windows, hwnd_message_handler.cc also seems to conflate focus/active. E.g.


 bool HWNDMessageHandler::IsActive() const {
   return ::GetActiveWindow() == hwnd();
 }


but then there is

 void HWNDMessageHandler::Activate() {
   if (IsMinimized())
     ::ShowWindow(hwnd(), SW_RESTORE);
   ::SetWindowPos(hwnd(), HWND_TOP, 0, 0, 0, 0, SWP_NOSIZE | SWP_NOMOVE);
   ::SetForegroundWindow(hwnd());
 }


i.e. why doesn't Activate() invoke `SetActiveWindow()` rather than SetForegroundWindow(). (why doesn't IsActive use GetForegroundWindow?)

But note neither of these use ::SetFocus[Window]. That's only done via ClearNativeFocus. Key state changes are not plumbed through at all on Windows:

 void DesktopWindowTreeHostWin::HandleNativeFocus(HWND last_focused_window) {
   // TODO(beng): inform the native_widget_delegate_.
 }

 void DesktopWindowTreeHostWin::HandleNativeBlur(HWND focused_window) {
   // TODO(beng): inform the native_widget_delegate_.
 }


Perhaps the robust way to proceed is to introduce entirely new plumbing to handle windowDid{Resign,Become}Main: on Mac.

Then (perhaps) entirely new plumbing to handle HWNDMessageHandler::OnSetFocus(..).

Then just delete OnNativeWidgetActivationChanged for being ambiguous and confusing.

Totally agree the problem will be tests and some components that depend on activation changes, but it's not as bad as it appears thus far as these are the only failing ones at the moment:

interactive_ui_test failures
BrowserActionInteractiveTest.FocusLossClosesPopup2
    Popup dismisses on activation changes, 
    which no longer occurs if we let the window keep activation.
WidgetTestInteractive.WindowModalWindowDestroyedActivationTest
    Test looks for Window Reactivation for popup dismissal.
WidgetCaptureTest.SetCaptureToNonToplevel
BrowserActionInteractiveTest.FocusLossClosesPopup1
NativeWidgetMacInteractiveUITest.ParentWindowTrafficLights
NativeWidgetMacInteractiveUITest.BubbleDismiss
WindowActivityWatcherTest.Basic


I took a look into seeing if we can hack around this with IsAlwaysRenderAsActive(), and that will work with UI we own.

What's less clear to me is for UI we don't own (IME, emoji input). We'll likely just need to follow the activation rules in Mac for these components, which makes dismissal for bubbles a little bit more fun. 


As for the Windows fun in #19 for SetActiveWindow/SetForegroundWindow, there's a bit of an unfortunate Windows API story there (https://blogs.msdn.microsoft.com/oldnewthing/20081006-00/?p=20643).

 Issue 881170  has been merged into this issue.

 Issue 893080  has been merged into this issue.

***UI Mass Triage***

Adding labels for expert review.