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..1361abd5149445d4179588b3af5b04da84eeb018 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,13 +201,19 @@ 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
   // horizontal shelf and that is sufficient to indicate separation, no
   // separator is required.
   set_separator_visibility(false);
+
+  Shell::Get()->AddShellObserver(this);
+  in_maximize_mode_ = Shell::Get()
+                          ->maximize_mode_controller()
+                          ->IsMaximizeModeWindowManagerEnabled();
 }
 
 SystemTray::~SystemTray() {
@@ -214,6 +223,7 @@ SystemTray::~SystemTray() {
   system_bubble_.reset();
   for (const auto& item : items_)
     item->OnTrayViewDestroyed();
+  Shell::Get()->RemoveShellObserver(this);
 }
 
 void SystemTray::InitializeTrayItems(
@@ -589,6 +599,14 @@ void SystemTray::HideBubble(const TrayBubbleView* bubble_view) {
   HideBubbleWithView(bubble_view);
 }
 
+void SystemTray::OnMaximizeModeStarted() {
+  in_maximize_mode_ = true;
+}
+
+void SystemTray::OnMaximizeModeEnded() {
+  in_maximize_mode_ = false;
+}
+
 void SystemTray::CloseBubble(const ui::KeyEvent& key_event) {
   CloseSystemBubble();
 }
@@ -630,6 +648,19 @@ void SystemTray::ActivateBubble() {
   bubble_view->GetWidget()->Activate();
 }
 
+void SystemTray::OnGestureEvent(ui::GestureEvent* event) {

[tdanderson, 2017/06/19 16:19:38]

Why are you restricting this to just a horizontally-aligned shelf?

[minch1, 2017/06/20 16:45:34]

Since tablet will has horizontal shelf only in the future.

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

Acknowledged.

+  if (in_maximize_mode_ && shelf_->IsHorizontalAlignment() &&

[tdanderson, 2017/06/19 16:19:38]

Side question to ask UX (OK to handle in a follow-on CL): since we're letting
the user open the system menu with a swipe-up, should we also be letting them
close the system menu with a swipe-down on a menu that is already open?

[minch1, 2017/06/20 16:45:34]

Yes, that's right. We definitely need that, I will do that in next CL.

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

Acknowledged.

+      ProcessGestureEvent(*event)) {
+    event->StopPropagation();
+  } else {
+    CustomButton::OnGestureEvent(event);

[tdanderson, 2017/06/19 16:19:38]

I think you'd instead want to call up to TrayBackrgroundView::OnGestureEvent()
since SystemTray inherits directly from TrayBackgroundView. 

If you were to call up to CustomButton::OnGestureEvent() and at some point in
the future someone added a TrayBackgroundView::OnGestureEvent() or
ActionableView::OnGestureEvent() override, these overrides would be skipped when
a gesture targeted SystemTray.

[minch1, 2017/06/20 16:45:34]

Done.

+  }
+}
+
+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 +675,98 @@ bool SystemTray::PerformAction(const ui::Event& event) {
   return true;
 }
 
+bool SystemTray::ProcessGestureEvent(const ui::GestureEvent& event) {

[tdanderson, 2017/06/19 16:19:38]

When building and trying out this CL locally, I can get a crash when I do the
following when the system menu is closed: without your finger leaving the
screen, very quickly swipe up on the system menu, then down, then up, then down
again.

One possible cause for this crash could be that you are not handling all
relevant possibilities for gesture events. For instance you may be receiving
ET_SCROLL_FLING_CANCEL, ET_GESTURE_SWIPE, ET_GESTURE_END, etc. I suggest adding
logging to this function to see which event types and sequences can be seen.

[minch1, 2017/06/20 16:45:34]

I am sorry I cannot repro this locally. But I guess this maybe the same reason
as another crash following (one finger tap the bubble then another finger tap
elsewhere). Quickly swiping maybe touch the desktop by accident. All the other
gesture events will return false here directly.
Let me know if you can still repro the crash after my change.

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

I can still repro locally with Patch Set 8. I will share/send you a video of
what I'm seeing.

+  if (event.type() == ui::ET_GESTURE_SCROLL_BEGIN)

[xdai1, 2017/06/19 19:01:30]

I think "switch case" might make the code cleaner and easier to follow.

[minch1, 2017/06/20 16:45:34]

I think "switch case" is not so good after update. Please check.

+    return StartGestureDrag(event);
+
+  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;
+  }
+  return false;
+}
+
+bool SystemTray::StartGestureDrag(const ui::GestureEvent& gesture) {
+  if ((HasSystemBubble() && full_system_tray_menu_) ||
+      IsDraggingDownOnShelf(gesture)) {

[tdanderson, 2017/06/19 16:19:38]

I don't understand what the check IsDraggingDownOnShelf() does and why you need
it. Can you please explain?

[minch1, 2017/06/20 16:45:34]

When there is no bubble. Swiping down on the system tray should actor the same
as if swiping down on the other positions on the shelf to hide the shelf.
If I didn't check this, swiping down on the system tray at the beginning will
not hide the shelf.

+    return false;
+  }
+
+  gesture_drag_amount_ = 0.f;
+  ShowDefaultView(BUBBLE_CREATE_NEW);
+  system_tray_bubble_bounds_ =
+      system_bubble_->bubble_view()->GetWidget()->GetWindowBoundsInScreen();
+  UpdateBoundsOnDragging(gesture);
+  return true;
+}
+
+void SystemTray::UpdateGestureDrag(const ui::GestureEvent& gesture) {
+  UpdateBoundsOnDragging(gesture);
+  gesture_drag_amount_ += gesture.details().scroll_y();
+}
+
+void SystemTray::CompleteGestureDrag(const ui::GestureEvent& gesture) {
+  gfx::Rect target_bounds = system_tray_bubble_bounds_;
+  if (ShouldShowSystemBubbleForDragging(gesture)) {
+    system_bubble_->bubble()->UpdateBounds(target_bounds, false);
+  } else {
+    target_bounds.set_y(shelf_->GetIdealBounds().y());
+    system_bubble_->bubble()->UpdateBounds(target_bounds, true);
+  }
+}
+
+bool SystemTray::IsDraggingDownOnShelf(const ui::GestureEvent& gesture) {
+  gfx::Point gesture_location = gesture.location();
+  View::ConvertPointToScreen(this, &gesture_location);
+
+  if (gesture_location.y() >= shelf_->GetIdealBounds().y() &&
+      gesture.details().scroll_y_hint() > 0) {
+    return true;
+  }
+  return false;
+}
+
+void SystemTray::UpdateBoundsOnDragging(const ui::GestureEvent& gesture) {
+  gfx::Point gesture_location = gesture.location();
+  View::ConvertPointToScreen(this, &gesture_location);
+
+  // System tray bubble should not be dragged higher than its original height.
+  if (gesture_location.y() < system_tray_bubble_bounds_.y())
+    gesture_location.set_y(system_tray_bubble_bounds_.y());
+
+  gfx::Rect bounds_on_gesture(
+      system_tray_bubble_bounds_.x(), gesture_location.y(),
+      system_tray_bubble_bounds_.width(), system_tray_bubble_bounds_.height());
+  system_bubble_->bubble_view()->GetWidget()->SetBounds(bounds_on_gesture);
+}
+
+bool SystemTray::ShouldShowSystemBubbleBasedOnDragAmount() {
+  if (gesture_drag_amount_ > 0)
+    return false;
+
+  int system_bubble_height = system_tray_bubble_bounds_.height();
+  float one_third = system_bubble_height / 3.0;
+
+  return fabs(gesture_drag_amount_) >= one_third;
+}
+
+bool SystemTray::ShouldShowSystemBubbleForDragging(
+    const ui::GestureEvent& gesture) {
+  if (gesture.type() == ui::ET_GESTURE_SCROLL_END)
+    return ShouldShowSystemBubbleBasedOnDragAmount();
+
+  const float kSwipingVelocity = 100.0f;
+  if (fabs(gesture.details().velocity_y()) > kSwipingVelocity)

[tdanderson, 2017/06/19 16:19:38]

Consider setting kSwipingVelocity to -100 and inlining
ShouldShowSystemBubbleBasedOnDragAmount(). Then you could simplify this function
to just:

const float kSwipingVelocity = -100.0f;
if (gesture.type() == FLING_START &&
    gesture.details.velocity_y() < kSwipingVelocity()) {
  return true;
}

return -gesture_drag_amount_ >= system_tray_bubble_bounds_.height() / 3.0;

[minch1, 2017/06/20 16:45:34]

Thanks. Done the change for ShouldShowSystemBubbleBasedOnDragAmount. But keep
line 764-766. Since should return false (close the bubble) if velocity > 100.0f.

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

Acknowledged.

+    return gesture.details().velocity_y() < 0;
+  return ShouldShowSystemBubbleBasedOnDragAmount();
+}
+
 void SystemTray::CloseSystemBubbleAndDeactivateSystemTray() {
   activation_observer_.reset();
   key_event_watcher_.reset();