ws: SetFocus fails on non-visible KSV content window |
||||
Issue descriptionws: SetFocus fails on non-visible KSV content window On ToT #582215 local debug builds of linux-chromeos (1) Run linux-chromeos with logging: out/Debug/chrome --vmodule=*/ws2/*=3 (2) Open the Keyboard Shortcut Viewer (Ctrl-Alt-/) Expected: No focus failures Actual: SetFocus fails The logged failure is: [...focus_handler.cc(30)] SetFocus failed (access denied or invalid window) The client focuses its DesktopWindowTreeHostMus::content_window before its visible here: #2 aura::FocusSynchronizer::SetFocusedWindow() #3 aura::FocusSynchronizer::OnWindowFocused() #4 wm::FocusController::SetFocusedWindow() #5 wm::FocusController::FocusAndActivateWindow() #6 wm::FocusController::FocusWindow() #7 wm::FocusController::ActivateWindow() #8 views::DesktopNativeWidgetAura::HandleActivationChanged() #9 views::DesktopWindowTreeHostMus::OnActiveFocusClientChanged() #10 aura::FocusSynchronizer::SetActiveFocusClient() #11 views::DesktopWindowTreeHostMus::Activate() #12 views::DesktopWindowTreeHostMus::ShowWindowWithState() #13 views::DesktopNativeWidgetAura::ShowWithWindowState() #14 views::Widget::Show() #15 keyboard_shortcut_viewer::KeyboardShortcutView::Toggle() The failure causes the client to revert window focus to the root window. AFAICT, there are no user-facing defects (can still click/touch/type in content window) As such, this isn't really a blocker, but it'd be good to understand the issue. This possible fix shows the content window immediately after the root window in DesktopWindowTreeHostMus::ShowWindowWithState: https://chromium-review.googlesource.com/c/chromium/src/+/1169689 (it only calls Show conditionally, for some Mash omnibox opacity animation) Scott, I hope you can find the right fix here. Thanks!
,
Aug 13
I'm worried this may be hiding some real focus issues. Upping priority.
,
Aug 14
,
Aug 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013 commit 93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013 Author: Scott Violet <sky@chromium.org> Date: Wed Aug 15 16:36:33 2018 views: cleanup NativeWidgetPrivate::Show()/DesktopWindowTreeHost::Show() NativeWidgetPrivate defined three related functions for show: . Show() . ShowMaximizedWithBounds() . ShowWindowWithState() The actual implementation of these generally funnelled into a single function. To make matters equally interesting DesktopWindowTreeHost had these variants: 1. WindowTreeHost::Show(): this comes by virture of DesktopWindowTreeHost having (or more commonly subclassing) a WindowTreeHost. 2. ShowWindowWithState() 3. ShowMaximizedWithBounds() As mentioned, 1 comes from WindowTreeHost. WindowTreeHost::Show() invokes the pure virtual method WindowTreeHost::ShowImpl(). The implementation of ShowImpl() is typically taken to mean show with a state of ui::SHOW_STATE_NORMAL. Because ShowImpl() is taken to mean SHOW_STATE_NORMAL, the implementations of DesktopWindowTreeHost::ShowWindowWithState()/ShowMaximizedWithBounds() can *not* call WindowTreeHost::Show() (otherwise we would end up in a loop of Show() -> ShowImpl()-> ShowWindowWithState() -> Show()...). This leads to the following pattern: 1. DesktopWindowTreeHost implementations of Show* do not call WindowTreeHost, but do the work in WindowTreeHost::Show(). 2. DesktopWindowTreeHost::ShowImpl() calls Show with a particular state. In an attempt to simplify things I've introduced a single function on NativeWidgetPrivate and DesktopWindowTreeHost. There is still the quirk that the code deals with WindowTreeHost::Show() being called outside of through Widget. This is unfortunate, but I kept it. I also made each DesktopWindowTreeHost implementation responsible for showing the appropriate windows. This means we don't end up with the double show that was causing problems. I also added some minor debugging code to ash. My knee jerk reaction is this pain/complexity is because WindowTreeHost has a public Show() function. If WindowTreeHost did not have a Show() function this code would be slightly less subtle. Rethinking WindowTreeHost::Show() is for another day. BUG= 873701 TEST=covered by tests Change-Id: Id3525e7bc61d741cd7239fc466ca0ac16a19eab6 Reviewed-on: https://chromium-review.googlesource.com/1174910 Commit-Queue: Scott Violet <sky@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#583278} [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ash/accelerators/debug_commands.cc [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/chrome/browser/ui/views/apps/app_window_desktop_native_widget_aura_win.cc [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/mus/desktop_window_tree_host_mus.cc [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/mus/desktop_window_tree_host_mus.h [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/desktop_aura/desktop_native_widget_aura.cc [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/desktop_aura/desktop_native_widget_aura.h [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/desktop_aura/desktop_native_widget_aura_unittest.cc [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/desktop_aura/desktop_window_tree_host.h [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/desktop_aura/desktop_window_tree_host_platform.cc [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/desktop_aura/desktop_window_tree_host_platform.h [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/desktop_aura/desktop_window_tree_host_win.h [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/native_widget_aura.cc [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/native_widget_aura.h [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/native_widget_mac.h [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/native_widget_mac.mm [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/native_widget_private.h [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/widget/widget.cc [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/win/hwnd_message_handler.cc [modify] https://crrev.com/93f4eeab1e3346d9b26cf0f7d8ccca13ce0cb013/ui/views/win/hwnd_message_handler.h
,
Aug 15
|
||||
►
Sign in to add a comment |
||||
Comment 1 by msw@chromium.org
, Aug 13