New issue
Advanced search Search tips

Issue 859614 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

MacViews: other window steals focus after a page loads

Reported by kwpol...@gmail.com, Jul 2

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3472.3 Safari/537.36

Steps to reproduce the problem:
1. Open two windows.
2. Navigate to a page in window 2 and cmd-` back to window 1.
3. After the page in window 2 loads, it unexpectedly steals focus.

What is the expected behavior?

What went wrong?
MacViews.

Did this work before? Yes Works with Cocoa mode.

Chrome version: 69.0.3472.3  Channel: dev
OS Version: OS X 10.13.5
Flash Version:
 
Cc: erikc...@chromium.org ccameron@chromium.org
Components: -UI Internals>Views>Desktop
Labels: Proj-MacViews
Status: Untriaged (was: Unconfirmed)
Thanks for filing this issue. Yes, I can reproduce it too. (I hope, this is exactly what you mean - see the steps below).

STR:

1.) open two windows
2.) type apple.com in the foreground windows' (2) Omnibox and press enter
3.) press immediately cmd-` to cycle to window (1) during apple.com is loading in window (2) which is in the background now.

Actual: Window (2) with apple.com comes back to the foreground.

Expected: Window (1) should stay in the foreground.

A screencast is attached.

screencast.mov
691 KB View Download
Yes, I’m experiencing the same thing. (It’s more apparent on slower connections.)
Cc: ellyjo...@chromium.org
Labels: -Pri-2 ReleaseBlock-Stable M-69 Pri-1
This also happens to me. 
Owner: erikc...@chromium.org
Status: Assigned (was: Untriaged)
erikchen, can you take a look?
This is the call to makeKeyAndOrderFront: that is causing the switch.

"""
0   libbase.dylib                       0x000000010a555fce base::debug::StackTrace::StackTrace(unsigned long) + 174
1   libbase.dylib                       0x000000010a55608d base::debug::StackTrace::StackTrace(unsigned long) + 29
2   libbase.dylib                       0x000000010a1766cc base::debug::StackTrace::StackTrace() + 28
3   libchrome_dll.dylib                 0x00000001148079fa -[AppController windowDidBecomeMain:] + 58
4   CoreFoundation                      0x00007fff28bfb28c __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
5   CoreFoundation                      0x00007fff28bfb17a _CFXRegistrationPost + 442
6   CoreFoundation                      0x00007fff28bfaec2 ___CFXNotificationPost_block_invoke + 50
7   CoreFoundation                      0x00007fff28bb9af2 -[_CFXNotificationRegistrar find:object:observer:enumerator:] + 1826
8   CoreFoundation                      0x00007fff28bb8b8c _CFXNotificationPost + 652
9   Foundation                          0x00007fff2acb7467 -[NSNotificationCenter postNotificationName:object:userInfo:] + 66
10  AppKit                              0x00007fff262e0017 -[NSWindow _changeKeyAndMainLimitedOK:] + 940
11  AppKit                              0x00007fff2638d676 -[NSWindow _makeKeyRegardlessOfVisibility] + 108
12  AppKit                              0x00007fff262e3018 NSPerformVisuallyAtomicChange + 146
13  AppKit                              0x00007fff2638d5ad -[NSWindow makeKeyAndOrderFront:] + 90
14  libcontent.dylib                    0x000000012c1e4469 content::WebContentsViewMac::Focus() + 201
15  libcontent.dylib                    0x000000012c18c798 content::WebContentsImpl::Focus() + 56
16  libwebview.dylib                    0x00000001440354a3 views::WebView::OnFocus() + 131
17  libwebview.dylib                    0x00000001440358a0 views::WebView::RenderViewHostChanged(content::RenderViewHost*, content::RenderViewHost*) + 80
18  libcontent.dylib                    0x000000012c1976e5 content::WebContentsImpl::NotifyViewSwapped(content::RenderViewHost*, content::RenderViewHost*) + 181
19  libcontent.dylib                    0x000000012c19f439 content::WebContentsImpl::NotifySwappedFromRenderManager(content::RenderFrameHost*, content::RenderFrameHost*, bool) + 137
20  libcontent.dylib                    0x000000012b365805 content::RenderFrameHostManager::CommitPending() + 3749
21  libcontent.dylib                    0x000000012b363f22 content::RenderFrameHostManager::CommitPendingIfNecessary(content::RenderFrameHostImpl*, bool, bool) + 514
22  libcontent.dylib                    0x000000012b363d05 content::RenderFrameHostManager::DidNavigateFrame(content::RenderFrameHostImpl*, bool, bool) + 85
23  libcontent.dylib                    0x000000012b2a64f2 content::NavigatorImpl::DidNavigate(content::RenderFrameHostImpl*, FrameHostMsg_DidCommitProvisionalLoad_Params const&, std::__1::unique_ptr<content::NavigationHandleImpl, std::__1::default_delete<content::NavigationHandleImpl> >, bool) + 546
24  libcontent.dylib                    0x000000012b2d7f74 content::RenderFrameHostImpl::DidCommitNavigationInternal(FrameHostMsg_DidCommitProvisionalLoad_Params*, bool) + 2996
25  libcontent.dylib                    0x000000012b2d7071 content::RenderFrameHostImpl::DidCommitProvisionalLoad(std::__1::unique_ptr<FrameHostMsg_DidCommitProvisionalLoad_Params, std::__1::default_delete<FrameHostMsg_DidCommitProvisionalLoad_Params> >, mojo::InterfaceRequest<service_manager::mojom::InterfaceProvider>) + 2305
26  libcontent.dylib                    0x00000001295c3404 content::mojom::FrameHostStubDispatch::Accept(content::mojom::FrameHost*, mojo::Message*) + 2228
27  libcontent.dylib                    0x000000012b30f583 content::mojom::FrameHostStub<mojo::RawPtrImplRefTraits<content::mojom::FrameHost> >::Accept(mojo::Message*) + 83
28  libbindings.dylib                   0x000000010d128a76 mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message*) + 6214
29  libbindings.dylib                   0x000000010d127221 mojo::InterfaceEndpointClient::HandleIncomingMessageThunk::Accept(mojo::Message*) + 33
30  libbindings.dylib                   0x000000010d12555b mojo::FilterChain::Accept(mojo::Message*) + 795
31  libbindings.dylib                   0x000000010d12c436 mojo::InterfaceEndpointClient::HandleIncomingMessage(mojo::Message*) + 230
"""

The problem is that Focus has 2 different semantics: 
  1) Update the first responder of a window
  2) Make the window have focus.

"""
void WebView::RenderViewHostChanged(content::RenderViewHost* old_host,
                                    content::RenderViewHost* new_host) {
  if (HasFocus())
    OnFocus();
  NotifyAccessibilityWebContentsChanged();
}
"""

Here, HasFocus() is defined in terms of (1), e.g.

"""
bool View::HasFocus() const {
  const FocusManager* focus_manager = GetFocusManager();
  return focus_manager && (focus_manager->GetFocusedView() == this);
}
"""

But the implementation of WebContentsView::Focus() uses both (1) and (2):
"""
void WebContentsViewMac::Focus() {
  if (delegate())
    delegate()->ResetStoredFocus();

  gfx::NativeView native_view = GetNativeViewForFocus();
  NSWindow* window = [native_view window];
  [window makeFirstResponder:native_view];
  if (![window isVisible])
    return;
  [window makeKeyAndOrderFront:nil];
}
"""

This is pretty much the same behavior for WebContentsViewAura:
"""
WebContentsViewAura::Focus() {
  ...
  if (!rwhv)
    rwhv = web_contents_->GetRenderWidgetHostView();
  if (rwhv)
    rwhv->Focus();
}

RenderWidgetHostViewAura::Focus() {
  aura::client::FocusClient* client = aura::client::GetFocusClient(window_);
  if (client)
    window_->Focus();
}
"""


Cc: a...@chromium.org
avi: I think the right approach is to create:
WebContents::FocusWindow() and WebContents::UpdateFocusWithinWindow(). And then update callers to be explicit about what they want.

Then we would update WebView::OnFocus to call WebContents::UpdateFocusWithinWindow(). wdyt?
"The problem is that Focus has 2 different semantics: 
  1) Update the first responder of a window
  2) Make the window have focus."

The second, "make the window have focus", is something that WebContents should never be doing. It has a delegate call, WebContentsDelegate::ActivateContents(), that it uses to ask its embedder to do that.

We should make Focus only do (1) and then ask the embedder via ActivateContents() to do the second.
This bug also appears on aura, it's just harder to trigger. Adding a 1-second delay to RenderWidgetHostViewAura::Focus makes it easy to see. 

The logic for WebContentsViewMac was added back in 2009:
https://codereview.chromium.org/165492/
with the reasoning: 

"""
There was an implicit assumption on Windows that TabContentsViewWin::Focus caused the window containing the TabContents to be foregrounded. This is because on Windows a HWND is focused with a call to SetFocus, which activates the containing top level window. On Mac, TabContentsViewMac::Focus needs to explicitly activate the containing window.
"""

So the problem is that TabContentsViewMac::Focus was modified to match the behavior of Windows. And the current implementation still reflects that old implementation. That means that we [probably] can't just change WebContentsViewMac::Focus without also changing WebContentsViewAura::Focus, and to do that we probably need to update many of the callsites. Furthermore, if there's behavior that's still relying on this windows-specific limitation, then we'll need to add an abstraction that allows us to set the view-element to be focused, the next time the window is activated, which would allow code to focus a view without activating the window on Windows.
Updating WebContentsViewMac::Focus to not call "[window makeKeyAndOrderFront:nil];" passes all tests.

