Issue metadata
Sign in to add a comment
|
MacViews: other window steals focus after a page loads
Reported by
kwpol...@gmail.com,
Jul 2
|
||||||||||||||||||||||
Issue descriptionUserAgent: 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:
,
Jul 2
Yes, I’m experiencing the same thing. (It’s more apparent on slower connections.)
,
Jul 2
This also happens to me.
,
Jul 3
erikchen, can you take a look?
,
Jul 3
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();
}
"""
,
Jul 3
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?
,
Jul 3
"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.
,
Jul 3
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.
,
Jul 9
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.
,
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
,
Jul 9
,
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
,
Jul 9
,
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
,
Jul 11
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by meh...@chromium.org
, Jul 2Components: -UI Internals>Views>Desktop
Labels: Proj-MacViews
Status: Untriaged (was: Unconfirmed)
691 KB
691 KB View Download