mash: Fix shelf activate/minimize/restore on item clicks. |
||||||||
Issue descriptionmash: Fix shelf activate/minimize/restore on item clicks. In chrome --mash (1) Click the browser shelf item (or others). (2) Click it again... and again... and again... Expected: The window activates/minimizes/restores as appropriate. Actual: The window doesn't activate/minimize/restore as appropriate. Maybe focus or activation problems are causing this defect? (ie. is the window losing focus/activation when you click the shelf?) This is related to (blocking) Issue 557406 , and (blocking?) Issue 681072 . This may also be related to Issue 730887 .
,
Aug 23 2017
Hey Scott, sorry to send this bug your way, but hopefully it's an easy fix for you. Here are some notes to go along with my WIP debugging CL: https://chromium-review.googlesource.com/627588 -The CL helps, but restoring the chrome window doesn't give kbd focus. -You can create new tabs while the window is minimized (has focus or global accel? new bug if accel) -The restore-tabs bubble disappears on first minimize (in mash only, new bug if not same defect) -I'll land the ShelfWidget::activating_as_fallback_ cleanup separately.
,
Aug 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a87f95175f23fe5e855dd0a84eea79d3faa44f26 commit a87f95175f23fe5e855dd0a84eea79d3faa44f26 Author: Scott Violet <sky@chromium.org> Date: Fri Aug 25 23:18:58 2017 chromeos: make mus not request focus change on pointer events Mus doesn't really have all the information to change focus (in so far as what windows are valid focus targets and what not). Clients should be the ones requesting the focus change. BUG= 730890 TEST=covered by tests Change-Id: I531a573fe1692084accbec8e579b50b8961d46b5 Reviewed-on: https://chromium-review.googlesource.com/636445 Commit-Queue: Scott Violet <sky@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#497590} [modify] https://crrev.com/a87f95175f23fe5e855dd0a84eea79d3faa44f26/services/ui/ws/BUILD.gn [add] https://crrev.com/a87f95175f23fe5e855dd0a84eea79d3faa44f26/services/ui/ws/debug_utils.cc [add] https://crrev.com/a87f95175f23fe5e855dd0a84eea79d3faa44f26/services/ui/ws/debug_utils.h [modify] https://crrev.com/a87f95175f23fe5e855dd0a84eea79d3faa44f26/services/ui/ws/display.cc [modify] https://crrev.com/a87f95175f23fe5e855dd0a84eea79d3faa44f26/services/ui/ws/event_dispatcher.cc [modify] https://crrev.com/a87f95175f23fe5e855dd0a84eea79d3faa44f26/services/ui/ws/event_dispatcher_unittest.cc [modify] https://crrev.com/a87f95175f23fe5e855dd0a84eea79d3faa44f26/services/ui/ws/window_tree.cc [modify] https://crrev.com/a87f95175f23fe5e855dd0a84eea79d3faa44f26/services/ui/ws/window_tree_unittest.cc
,
Aug 25 2017
,
Aug 28 2017
I'm still observing some broken behavior here: 1) Start "chrome --mash", click the browser icon twice -> crash! 2) (working around CLC's crash) Chrome won't restore after minimize. 3) (working around ui::BaseWindow defects) Restoring places Chrome behind QuickLaunch. 4) (working around ui::BaseWindow defects) Minimizing closes the restore pages bubble. 5) (working around ui::BaseWindow defects) Restoring doesn't give kbd focus to omnibox. 6) (working around ui::BaseWindow defects) I also once hit focus_controller.cc's "Check failed: rules_->CanFocusWindow(window, nullptr)" via OmniboxViewViews::OnFocus with above workarounds when clicking between the omnibox, content area, and shelf button (haven't found consistent repro). I'll take this bug and work on some of the above defects, or split out some issues. I wonder why aura::Window isn't a ui::BaseWindow? Perhaps that'd help here and beyond.
,
Aug 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/88368ef41414997f0add81a50b9ac4a367aaf662 commit 88368ef41414997f0add81a50b9ac4a367aaf662 Author: Mike Wasserman <msw@chromium.org> Date: Mon Aug 28 23:42:46 2017 mash: Fix shelf button minimize crash. Use Chrome's AppList[Service|Presenter] to avoid crashes. (in the mash config, Chrome can't access Ash's ash::Shell) Bug: 730890 Test: "chrome --mash" shelf item minimize doesn't crash. Change-Id: Icc827345bf3b059b327ef38db49660b0c1b0fbb2 Reviewed-on: https://chromium-review.googlesource.com/638552 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#497939} [modify] https://crrev.com/88368ef41414997f0add81a50b9ac4a367aaf662/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
,
Aug 30 2017
Hey Scott, can you look at defect (2): Chrome won't restore after minimize My WIP CL 'fixes' (2) and (5), but breaks focus: http://crrev.com/c/642367 It seems like |DesktopWindowTreeHostMus::is_active_| tracking is broken. Also, removing a FocusController::FocusAndActivateWindow early return helps. (sadly, 'fixing' activation in this way, like my CL above, causes focus to break) It's too bad I can't quickly grok the intended behavior and fix this code myself.
,
Sep 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1a445c214a5eb2687dc283b20ca9194098ea4686 commit 1a445c214a5eb2687dc283b20ca9194098ea4686 Author: Scott Violet <sky@chromium.org> Date: Mon Sep 18 18:51:37 2017 chromeos: fixs focus related issues There are a couple of changes here: . FocusSynchronizer should reset the active focus client when focus is lost. This is effectively a signal that the active window is no longer active. . DesktopWindowTreeHostMus should key off OnActiveFocusClientChanged() to set it's active/inactive state. Mus only maintains the notion of focus and leaves activation up to clients. So, when the active focus client changes it means either the widget is gaining or losing activation. . Renamed DesktopWindowTreeHost::OnNativeWidgetActivationChanged() to OnActiveWindowChanged. The NativeWidget here is too confusing with the function of the same name on Widget and NativeWidgetDelegate. I considered OnActivationClientActiveWindowChanged() to be more specific, but that is slightly misleading because this observer isn't installed on the ActivationClient directly. . DesktopWindowTreeHostMus::OnActiveWindowChanged() needs to potentially reset the active focus client. This function is called when the active window at the wm::FocusController changes. This can correspond to a local change that needs to propagate to mus. By resetting the active focus client we ensure that happens. . DesktopWindowTreeHostMus::Deactivate() waited for async processing. I've changed it to immediately reset active status. BUG= 730890 TEST=covered by tests Change-Id: I5df638f8b73279d12c10b1492d9a1567145d140f Reviewed-on: https://chromium-review.googlesource.com/669951 Reviewed-by: Elliot Glaysher <erg@chromium.org> Commit-Queue: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#502623} [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/services/ui/ws/display.cc [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/services/ui/ws/window_tree.cc [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/aura/BUILD.gn [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/aura/mus/DEPS [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/aura/mus/focus_synchronizer.cc [add] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/aura/mus/focus_synchronizer_unittest.cc [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/aura/test/mus/change_completion_waiter.cc [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/aura/test/mus/change_completion_waiter.h [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/aura/test/mus/window_tree_client_private.cc [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/aura/test/mus/window_tree_client_private.h [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/views/mus/desktop_window_tree_host_mus.cc [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/views/mus/desktop_window_tree_host_mus.h [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/views/mus/desktop_window_tree_host_mus_unittest.cc [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/views/widget/desktop_aura/desktop_native_widget_aura.cc [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/views/widget/desktop_aura/desktop_window_tree_host.h [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/views/widget/desktop_aura/desktop_window_tree_host_win.h [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc [modify] https://crrev.com/1a445c214a5eb2687dc283b20ca9194098ea4686/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h
,
Sep 18 2017
Awesome, all the expected interactions seem to work for me on mash, thanks!!!
,
Jan 22 2018
,
Jan 23 2018
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by zork@chromium.org
, Jun 9 2017