However, updating WebContentsViewAura::Focus to not call RenderWidgetHostViewAura::Focus() [which has the same effects] causes lots of test errors.

I guess I'll just remove the call from WebContentsViewMac::Focus.
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/292ad04dc764c3ae4643f71f75b13afa7da4c745

commit 292ad04dc764c3ae4643f71f75b13afa7da4c745
Author: erikchen <erikchen@chromium.org>
Date: Mon Jul 09 17:17:22 2018

Remove makeKeyAndOrderFront: from WebContentsViewMac::Focus().

WebContentsView::Focus() is supposed to update the first responder of the
Window, not make the window focused. The latter should go through
WebContentsDelegate::ActivateContents().

This logic is usually a no-op because the window receiving the call is the
active window. But in the case where the user quickly switches windows, this can
cause the old window to re-take focus.

This logic was added in 2009 not because it had the right semantics, but to
match the behavior of Windows: http://codereview.chromium.org/165492.

Bug:  859614 
Change-Id: I2bc44a825d93159d774c632da17cc2f2d151fc3a
Reviewed-on: https://chromium-review.googlesource.com/1124772
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573344}
[modify] https://crrev.com/292ad04dc764c3ae4643f71f75b13afa7da4c745/content/browser/web_contents/web_contents_view_mac.mm

Status: Fixed (was: Assigned)
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b2fa79b83193e3aa618798a44a397025ec8fe8ae

commit b2fa79b83193e3aa618798a44a397025ec8fe8ae
Author: Philip Rogers <pdr@chromium.org>
Date: Mon Jul 09 23:24:16 2018

Revert "Remove makeKeyAndOrderFront: from WebContentsViewMac::Focus()."

This reverts commit 292ad04dc764c3ae4643f71f75b13afa7da4c745.

Reason for revert: I think this caused an active window test failure, see  https://crbug.com/861963 

Original change's description:
> Remove makeKeyAndOrderFront: from WebContentsViewMac::Focus().
> 
> WebContentsView::Focus() is supposed to update the first responder of the
> Window, not make the window focused. The latter should go through
> WebContentsDelegate::ActivateContents().
> 
> This logic is usually a no-op because the window receiving the call is the
> active window. But in the case where the user quickly switches windows, this can
> cause the old window to re-take focus.
> 
> This logic was added in 2009 not because it had the right semantics, but to
> match the behavior of Windows: http://codereview.chromium.org/165492.
> 
> Bug:  859614 
> Change-Id: I2bc44a825d93159d774c632da17cc2f2d151fc3a
> Reviewed-on: https://chromium-review.googlesource.com/1124772
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573344}

TBR=avi@chromium.org,erikchen@chromium.org

Change-Id: I6ae990055c9a9cf0bd84f7599939e25490178304
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  859614 
Reviewed-on: https://chromium-review.googlesource.com/1130141
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573517}
[modify] https://crrev.com/b2fa79b83193e3aa618798a44a397025ec8fe8ae/content/browser/web_contents/web_contents_view_mac.mm

Status: Assigned (was: Fixed)
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b73824ff37506f377dc9c9d6789d1db74e6ca5f6

commit b73824ff37506f377dc9c9d6789d1db74e6ca5f6
Author: erikchen <erikchen@chromium.org>
Date: Wed Jul 11 14:45:51 2018

[Reland #1] Remove makeKeyAndOrderFront: from WebContentsViewMac::Focus().

The original CL caused a test failure in
DetachToBrowserTabDragControllerTest.DetachToOwnWindow. This was due to a bug in
the test itself, which was generating an incorrect mouse-up event. This CL fixes
the test to delay generation of the mouse-up event until the new tab dragging
window has been detached.

> WebContentsView::Focus() is supposed to update the first responder of the
> Window, not make the window focused. The latter should go through
> WebContentsDelegate::ActivateContents().
>
> This logic is usually a no-op because the window receiving the call is the
> active window. But in the case where the user quickly switches windows, this can
> cause the old window to re-take focus.
>
> This logic was added in 2009 not because it had the right semantics, but to
> match the behavior of Windows: http://codereview.chromium.org/165492.
>
> Bug:  859614 
> Change-Id: I2bc44a825d93159d774c632da17cc2f2d151fc3a
> Reviewed-on: https://chromium-review.googlesource.com/1124772
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573344}

Bug:  859614 
Change-Id: I441a00ad015eb3559718b448fc42f98827beee35
Reviewed-on: https://chromium-review.googlesource.com/1132106
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574159}
[modify] https://crrev.com/b73824ff37506f377dc9c9d6789d1db74e6ca5f6/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
[modify] https://crrev.com/b73824ff37506f377dc9c9d6789d1db74e6ca5f6/content/browser/web_contents/web_contents_view_mac.mm

Status: Fixed (was: Assigned)

Sign in to add a comment