Tablet WM : Swiping on system tray bubble.

ash/system/tray/system_tray.cc

Index: ash/system/tray/system_tray.cc
diff --git a/ash/system/tray/system_tray.cc b/ash/system/tray/system_tray.cc
index 78f2929147dab3951ce59e4d5e0d922ef7483b9c..210406476cb9d705fc1ea0a755881fc95bf4d8d5 100644
--- a/ash/system/tray/system_tray.cc
+++ b/ash/system/tray/system_tray.cc
@@ -50,6 +50,7 @@
 #include "ash/system/user/tray_user.h"
 #include "ash/system/web_notification/web_notification_tray.h"
 #include "ash/wm/container_finder.h"
+#include "ash/wm/maximize_mode/maximize_mode_controller.h"
 #include "ash/wm/widget_finder.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -172,8 +173,10 @@ class SystemTray::ActivationObserver : public ::wm::ActivationChangeObserver {
     int container_id = wm::GetContainerForWindow(gained_active)->id();
 
     // Don't close the bubble if a popup notification is activated.
-    if (container_id == kShellWindowId_StatusContainer)
+    if (container_id == kShellWindowId_StatusContainer ||
+        container_id == kShellWindowId_SettingBubbleContainer) {
       return;
+    }
 
     views::Widget* bubble_widget =
         tray_->GetSystemBubble()->bubble_view()->GetWidget();
