Sorry, I don't quite follow, mostly because the video doesn't show your input events so I can't really see what you're doing.

* When you say "tap" do you mean mouse or touch?  Or does it matter?
* Where are you talking about tapping?  In the video it looks like you're clicking on the page content, which I would probably expect to deselect your other tabs (did it not used to do this?).  Is that actually what you're doing?

By "tap" I mean "click on laptop trackpad", it seems to result in different behavior than if I "click" with laptop's click button (Lenovo X1 Yoga) -- can't repro with a click.

In the video I'm shift-tapping and holding Shift key for a little while before releasing on the first two attempts (and yes I'm clicking content to undo selection between attempts). On the third attempt I'm releasing the shift-key swiftly (at the same time as releasing tap), notice how the tabs are selected and then quickly unselected (without further clicking/tapping -- maybe "tap off" generates a post-click event which races with shift-key's release..?)

(sorry I had all those details in initial report but Canary crashed on me before submitting and looks like I rewrote it with way less details...)

Interesting.  I wonder what Spy++ would say the sequence of input events is in these two cases.  Wonder if there's some kind of trackpad software issue here?  One way to check would be to see if you can reproduce on a machine with a different manufacturer trackpad/driver.

It's also interesting that you haven't seen this before, since I can't think offhand of what might have changed to cause it.  Getting a bisect of this would be useful if you think you can reproduce reliably enough to do one confidently.

I don't have Visual Studio on this machine, do you know of any other way of observing the events?

https://docs.microsoft.com/en-us/windows/desktop/tablet/timeline-of-mouse-messages-and-system-events says that there can be multiple events associated with a tap, perhaps it results in something like tap + shift-release + click (and the click unselects)?

If you want to package me a mini_installer local Chromium build with extra logs (I'm not sure where to put them and also can't build chrome on this machine...). I can look at the output in Syzyprof.

Or I can attach a debugger with a breakpoint on the method that results in the unselection if you know which symbol it is?

I'll set the bit for QA to attempt a bisect in the meantime (or I can try as a last resort if it's really specific to my machine by that would surprise me...)
@QA : See video for repro (3rd time in video is the regression example), need to quickly release the Shift key after tapping (Lenovo trackpad).

@5: Possibly https://sourceforge.net/projects/winspyex/ ?

Able to reproduce the issue on reported chrome version 70.0.3508.0 and on latest chrome 70.0.3510.0 using Windows-10 and Linux 17.10, hence providing Bisect Info
Note: Issue is not seen on Mac
Bisect Info:
================
Good build: 69.0.3469.3
Bad build: 69.0.3472.0

After performing per-revision bisect get the error message: "Error running the gsutil command: ServiceException: 401 Anonymous caller does not have storage.objects.get access to chrome-test-builds/official-by-commit/Win x64 Builder" and for chromium bisect got all bad builds, hence providing manual change log from omahaproxy

Change-log: https://chromium.googlesource.com/chromium/src/+log/69.0.3469.0..69.0.3472.0?pretty=fuller&n=10000
suspecting: https://chromium.googlesource.com/chromium/src/+/dcdd42dd3445ead273ec357ef2fc286e657a0273 from above change log
Reviewed-on: https://chromium-review.googlesource.com/1112433

@Orin Jaworski: Please confirm the issue and help in re-assigning if it is not related to your change.

Thanks!

I haven't been able to reproduce this problem on Windows canary 70.0.3510.2.  The "suspecting" change in comment #8 is cosmetic and almost certainly not the cause.  Assigning to the tab-strip owner, in case there's a bug in selection logic, but I am more suspicious of Lenovo trackpads and phantom touch screen events at this point.  I have seen it in the past, where events come seemingly from nowhere and it turned out to be a bad screen or an automatic update with faulty driver logic.  Does Chrome have a flag to enable input logging?  Or can you dump some WinSpy data here, along with a video of it being captured?  It's great that you can repro, and sometimes it's a whole class of machines/environments that can repro while others cannot.

Right after I posted that comment, I gave it one more go and was able to repro. :)  You really have to release shift *immediately* after tapping the touchpad.  It probably is some extra event getting sent by the touch driver and Chrome is handling it differently because it no longer detects shift pressed.  My advice at this point is to find out what this extra event is and see if there's a safe work-around.  Are all the repro machines using Lenovo?  There might be a diagnostic tool that can observe the events directly, ideally with precise time-stamps.

I'm not going to have time to try and investigate this in the near future; +CC some folks who know something about event handling.

I'm still suspicious about whether this is a recent regression; it sounds like the sort of thing that ought to have been wrong for a while.

Spy++ or logging all event details going through EventProcessor::OnEventFromSource or similar may be enlightening.
Perhaps the mouse event is not getting EF_SHIFT_DOWN or similar? The tab code in question appears to be:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab.cc?rcl=f90c85244ca2b038d396195d73721d0f340bfc93&l=776

M69 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.

@13: This isn't assigned to anyone, so no one is on the hook to fix.

I am not familiar with the code involved here, but I suspect this is running logic similar to what happens when you do this:

* Open several tabs
* Click one tab
* Press Shift key down (hold)
* Press mouse button down on another tab to select tab range (hold)
* Release Shift key
* Release mouse button

The expectation when selecting tab range seems to be that Shift will be held for the duration of the second click.  And I guess the Lenovo touchpad (and possibly others) are trying to be helpful by adding an artificial delay between the down and up events.  I've seen this kind of thing coming from touch drivers before.  It can be hard to work around, unless there is a clear way to distinguish the events.  If they are indistinguishable from the above natural event sequence (quite possible), then I think the only solution would be to change tab range selection logic more fundamentally, perhaps marking the intention to select range on press instead of considering shift-state on depress.

Good commentary!  If I manually do the following with my mouse:

* Click on a tab
* Hold shift
* Mouse down on another tab -> we draw tabs as multi-selected
* Release shift
* Mouse up -> selection becomes just the new tab

I think it's this last step that's a problem.  We shouldn't be clearing the selection to one tab when we mouse up.  Windows explorer doesn't.

Ah indeed, being slow on the mouse up reproduces the bug with a mouse (I'd only ever repro'ed with a trackpad -- "smarts" in the trackpad as mentioned could explain).

For some reason I feel this has been happening to me more recently (i.e. pretty much 100% on my laptop unless I explicitly make an effort to hold the Shift or Ctrl key extra long)... (could be an OS or driver update affecting timings on the trackpad..)

M69 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.

I don't think this should be RBS.  Still not convinced something on the Chrome side changed here.

I've installed Spy++ and confirm that this is what's happening. I ran the repro as well as the long-shift-hold to force the selection (full logs attached).

The main difference is that this sequence of events results in undoing selection (actual):

WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:0 fUp:0
WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
WM_LBUTTONDOWN fwKeys:MK_LBUTTON | MK_SHIFT xPos:708 yPos:40
WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
WM_KEYUP nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:1
WM_PAINT hdc:00000000
WM_LBUTTONUP fwKeys:0000 xPos:708 yPos:40

whereas this results in keeping it (expected)

  WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:0 fUp:0
  WM_KEYUP nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:1
  WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:0 fUp:0
  WM_LBUTTONDOWN fwKeys:MK_LBUTTON | MK_SHIFT xPos:508 yPos:37
  WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:3 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
  WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
  WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
  WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
  WM_LBUTTONUP fwKeys:MK_SHIFT xPos:508 yPos:37
  WM_KEYDOWN nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:0
  WM_KEYUP nVirtKey:VK_SHIFT cRepeat:1 ScanCode:2A fExtended:0 fAltDown:0 fRepeat:1 fUp:1


The main difference being "WM_LBUTTONUP" coming *after* "WM_KEYUP nVirtKey:VK_SHIFT" in the repro case whereas the expected behavior is that "WM_LBUTTONDOWN fwKeys:MK_LBUTTON | MK_SHIFT" is sufficient and "WM_LBUTTONUP" has no impact.

Deleted: MultiTabSelectBug(Selection Works).sxl 11.6 KB

MultiTabSelectBug(Selection Works).sxl 11.6 KB Download

Deleted: MultiTabSelectBug(Repro - Selection undone).sxl 13.0 KB

MultiTabSelectBug(Repro - Selection undone).sxl 13.0 KB Download

What's happening here is Tab::OnMouseReleased():

  } else if (event.IsOnlyLeftMouseButton() && !event.IsShiftDown() &&
             !event.IsControlDown()) {
    // If the tab was already selected mouse pressed doesn't change the
    // selection. Reset it now to handle the case where multiple tabs were
    // selected.
    controller_->SelectTab(this);

The intent is that if you have a multiselection, and you just make a click on one of the tabs, it should clear the multiselection. This is inadvertently triggering in this case, where the shift key is let up early, the Tab::OnMouseReleased() call sees just a left mouse up with no modifiers, and thinks that it was a simple click and clears the selection.

The intent to clear a selection with just a click is probably a good one, but it's likely not the best implementation.

Yes, exactly.  The trouble comes from the assumption that shift will be held for the duration of a click when really the shift-state should only be checked on mouse down.  The click is of short duration, but the key can and does get released in between mouse press & release.  To be more consistent with the way users think, the behavior should only be decided on mouse press, not release.  This may involve some significant reworking of existing logic, but the result will be a more intuitive and predictable UI, especially when touch drivers produce artificially delayed release events.

The problem is that we can't distinguish at mouse-down time between "clicking on an existing tab in the selection to drag the selection" versus "clicking on an existing tab in the selection to deselect everything but this".  That particular determination has to be made at mouseup time; see also how e.g. Windows File Explorer multiselection handles this case.

What we need is a better heuristic on the code that Avi found, e.g. something to do with what the effect of the corresponding mousedown was.

I don't mean that mouse-down can only invoke one specific behavior; the user may drag or do other things.  But when it is decidedly a "click" interaction, the shift state on press is what matters, not the shift state on release.  When clicking, especially with keyboard modifiers, users think in terms of the mouse-down, not up; they don't press mouse, then hold shift, then release mouse.  So yes, I think we agree that the effect of the mouse-down matters (click vs. drag) but we can't naively check shift-state later on release.  It has to be remembered as it was on press.

So to fix the problem at hand, it may be sufficient to use the press event's shift-state in place of current event's shift state.  I'm not familiar with all this logic, though, so I'm not sure how involved that change is.  Ideally there's already some state being tracked for the current interaction sequence, and it's a simple matter to make use of it.