Commit dc1eb4f9 authored by Collin Baker's avatar Collin Baker Committed by Commit Bot

Fix tab strip drag-to-open bugs

Fixes the following:
- Fractional part of drag updates was thrown away, causing the current
  height to lag behind the touch position. The error compounded the
  longer the drag was in progress.
- Tab strip got stuck when dragged slowly to the fully open or closed
  position. Animations don't get run when they're at their target value,
  but I assumed otherwise.
- Incorrect assumptions about ordering of ET_GESTURE_SCROLL_END,
  ET_GESTURE_SWIPE, and ET_GESTURE_END resulted in occasional DCHECKs.
  Changed to be more permissive.

Bug: 1043374
Change-Id: Iedab85a33bf6c89b51f1a88e73864ba32e885781
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2076783Reviewed-by: default avatarTaylor Bergquist <tbergquist@chromium.org>
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745638}
parent bb7daded
...@@ -170,24 +170,28 @@ class WebUITabStripContainerView::DragToOpenHandler : public ui::EventHandler { ...@@ -170,24 +170,28 @@ class WebUITabStripContainerView::DragToOpenHandler : public ui::EventHandler {
switch (event->type()) { switch (event->type()) {
case ui::ET_GESTURE_SCROLL_BEGIN: case ui::ET_GESTURE_SCROLL_BEGIN:
drag_in_progress_ = true; drag_in_progress_ = true;
container_->UpdateHeightForDragToOpen(event->details().scroll_y_hint());
event->SetHandled(); event->SetHandled();
break; break;
case ui::ET_GESTURE_SCROLL_UPDATE: case ui::ET_GESTURE_SCROLL_UPDATE:
DCHECK(drag_in_progress_); if (drag_in_progress_) {
container_->UpdateHeightForDragToOpen(event->details().scroll_y()); container_->UpdateHeightForDragToOpen(event->details().scroll_y());
event->SetHandled(); event->SetHandled();
}
break; break;
case ui::ET_GESTURE_SCROLL_END: case ui::ET_GESTURE_SCROLL_END:
DCHECK(drag_in_progress_); if (drag_in_progress_) {
container_->EndDragToOpen(false); container_->EndDragToOpen(false);
event->SetHandled(); event->SetHandled();
drag_in_progress_ = false; drag_in_progress_ = false;
}
break; break;
case ui::ET_GESTURE_SWIPE: case ui::ET_GESTURE_SWIPE:
// If a touch is released at high velocity, the scroll gesture // If a touch is released at high velocity, the scroll gesture
// is "converted" to a swipe gesture. Note that a // is "converted" to a swipe gesture. ET_GESTURE_END is still
// ET_GESTURE_SCROLL_END event will not be sent. ET_GESTURE_END // sent after. From logging, it seems like ET_GESTURE_SCROLL_END
// is still sent after, however. // is sometimes also sent after this. It will be ignored here
// since |drag_in_progress_| is set to false.
DCHECK(drag_in_progress_); DCHECK(drag_in_progress_);
container_->EndDragToOpen(event->details().swipe_down()); container_->EndDragToOpen(event->details().swipe_down());
event->SetHandled(); event->SetHandled();
...@@ -391,7 +395,7 @@ void WebUITabStripContainerView::CloseContainer() { ...@@ -391,7 +395,7 @@ void WebUITabStripContainerView::CloseContainer() {
iph_tracker_->NotifyEvent(feature_engagement::events::kWebUITabStripClosed); iph_tracker_->NotifyEvent(feature_engagement::events::kWebUITabStripClosed);
} }
void WebUITabStripContainerView::UpdateHeightForDragToOpen(int height_delta) { void WebUITabStripContainerView::UpdateHeightForDragToOpen(float height_delta) {
if (!current_drag_height_) { if (!current_drag_height_) {
// If we are visible and aren't already dragging, ignore; either we are // If we are visible and aren't already dragging, ignore; either we are
// animating open, or the touch would've triggered autoclose. // animating open, or the touch would've triggered autoclose.
...@@ -403,8 +407,9 @@ void WebUITabStripContainerView::UpdateHeightForDragToOpen(int height_delta) { ...@@ -403,8 +407,9 @@ void WebUITabStripContainerView::UpdateHeightForDragToOpen(int height_delta) {
animation_.Reset(); animation_.Reset();
} }
current_drag_height_ = base::ClampToRange( current_drag_height_ =
*current_drag_height_ + height_delta, 0, desired_height_); base::ClampToRange(*current_drag_height_ + height_delta, 0.0f,
static_cast<float>(desired_height_));
PreferredSizeChanged(); PreferredSizeChanged();
} }
...@@ -433,8 +438,12 @@ void WebUITabStripContainerView::SetContainerTargetVisibility( ...@@ -433,8 +438,12 @@ void WebUITabStripContainerView::SetContainerTargetVisibility(
bool target_visible) { bool target_visible) {
if (target_visible) { if (target_visible) {
SetVisible(true); SetVisible(true);
PreferredSizeChanged();
if (animation_.GetCurrentValue() < 1.0) {
animation_.SetSlideDuration(base::TimeDelta::FromMilliseconds(250)); animation_.SetSlideDuration(base::TimeDelta::FromMilliseconds(250));
animation_.Show(); animation_.Show();
}
web_view_->SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY); web_view_->SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);
time_at_open_ = base::TimeTicks::Now(); time_at_open_ = base::TimeTicks::Now();
...@@ -454,8 +463,14 @@ void WebUITabStripContainerView::SetContainerTargetVisibility( ...@@ -454,8 +463,14 @@ void WebUITabStripContainerView::SetContainerTargetVisibility(
time_at_open_ = base::nullopt; time_at_open_ = base::nullopt;
} }
if (animation_.GetCurrentValue() > 0.0) {
animation_.SetSlideDuration(base::TimeDelta::FromMilliseconds(200)); animation_.SetSlideDuration(base::TimeDelta::FromMilliseconds(200));
animation_.Hide(); animation_.Hide();
} else {
PreferredSizeChanged();
SetVisible(false);
}
web_view_->SetFocusBehavior(FocusBehavior::NEVER); web_view_->SetFocusBehavior(FocusBehavior::NEVER);
// Tapping in the WebUI tab strip gives keyboard focus to the // Tapping in the WebUI tab strip gives keyboard focus to the
...@@ -564,7 +579,7 @@ int WebUITabStripContainerView::GetHeightForWidth(int w) const { ...@@ -564,7 +579,7 @@ int WebUITabStripContainerView::GetHeightForWidth(int w) const {
desired_height_); desired_height_);
} }
if (current_drag_height_) if (current_drag_height_)
return *current_drag_height_; return std::round(*current_drag_height_);
return GetVisible() ? desired_height_ : 0; return GetVisible() ? desired_height_ : 0;
} }
......
...@@ -89,7 +89,7 @@ class WebUITabStripContainerView : public TabStripUIEmbedder, ...@@ -89,7 +89,7 @@ class WebUITabStripContainerView : public TabStripUIEmbedder,
class DragToOpenHandler; class DragToOpenHandler;
// Called as we are dragged open. // Called as we are dragged open.
void UpdateHeightForDragToOpen(int height_delta); void UpdateHeightForDragToOpen(float height_delta);
// Called when drag-to-open finishes. If |fling_to_open| is true, the // Called when drag-to-open finishes. If |fling_to_open| is true, the
// user released their touch with a high enough velocity that we // user released their touch with a high enough velocity that we
...@@ -145,7 +145,7 @@ class WebUITabStripContainerView : public TabStripUIEmbedder, ...@@ -145,7 +145,7 @@ class WebUITabStripContainerView : public TabStripUIEmbedder,
views::View* tab_counter_ = nullptr; views::View* tab_counter_ = nullptr;
int desired_height_ = 0; int desired_height_ = 0;
base::Optional<int> current_drag_height_; base::Optional<float> current_drag_height_;
// When opened, if currently open. Used to calculate metric for how // When opened, if currently open. Used to calculate metric for how
// long the tab strip is kept open. // long the tab strip is kept open.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment