mac_views_browser: Activating a tab triggers 3 focus changes |
|||||||||||||
Issue descriptionChrome Version : 65.0.3294.5 OS Version: OS X 10.12.6 run, e.g., BrowserViewFocusTest.TabChangesAvoidSpuriousFocus On Mac, focused_classes.size() will be 4. It should be 2. The call to content::WebContentsViewMac::RestoreFocus() is triggering 3 focus events: - -[NSWindow makeFirstResponder:] first does: - -[BridgedContentView resignFirstResponder] (triggering views::FocusManager::ClearFocus()), then - -[RenderWidgetHostViewCocoa becomeFirstResponder] (triggering views::WebView::OnWebContentsFocused()); then - ChromeWebContentsViewFocusHelper::RestoreFocus() does a second views::View::RequestFocus() The content layer (WebContentsViewMac/RenderWidgetHostViewCocoa) may be trying to do too much: on views the embedder does more. Or perhaps the views embedder is doing too much and WebContentsViewAura should be doing this stuff instead.
,
Mar 23 2018
MacViews triage: robliao, let's target a fix for this at M-68.
,
Mar 27 2018
,
Mar 29 2018
** Bulk Edit ** FYI: Starting 04/13 M68 will be in canary, M68 Dev promotion will be on 04/26.
,
Apr 25 2018
Pls mark the bug as fixed if CL is landed in trunk and nothing else is pending. Thank you.
,
Jun 20 2018
,
Jul 12
,
Jul 12
,
Jul 21
,
Jul 26
,
Aug 13
,
Aug 13
,
Nov 23
,
Jan 2
,
Jan 2
This still happens locally. There are four focus transitions. Here they are: #1: WebView -> nullptr 0 libbase.dylib 0x0000000115c8677f base::debug::StackTrace::StackTrace(unsigned long) + 31 1 interactive_ui_tests 0x000000010d7b141d FocusedViewClassRecorder::OnDidChangeFocus(views::View*, views::View*) + 109 2 libviews.dylib 0x0000000115859d19 views::FocusManager::SetFocusedViewWithReason(views::View*, views::FocusManager::FocusChangeReason) + 713 3 libviews.dylib 0x000000011585a5de views::FocusManager::ClearFocus() + 30 4 interactive_ui_tests 0x000000010fa2c93c BrowserView::OnActiveTabChanged(content::WebContents*, content::WebContents*, int, int) + 268 5 interactive_ui_tests 0x000000010f84d1a4 Browser::OnActiveTabChanged(content::WebContents*, content::WebContents*, int, int) + 100 6 interactive_ui_tests 0x000000010f84cd15 Browser::OnTabStripModelChanged(TabStripModel*, TabStripModelChange const&, TabStripSelectionChange const&) + 613 7 interactive_ui_tests 0x000000010f8aac3f TabStripModel::SetSelection(ui::ListSelectionModel, TabStripModelObserver::ChangeReason, bool) + 447 8 interactive_ui_tests 0x000000010f8abf0c TabStripModel::ActivateTabAt(int, bool) + 188 9 interactive_ui_tests 0x000000010f856f29 chrome::SelectNextTab(Browser*) + 41 10 interactive_ui_tests 0x000000010f8549e5 chrome::BrowserCommandController::ExecuteCommandWithDisposition(int, WindowOpenDisposition) + 885 11 interactive_ui_tests 0x000000010f9b658b BrowserFrameMac::ExecuteCommand(int, WindowOpenDisposition, bool) + 123 12 libviews.dylib 0x00000001158aab86 non-virtual thunk to views::BridgedNativeWidgetHostImpl::ExecuteCommand(int, WindowOpenDisposition, bool, bool*) + 22 13 interactive_ui_tests 0x000000010f985615 -[ChromeCommandDispatcherDelegate prePerformKeyEquivalent:window:] + 197 14 libui_base.dylib 0x000000011662c567 -[CommandDispatcher performKeyEquivalent:] + 263 15 AppKit 0x00007fff4a8d4e0d routeKeyEquivalent + 444 16 AppKit 0x00007fff4a0e3c39 -[NSApplication(NSEvent) sendEvent:] + 1077 17 interactive_ui_tests 0x000000010e208264 __34-[BrowserCrApplication sendEvent:]_block_invoke + 196 18 libbase.dylib 0x0000000115b7dc9a base::mac::CallWithEHFrame(void () block_pointer) + 10 19 interactive_ui_tests 0x000000010e207f76 -[BrowserCrApplication sendEvent:] + 774 20 interactive_ui_tests 0x000000010e9cc4b9 ui_controls::SendKeyPressNotifyWhenDone(gfx::NativeWindow, ui::KeyboardCode, bool, bool, bool, bool, base::OnceCallback<void ()>) + 1785 21 interactive_ui_tests 0x000000010d79c795 ui_test_utils::SendKeyPressToWindowSync(gfx::NativeWindow, ui::KeyboardCode, bool, bool, bool, bool) + 133 22 interactive_ui_tests 0x000000010d7b0637 BrowserViewFocusTest_TabChangesAvoidSpuriousFocus_Test::RunTestOnMainThread() + 391 #2: WebView -> nullptr again 0 libbase.dylib 0x0000000115c8677f base::debug::StackTrace::StackTrace(unsigned long) + 31 1 interactive_ui_tests 0x000000010d7b141d FocusedViewClassRecorder::OnDidChangeFocus(views::View*, views::View*) + 109 2 libviews.dylib 0x0000000115859d19 views::FocusManager::SetFocusedViewWithReason(views::View*, views::FocusManager::FocusChangeReason) + 713 3 libviews.dylib 0x000000011585a711 views::FocusManager::StoreFocusedView(bool) + 113 4 libviews.dylib 0x000000011589e84f -[BridgedContentView resignFirstResponder] + 79 5 AppKit 0x00007fff4a22a522 -[NSWindow _realMakeFirstResponder:] + 258 6 libviews.dylib 0x000000011587b450 views::View::Focus() + 32 7 libviews.dylib 0x0000000115859c5a views::FocusManager::SetFocusedViewWithReason(views::View*, views::FocusManager::FocusChangeReason) + 522 8 interactive_ui_tests 0x000000010facf317 ChromeWebContentsViewFocusHelper::RestoreFocus() + 55 9 libcontent.dylib 0x0000000117b9d0e8 content::WebContentsViewMac::RestoreFocus() + 24 10 interactive_ui_tests 0x000000010fa2cb81 BrowserView::OnActiveTabChanged(content::WebContents*, content::WebContents*, int, int) + 849 11 interactive_ui_tests 0x000000010f84d1a4 Browser::OnActiveTabChanged(content::WebContents*, content::WebContents*, int, int) + 100 12 interactive_ui_tests 0x000000010f84cd15 Browser::OnTabStripModelChanged(TabStripModel*, TabStripModelChange const&, TabStripSelectionChange const&) + 613 13 interactive_ui_tests 0x000000010f8aac3f TabStripModel::SetSelection(ui::ListSelectionModel, TabStripModelObserver::ChangeReason, bool) + 447 14 interactive_ui_tests 0x000000010f8abf0c TabStripModel::ActivateTabAt(int, bool) + 188 15 interactive_ui_tests 0x000000010f856f29 chrome::SelectNextTab(Browser*) + 41 16 interactive_ui_tests 0x000000010f8549e5 chrome::BrowserCommandController::ExecuteCommandWithDisposition(int, WindowOpenDisposition) + 885 17 interactive_ui_tests 0x000000010f9b658b BrowserFrameMac::ExecuteCommand(int, WindowOpenDisposition, bool) + 123 18 libviews.dylib 0x00000001158aab86 non-virtual thunk to views::BridgedNativeWidgetHostImpl::ExecuteCommand(int, WindowOpenDisposition, bool, bool*) + 22 19 interactive_ui_tests 0x000000010f985615 -[ChromeCommandDispatcherDelegate prePerformKeyEquivalent:window:] + 197 20 libui_base.dylib 0x000000011662c567 -[CommandDispatcher performKeyEquivalent:] + 263 21 AppKit 0x00007fff4a8d4e0d routeKeyEquivalent + 444 22 AppKit 0x00007fff4a0e3c39 -[NSApplication(NSEvent) sendEvent:] + 1077 23 interactive_ui_tests 0x000000010e208264 __34-[BrowserCrApplication sendEvent:]_block_invoke + 196 24 libbase.dylib 0x0000000115b7dc9a base::mac::CallWithEHFrame(void () block_pointer) + 10 25 interactive_ui_tests 0x000000010e207f76 -[BrowserCrApplication sendEvent:] + 774 26 interactive_ui_tests 0x000000010e9cc4b9 ui_controls::SendKeyPressNotifyWhenDone(gfx::NativeWindow, ui::KeyboardCode, bool, bool, bool, bool, base::OnceCallback<void ()>) + 1785 27 interactive_ui_tests 0x000000010d79c795 ui_test_utils::SendKeyPressToWindowSync(gfx::NativeWindow, ui::KeyboardCode, bool, bool, bool, bool) + 133 28 interactive_ui_tests 0x000000010d7b0637 BrowserViewFocusTest_TabChangesAvoidSpuriousFocus_Test::RunTestOnMainThread() + 391 #3: nullptr -> WebView: 0 libbase.dylib 0x0000000115c8677f base::debug::StackTrace::StackTrace(unsigned long) + 31 1 interactive_ui_tests 0x000000010d7b141d FocusedViewClassRecorder::OnDidChangeFocus(views::View*, views::View*) + 109 2 libviews.dylib 0x0000000115859d19 views::FocusManager::SetFocusedViewWithReason(views::View*, views::FocusManager::FocusChangeReason) + 713 3 libcontent.dylib 0x0000000117b812a8 content::WebContentsImpl::NotifyWebContentsFocused(content::RenderWidgetHost*) + 232 4 libcontent.dylib 0x00000001179f03ee content::RenderWidgetHostImpl::GotFocus() + 30 5 libcontent.dylib 0x0000000117a13c73 content::RenderWidgetHostViewMac::OnFirstResponderChanged(bool) + 67 6 libcontent.dylib 0x0000000117c72a0e -[RenderWidgetHostViewCocoa becomeFirstResponder] + 110 7 AppKit 0x00007fff4a22a62f -[NSWindow _realMakeFirstResponder:] + 527 8 libviews.dylib 0x000000011587b450 views::View::Focus() + 32 9 libviews.dylib 0x0000000115859c5a views::FocusManager::SetFocusedViewWithReason(views::View*, views::FocusManager::FocusChangeReason) + 522 10 interactive_ui_tests 0x000000010facf317 ChromeWebContentsViewFocusHelper::RestoreFocus() + 55 11 libcontent.dylib 0x0000000117b9d0e8 content::WebContentsViewMac::RestoreFocus() + 24 12 interactive_ui_tests 0x000000010fa2cb81 BrowserView::OnActiveTabChanged(content::WebContents*, content::WebContents*, int, int) + 849 13 interactive_ui_tests 0x000000010f84d1a4 Browser::OnActiveTabChanged(content::WebContents*, content::WebContents*, int, int) + 100 14 interactive_ui_tests 0x000000010f84cd15 Browser::OnTabStripModelChanged(TabStripModel*, TabStripModelChange const&, TabStripSelectionChange const&) + 613 15 interactive_ui_tests 0x000000010f8aac3f TabStripModel::SetSelection(ui::ListSelectionModel, TabStripModelObserver::ChangeReason, bool) + 447 16 interactive_ui_tests 0x000000010f8abf0c TabStripModel::ActivateTabAt(int, bool) + 188 17 interactive_ui_tests 0x000000010f856f29 chrome::SelectNextTab(Browser*) + 41 18 interactive_ui_tests 0x000000010f8549e5 chrome::BrowserCommandController::ExecuteCommandWithDisposition(int, WindowOpenDisposition) + 885 19 interactive_ui_tests 0x000000010f9b658b BrowserFrameMac::ExecuteCommand(int, WindowOpenDisposition, bool) + 123 20 libviews.dylib 0x00000001158aab86 non-virtual thunk to views::BridgedNativeWidgetHostImpl::ExecuteCommand(int, WindowOpenDisposition, bool, bool*) + 22 21 interactive_ui_tests 0x000000010f985615 -[ChromeCommandDispatcherDelegate prePerformKeyEquivalent:window:] + 197 22 libui_base.dylib 0x000000011662c567 -[CommandDispatcher performKeyEquivalent:] + 263 23 AppKit 0x00007fff4a8d4e0d routeKeyEquivalent + 444 24 AppKit 0x00007fff4a0e3c39 -[NSApplication(NSEvent) sendEvent:] + 1077 25 interactive_ui_tests 0x000000010e208264 __34-[BrowserCrApplication sendEvent:]_block_invoke + 196 26 libbase.dylib 0x0000000115b7dc9a base::mac::CallWithEHFrame(void () block_pointer) + 10 27 interactive_ui_tests 0x000000010e207f76 -[BrowserCrApplication sendEvent:] + 774 28 interactive_ui_tests 0x000000010e9cc4b9 ui_controls::SendKeyPressNotifyWhenDone(gfx::NativeWindow, ui::KeyboardCode, bool, bool, bool, bool, base::OnceCallback<void ()>) + 1785 29 interactive_ui_tests 0x000000010d79c795 ui_test_utils::SendKeyPressToWindowSync(gfx::NativeWindow, ui::KeyboardCode, bool, bool, bool, bool) + 133 30 interactive_ui_tests 0x000000010d7b0637 BrowserViewFocusTest_TabChangesAvoidSpuriousFocus_Test::RunTestOnMainThread() + 391 #4: nullptr -> WebView: 0 libbase.dylib 0x0000000115c8677f base::debug::StackTrace::StackTrace(unsigned long) + 31 1 interactive_ui_tests 0x000000010d7b141d FocusedViewClassRecorder::OnDidChangeFocus(views::View*, views::View*) + 109 2 libviews.dylib 0x0000000115859d19 views::FocusManager::SetFocusedViewWithReason(views::View*, views::FocusManager::FocusChangeReason) + 713 3 interactive_ui_tests 0x000000010facf317 ChromeWebContentsViewFocusHelper::RestoreFocus() + 55 4 libcontent.dylib 0x0000000117b9d0e8 content::WebContentsViewMac::RestoreFocus() + 24 5 interactive_ui_tests 0x000000010fa2cb81 BrowserView::OnActiveTabChanged(content::WebContents*, content::WebContents*, int, int) + 849 6 interactive_ui_tests 0x000000010f84d1a4 Browser::OnActiveTabChanged(content::WebContents*, content::WebContents*, int, int) + 100 7 interactive_ui_tests 0x000000010f84cd15 Browser::OnTabStripModelChanged(TabStripModel*, TabStripModelChange const&, TabStripSelectionChange const&) + 613 8 interactive_ui_tests 0x000000010f8aac3f TabStripModel::SetSelection(ui::ListSelectionModel, TabStripModelObserver::ChangeReason, bool) + 447 9 interactive_ui_tests 0x000000010f8abf0c TabStripModel::ActivateTabAt(int, bool) + 188 10 interactive_ui_tests 0x000000010f856f29 chrome::SelectNextTab(Browser*) + 41 11 interactive_ui_tests 0x000000010f8549e5 chrome::BrowserCommandController::ExecuteCommandWithDisposition(int, WindowOpenDisposition) + 885 12 interactive_ui_tests 0x000000010f9b658b BrowserFrameMac::ExecuteCommand(int, WindowOpenDisposition, bool) + 123 13 libviews.dylib 0x00000001158aab86 non-virtual thunk to views::BridgedNativeWidgetHostImpl::ExecuteCommand(int, WindowOpenDisposition, bool, bool*) + 22 14 interactive_ui_tests 0x000000010f985615 -[ChromeCommandDispatcherDelegate prePerformKeyEquivalent:window:] + 197 15 libui_base.dylib 0x000000011662c567 -[CommandDispatcher performKeyEquivalent:] + 263 16 AppKit 0x00007fff4a8d4e0d routeKeyEquivalent + 444 17 AppKit 0x00007fff4a0e3c39 -[NSApplication(NSEvent) sendEvent:] + 1077 18 interactive_ui_tests 0x000000010e208264 __34-[BrowserCrApplication sendEvent:]_block_invoke + 196 19 libbase.dylib 0x0000000115b7dc9a base::mac::CallWithEHFrame(void () block_pointer) + 10 20 interactive_ui_tests 0x000000010e207f76 -[BrowserCrApplication sendEvent:] + 774 21 interactive_ui_tests 0x000000010e9cc4b9 ui_controls::SendKeyPressNotifyWhenDone(gfx::NativeWindow, ui::KeyboardCode, bool, bool, bool, bool, base::OnceCallback<void ()>) + 1785 22 interactive_ui_tests 0x000000010d79c795 ui_test_utils::SendKeyPressToWindowSync(gfx::NativeWindow, ui::KeyboardCode, bool, bool, bool, bool) + 133 23 interactive_ui_tests 0x000000010d7b0637 BrowserViewFocusTest_TabChangesAvoidSpuriousFocus_Test::RunTestOnMainThread() + 391
,
Jan 2
Interpreting these stack traces:
#1 "should happen": BrowserView::OnActiveTabChanged() should unfocus the WebContents.
#2 seems spurious: BrowserView::OnActiveTabChanged() -> WebContentsViewMac::RestoreFocus() (danger!) -> View::Focus() -> [NSWindow _realMakeFirstResponder:] -> [BridgedContentView resignFirstResponder] -> FocusManager::SetFocusedViewWithReason() - this marks the WebContents as unfocused again, because the BridgedContentView is resigning first responder in response to OnActiveTabChanged().
#3 is also spurious: [RenderWidgetHostViewCocoa becomeFirstResponder] -> RenderWidgetHostViewMac::OnFirstResponderChanged -> RenderWidgetHostImpl::GotFocus() -> WebContentsImpl::NotifyWebContentsFocused() -> FocusManager::SetFocusedViewWithReason (!)
#4 looks fine to me: BrowserView::OnActiveTabChanged() should focus the WebContents
I think the OP is right here - the middle two of these are problematic and caused by too many pieces of this code trying to synchronize too many pieces of state. In particular, I'm suspicious of this:
void WebContentsViewMac::Focus() {
if (delegate())
delegate()->ResetStoredFocus();
gfx::NativeView native_view = GetNativeViewForFocus();
NSWindow* window = [native_view.GetNativeNSView() window];
[window makeFirstResponder:native_view.GetNativeNSView()];
}
The Aura version instead does:
void WebContentsViewAura::Focus() {
if (delegate_)
delegate_->ResetStoredFocus();
if (web_contents_->GetInterstitialPage()) {
web_contents_->GetInterstitialPage()->Focus();
return;
}
if (delegate_ && delegate_->Focus())
return;
RenderWidgetHostView* rwhv =
web_contents_->GetFullscreenRenderWidgetHostView();
if (!rwhv)
rwhv = web_contents_->GetRenderWidgetHostView();
if (rwhv)
rwhv->Focus();
}
ignoring the GetInterstitialPage() and delegate_->Focus() cases (which handle various special cases - a tab modal being present, etc), this ultimately ends up falling through to RenderWidgetHostView::Focus. In Aura, this does:
void RenderWidgetHostViewAura::Focus() {
// Make sure we have a FocusClient before attempting to Focus(). In some
// situations we may not yet be in a valid Window hierarchy (such as reloading
// after out of memory discarded the tab).
aura::client::FocusClient* client = aura::client::GetFocusClient(window_);
if (client)
window_->Focus();
}
In RenderWidgetHostViewMac, it does:
void RenderWidgetHostViewMac::Focus() {
ns_view_bridge_->MakeFirstResponder();
}
It's possible that all that is needed here is for WebContentsViewMac to behave the same way as WebContentsViewAura?
Over to avi@ for focus nastiness.
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by gov...@chromium.org
, Feb 8 2018