@@ -198,7 +201,8 @@ class SystemTray::ActivationObserver : public ::wm::ActivationChangeObserver {
 
 // SystemTray
 
-SystemTray::SystemTray(Shelf* shelf) : TrayBackgroundView(shelf) {
+SystemTray::SystemTray(Shelf* shelf)
+    : TrayBackgroundView(shelf), shelf_(shelf) {
   SetInkDropMode(InkDropMode::ON);
 
   // Since user avatar is on the right hand side of System tray of a
@@ -630,6 +634,21 @@ void SystemTray::ActivateBubble() {
   bubble_view->GetWidget()->Activate();
 }
 
+void SystemTray::OnGestureEvent(ui::GestureEvent* event) {
+  if (Shell::Get()
+          ->maximize_mode_controller()
+          ->IsMaximizeModeWindowManagerEnabled() &&
+      shelf_->IsHorizontalAlignment() && ProcessGestureEvent(*event)) {
+    event->SetHandled();
+  } else {
+    TrayBackgroundView::OnGestureEvent(event);
+  }
+}
+
+gfx::Rect SystemTray::GetWorkAreaBoundsInScreen() const {
+  return shelf_->GetUserWorkAreaBounds();
+}
+
 bool SystemTray::PerformAction(const ui::Event& event) {
   // If we're already showing a full system tray menu, either default or
   // detailed menu, hide it; otherwise, show it (and hide any popup that's
@@ -644,6 +663,94 @@ bool SystemTray::PerformAction(const ui::Event& event) {
   return true;
 }
 
+bool SystemTray::ProcessGestureEvent(const ui::GestureEvent& event) {
+  if (event.type() == ui::ET_GESTURE_SCROLL_BEGIN)
+    return StartGestureDrag(event);
+
+  if (!HasSystemBubble())
+    return false;
+
+  if (event.type() == ui::ET_GESTURE_SCROLL_UPDATE) {
+    UpdateGestureDrag(event);
+    return true;
+  }
+
+  if (event.type() == ui::ET_GESTURE_SCROLL_END ||
+      event.type() == ui::ET_SCROLL_FLING_START) {
+    CompleteGestureDrag(event);
+    return true;
+  }
+  gesture_drag_amount_ = 0.f;

[tdanderson, 2017/06/22 20:21:42]

You should also reset this member upon completing a gesture drag, I believe (so,
right after line 680).

After thinking about this some more, I think the 'safer' thing to do would be to
have a |is_in_drag_| member or similar. But we can come back to that idea in a
future CL since you will presumably be refactoring this code in the near future
anyway. In the meanwhile can you please add a "// TODO(minch|tdanderson):
Consider adding a boolean member to track whether or not a drag is in progress"

[minch1, 2017/06/23 22:39:56]

I am not sure following cl will change this or not. Let me do it in this cl.
Actually, I found it is not correct to set get_drag_amount_= 0.f here. Since if
there is an event happened between SCROLL_UPDATE and SCROLL_END / FLING_START
(this is possible, eg.ET_GESTURE_DRAG). It will reset this value which will lead
to wrong completion of the dragging.
I think it is enough to reset is_in_drag_ in CompleteGestureDrag. Since there is
always SCROLL_END / FLING_START for a SCROLL_START.

[tdanderson, 2017/06/26 18:13:01]

Acknowledged.

+  return false;
+}
+
+bool SystemTray::StartGestureDrag(const ui::GestureEvent& gesture) {
+  // Close the system bubble if there is already a full one opened. And return
+  // false to let shelf handle the event.
+  if ((HasSystemBubble() && full_system_tray_menu_)) {

[tdanderson, 2017/06/22 20:21:42]

nit: unnecessary extra ()

[minch1, 2017/06/23 22:39:56]

Done.

+    system_bubble_->bubble()->Close();

[tdanderson, 2017/06/22 20:21:42]

I tried this patch set locally, and as far as I can tell adding just this line
is sufficient to fix the crash. I don't think you need to make any changes to
tray_event_filter.cc. Can you please verify?

[minch1, 2017/06/23 22:39:56]

I think that should because you only swiping on the system tray area. If  you
swiping on other overlap area of system tray bubble and shelf during animation,
it will cause crash.
If you want to repro this, I think you can try to 
1.set |kAnimationDurationMS| longer (eg, 2000).
2.swiping on the system tray ends with aiming close the bubble, eg, ends with
fling down with enough velocity.
3.before animation finish, swiping on the overlap area of the bubble and shelf.
Crash happened.
Another case is that if you keep dragging the bubble (not release the finger),
and then swiping / touch the overlap area of the bubble and shelf with another
finger at the same time, you will find that this will not dismiss the bubble.
This is not consistent when you touch elsewhere (eg.desktop) in the same
situation.
I think it should close the bubble in this situation. So I changed some logic in
tray_event_filter.cc
I also added a UT for this, please check.

[tdanderson, 2017/06/26 18:13:01]

OK, this makes sense, thank you for the explanation and following through with
this.

+    return false;
+  }
+
+  // If the scroll sequence begins to scroll downward, return false so that the
+  // event will instead by handled by the shelf.
+  if (gesture.details().scroll_y_hint() > 0)
+    return false;
+
+  ShellPort::Get()->RecordUserMetricsAction(
+      UMA_STATUS_AREA_SYSTEM_TRAY_SWIPING);
+
+  gesture_drag_amount_ = 0.f;
+  ShowDefaultView(BUBBLE_CREATE_NEW);
+  system_tray_bubble_bounds_ =
+      system_bubble_->bubble_view()->GetWidget()->GetWindowBoundsInScreen();
+
+  gfx::Point location(gesture.location());
+  SetBubbleBounds(&location);
+  return true;
+}
+
+void SystemTray::UpdateGestureDrag(const ui::GestureEvent& gesture) {
+  gfx::Point location(gesture.location());
+  SetBubbleBounds(&location);
+  gesture_drag_amount_ += gesture.details().scroll_y();
+}
+
+void SystemTray::CompleteGestureDrag(const ui::GestureEvent& gesture) {
+  const bool hide_bubble = !ShouldShowSystemBubbleAfterScrollSequence(gesture);
+  gfx::Rect target_bounds = system_tray_bubble_bounds_;
+
+  if (hide_bubble)
+    target_bounds.set_y(shelf_->GetIdealBounds().y());
+
+  system_bubble_->bubble()->AnimateToTargetBounds(target_bounds, hide_bubble);
+}
+
+void SystemTray::SetBubbleBounds(gfx::Point* location) {

[tdanderson, 2017/06/22 20:21:42]

There doesn't seem to be any need for this parameter to be a pointer type, since
the call sites never access the mutated point after SetBubbleBounds()
terminates. I suggest instead:

Call sites just look like:
SetBubbleBounds(gesture.location());

Then:
void SystemTray::SetBubbleBounds(const gfx::Point& location) {
  gfx::Point* location_in_screen_coordinates(location);
  ConvertPointToScreen(this, &location_in_screen_coordinates);
  ...
}

[minch1, 2017/06/23 22:39:57]

Done.

+  View::ConvertPointToScreen(this, location);
+
+  // System tray bubble should not be dragged higher than its original height.
+  if (location->y() < system_tray_bubble_bounds_.y())
+    location->set_y(system_tray_bubble_bounds_.y());
+
+  gfx::Rect bounds_on_location = system_tray_bubble_bounds_;
+  bounds_on_location.set_y(location->y());
+  system_bubble_->bubble_view()->GetWidget()->SetBounds(bounds_on_location);
+}
+
+bool SystemTray::ShouldShowSystemBubbleAfterScrollSequence(
+    const ui::GestureEvent& gesture) {
+  // If the scroll sequence terminates with a fling, show the system menu if the
+  // fling was fast enough and in the correct direction.
+  if (gesture.type() == ui::ET_SCROLL_FLING_START &&
+      fabs(gesture.details().velocity_y()) > kFlingVelocity) {
+    return gesture.details().velocity_y() < 0;
+  }
+
+  // Show the system menu if it is already at least one-third visible.
+  return -gesture_drag_amount_ >= system_tray_bubble_bounds_.height() / 3.0;
+}
+
 void SystemTray::CloseSystemBubbleAndDeactivateSystemTray() {
   activation_observer_.reset();
   key_event_watcher_.reset();