New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 730890 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 557406
issue 681072



Sign in to add a comment

mash: Fix shelf activate/minimize/restore on item clicks.

Project Member Reported by msw@chromium.org, Jun 8 2017

Issue description

mash: 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 .
 

Comment 1 by zork@chromium.org, Jun 9 2017

Status: Assigned (was: Untriaged)

Comment 2 by msw@chromium.org, Aug 23 2017

Cc: -sky@chromium.org msw@chromium.org
Owner: sky@chromium.org
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.
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Comment 4 by sky@chromium.org, Aug 25 2017

Status: Fixed (was: Assigned)

Comment 5 by msw@chromium.org, Aug 28 2017

Cc: sky@chromium.org
Owner: msw@chromium.org
Status: Started (was: Fixed)
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.
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Comment 7 by msw@chromium.org, Aug 30 2017

Cc: e...@chromium.org sadrul@chromium.org
Owner: sky@chromium.org
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.
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Comment 9 by msw@chromium.org, Sep 18 2017

Status: Fixed (was: Started)
Awesome, all the expected interactions seem to work for me on mash, thanks!!!

Comment 10 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 11 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment