macviews triage: over to sdy@

Looks similar to  issue 853240 . Assigning to kylixrd@ for evaluation.

Is this only on Mac? I'm unable to reproduce this on other platforms.

> Is this only on Mac? I'm unable to reproduce this on other platforms.

Not sure. Unfortunately I have only a Mac to test it :(

I have this CL in flight that *might* help to address this: https://crrev.com/c/1106044

It is to specifically address  issue 853240 , but may also help this. Once this lands, this should be retested.

Great, thank you. Once your CL lands I will retest it and give a feedback. 

mehmet: If this still is an issue on Mac, feel free to bump this back to P1.

Triage: Upgrading to P1 - Flicker or movement now meets the P1 bar.

> mehmet: If this still is an issue on Mac, feel free to bump this back to P1.

Yes, it is still reproducible.

I was able to bisect it and this is the regression range: 

https://chromium.googlesource.com/chromium/src/+log/c2aa90817727c49eb306db48ef753c4a4eb9b264..6560e0eda707b5ca9088c376a7071eb8d607c873

Maybe caused by https://chromium-review.googlesource.com/c/chromium/src/+/1024296 ?

erikchen@ Can you please take a look at this issue? Thanks :)

Not sure what's going on with that bisect, but don't think any of the CLs are relevant. My CL was just a refactor. Pretty sure the relevant CL is: https://chromium-review.googlesource.com/c/chromium/src/+/1024666, which landed the same day.

Here's a more detailed description of what's happening:
  1) Let's say there is a window with 5 tabs, numbered 0 through 4. 
  2) I drag tab #4 slowly to the right. At some point, I will hit a threshold where tab #3 flickers/disappears. 
  3) At this point, I let go of tab #4. It gets dropped into tab #3's position!

Diving into the code:
  TabDragController::GetInsertionIndexForDraggedBounds fires many times during the slow drag. In the beginning, it's working properly and returns (4). At the same time that tab #3 disappears, TabDragController::GetInsertionIndexForDraggedBounds starts returning (3). The reason for this is:

"""
  const Tab* last_visible_tab = attached_tabstrip_->GetLastVisibleTab();
  int last_insertion_point = last_visible_tab ?
      (attached_tabstrip_->GetModelIndexOfTab(last_visible_tab) + 1) : 0;
  if (drag_data_[0].attached_tab) {
    // We're not in the process of attaching, so clamp the insertion point to
    // keep it within the visible region.
    last_insertion_point = std::max(
        0, last_insertion_point - static_cast<int>(drag_data_.size()));
  }
"""

The index of the last visible tab turns from (4) to (3) when the tab is dragged sufficiently far. Dragging it further to the right will cause the index of the last visible tab to go back to 4, and then 3, and then 4, etc. causing flickering.

The reason that GetLastVisibleTab() is changing is that TabStrip::ShouldTabBeVisible is incorrectly returning "false", from this conditional:
"""
if (right_edge > tabstrip_right)
  return false;
"""

This logic was added here: https://chromium-review.googlesource.com/c/chromium/src/+/1024666/4/chrome/browser/ui/views/tabs/tab_strip.cc#614

Over to pkasting. 

 Issue 856930  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/46c63393ecad428f9c52e7d35f968fccc73aa42c

commit 46c63393ecad428f9c52e7d35f968fccc73aa42c
Author: Peter Kasting <pkasting@chromium.org>
Date: Wed Jun 27 16:52:02 2018

Handle tab drag area bounds correctly for refresh.

Pre-refresh, during dragging, tabs could slide anywhere in the strip (from
tab left = 0 to tab right = tabstrip width).  In refresh, tabs should not drag
over the frame grab width or a LEADING or TRAILING new tab button.

This also prevents strange visual artifacts in TRAILING mode that currently
happen because a tab can be dragged past the point where it's supposed to be
allowed to remain visible.  With the updated code, tabs can't be dragged this
far, so the artifacts can't occur.

Along the way, I moved the Finch feature for the new tab button position into
the one function that uses it, because while trying to test locally by
commenting out parts of that function, I kept getting "unusued variable"
warnings.

Bug:  852936 
Test: On Mac with refresh enabled, dragging a tab to the right edge of the strip should not result in flickering/disappearing tabs.
Change-Id: Idef63702d5c3161a38e1662feec8373239334c0d
Reviewed-on: https://chromium-review.googlesource.com/1114299
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570810}
[modify] https://crrev.com/46c63393ecad428f9c52e7d35f968fccc73aa42c/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc
[modify] https://crrev.com/46c63393ecad428f9c52e7d35f968fccc73aa42c/chrome/browser/ui/views/tabs/tab_drag_controller.cc
[modify] https://crrev.com/46c63393ecad428f9c52e7d35f968fccc73aa42c/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
[modify] https://crrev.com/46c63393ecad428f9c52e7d35f968fccc73aa42c/chrome/browser/ui/views/tabs/tab_strip.cc
[modify] https://crrev.com/46c63393ecad428f9c52e7d35f968fccc73aa42c/chrome/browser/ui/views/tabs/tab_strip.h

Thank you pkasting@, works perfectly now for me in latest Trunk #570824 on macOS 10.12.5.

Able to reproduce the issue on Mac 10.13.5 on reported version 69.0.3457.2.

Verified the fix on Mac 10.13.5, as per comment#0 on latest chrome version #69.0.3493.0.
Attaching screen cast for reference.
Observed that the tab is visible.
Hence, the fix is working as expected.
Adding the verified labels.

Thanks...!!

Deleted: 852936.mp4 858 KB

	852936.mp4 858 KB View Download