Screencasts of some tricky test cases

Deleted: tabdrag_many_attachments.mov 2.1 MB

tabdrag_many_attachments.mov 2.1 MB Download

Deleted: tabdrag_quick_detach.mov 1.1 MB

tabdrag_quick_detach.mov 1.1 MB Download

I can reproduce the tabdrag_quick_detach when dragging to the top, over the menu bar. Cocoa version continues the drag of the semi-transparent window even when the cursor is over the menu bar. This is extremely easy to test, if your case also involved dragging over the menu bar.

When reproducing tabdrag_many_attachments I catched `bridged_native_widget.mm(481)] Check failed: !view || !compositor_widget_` with this stacktrace: https://gist.github.com/anonymous/ca12b5b62723b7d1d429

But more often the dragged tab shifts to the leftmost position and I'm dragging a window by the title.

What to do with the DCHECK? I think the quick juggling of the window should reproduce the issue.

Deleted: tabdrag_quick_detach_cocoa.mp4 3.9 MB

tabdrag_quick_detach_cocoa.mp4 3.9 MB Download

Added CocoaShakeDetach test, and it reproduces the detachment pretty reliably. The cause of the problem seems to be that we use global mouse position somewhere, and not the coordinates we get from the LMouseDragged event.

If I'm correct, it should be possible to reliably reproduce the issue without any flakiness by just overriding FakeNSEventTestingDonor / FakeNSWindowTestingDonor to return incorrect mouse location.

Deleted: tabdrag_many_attachments2.mp4 1.1 MB

tabdrag_many_attachments2.mp4 1.1 MB Download

Was debugging the tabdrag_many_attachments3 bug today, and it happens because CocoaWindowMoveLoop ends prematurely because of LoopExitReason::MOUSE_UP.

Definitely didn't expect this, probably it's a result of simulated MouseUp event. Will continue debugging it further tomorrow.

Deleted: tabdrag_many_attachments3.mp4 1.3 MB

tabdrag_many_attachments3.mp4 1.3 MB Download

That MouseUp event is generated by the previous CocoaWindowMoveLoop, which is terminated by BridgedNativeWidget::EndMoveLoop(). But it's processed by the wrong window.

As it stands, I can't really write a test for that, as it's racy behaviour at the WindowServer level :-/

Ignoring the generated MouseUp event resulted in even worse bugs — new window stucking to the top of the screen, with its height expanded.

You've mentioned a different solution:
// TODO(tapted): A better fix would be to keep the temporary window
// around and never call EndMoveLoop() on Mac, making this block of code
// obsolete.

Could you please elaborate on that a little?

Sorry I haven't had time to play around with the CL more - my daily schedule has exploded these last couple of weeks :/

Hum - we could perhaps use CGEventSetSomethingValueField to hardcode a fixed NSWindow that the event should happen on. Then, if the event tap can't find that window, or if the event would somehow "miss" the window swallow or edit the event.

But... if the event is getting processed by the window server after the event tap then that's another problem :(

Regarding
// TODO(tapted): A better fix would be to keep the temporary window
// around and never call EndMoveLoop() on Mac, making this block of code
// obsolete.

This was the rogue variable in tab_drag_controller.cc -- the one I commented on here: https://codereview.chromium.org/1747803003/diff/1/chrome/browser/ui/views/tabs/tab_drag_controller.cc#newcode91

but I never had time to explore it further.

The idea would be that tab dragging in toolkit views (optionally?) behaves more like how tab dragging currently behaves in Cocoa. In Cocoa, when you drag a tab off a browser, that temporary window hangs around for the entire drag operation; until you release the mouse. In views, as soon as that tab would be getting attached to a browser, the temporary window gets destroyed -- it creates a lot of hard problems like this, and it also doesn't feel as nice in terms of UX, in my opinion. It works OK on ChromeOS since windows are just textures -- really cheap. But on Desktop platforms creating and destroying native windows (a DesktopWindowTreeHost in Aura parlance) is really expensive. E.g. I think there would be support for using the same approach on Windows/Linux desktop platforms (perhaps even CrOS too).

### Regarding the MacViews' CocoaWindowMoveLoop
Tried creating CGEvents with windowNumber specified (created NSEvent using same approach as in SendMouseEventsNotifyWhenDone, copied its CGEvent to a new CGEventRef, set kCGEventSourceUserData on a copy), but it didn't really change anything. 

windowNumber seems to be only used to convert a local window-specific mouse cursor position to a screen-global one, I wasn't able to detect any window-specific event filtering.

We can probably re-post the event in that case, but if there's a new window on top 
of the window we want to send the event to, the click won't go through it. An option would be to make the top window -setIgnoresMouseEvents: to make the top window transparent for mouse clicks/releases, re-post the event, verify that the correct window did receive it, disable -setIgnoresMouseEvents: and proceed with sending the NSLeftMouseDown click.

BTW, when are CocoaWindowMoveLoop::Run()'s finishing Up/Down mouse events supposed to be sent? It seems that a new window is always supposed to exist on top of the old one, and that the Up event mostly arrives to the bottom window seems to be a happenstance.

Otherwise there would be an option to wait for the Up event to be processed by the correct window, then show the new window and send the appropriate events.

### Regarding Cocoa Chrome approach
Cocoa's -[TabStripDragController maybeStartDrag:forTab:] spins its own event loop and does not rely on WindowServer to do the window moving, instead handling all the move operations manually. This has a nasty side-effect, that one can't "Drag the window to the edge of the screen. After a moment, the window moves to the next space." El Capitan feature. Also cursor doesn't move as a single whole when moving the window (or it's my imagination playing tricks on me).

The TabDragController's logic is overly complex, and it seems that there are not enough tests for all the different behaviors, so this kind of major change sounds extremely risky to me.

> BTW, when are CocoaWindowMoveLoop::Run()'s finishing Up/Down mouse events supposed to be sent? 

I don't think I've thought about where they're supposed to go -- they merely need to go to the "right" place so that tab dragging works :). I'll need to play around with the code more -- I might be able to figure out what you're getting at here.

> ### Regarding Cocoa Chrome approach

The goal in MacViews would not be to simulate Cocoa's TabStripDragController stuff - I agree it's not nice that you can't move windows between spaces.

Indeed, it would be a tricky change to get the window-is-kept-during-drag working. However, it might allow the toolkit-views TabDragController to be simplified/made less complex. Sadly, if it is currently overly complex, we should try to update it so that it's only as complex as it needs to be (it's going to be around for a while, and it probably has to be done eventually..)

Turns out the simulated Up mouse event is not necessary at all in the ENDED_EXTERNALLY case, at least on El Capitan. Both Up/Down events were sent in your code when the tab was reattached, and it really works without the Up event.

The logic looks like this now:

  if (exit_reason != MOUSE_UP) {
    if (exit_reason == ENDED_EXTERNALLY) {
      BridgedNativeWidget::IgnoreNextMouseDownForDraggableRegions();
      SendCustomLeftMouseEvent(window, kCGEventLeftMouseDown);
    } else {
      SendCustomLeftMouseEvent(window, kCGEventLeftMouseUp);
    }
  }

But I've been able to reproduce another weird bug using the same wild dragging around the parent window: tab attaches to parent, but on the next TabDragController::Drag() event it detaches without setting a new RunMoveLoop, and it's still being dragged, because the last RunMoveLoop quit with ENDED_EXTERNALLY reason, and it didn't send a MouseUp event.

I think it should set up a new RunMoveLoop, still digging through TabDragController's code to figure out how it's supposed to work.

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

commit 2d62862c2faaa841bbeaf9a455df499e62c26476
Author: patricialor <patricialor@chromium.org>
Date: Mon Mar 21 00:38:29 2016

MacViews: Enable acceptsFirstMouse for BridgedContentView.

This allows an initial mouse click on an inactive window to be accepted
immediately, so two clicks are not required (i.e. one to activate the view and
the second to send the mouse event to the view itself).

See linked bugs for certificate viewer and tab dragging, both which require
this to be enabled.

BUG= 594079 , 587239 

Review URL: https://codereview.chromium.org/1815563002

Cr-Commit-Position: refs/heads/master@{#382240}

[modify] https://crrev.com/2d62862c2faaa841bbeaf9a455df499e62c26476/ui/views/cocoa/bridged_content_view.mm

Fixed the tabdrag_many_attachments.mov problem locally — was unable to reproduce even after ferocious jiggling.

There's still a problem when the cursor moves to the screen edge while dragging a detached window — will try to tackle it next. Maybe I'll be able to finally fix the remaining issues this week (fingers crossed).

Fixed the issue when the dragging starts directly on top of the menu bar as well — now I directly detect this case and adjust the mouse cursor and window position so it wouldn't overlap.

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

commit 6984eac05884a72bc81697589817df83bcde0551
Author: mblsha <mblsha@yandex-team.ru>
Date: Fri Jun 17 18:38:18 2016

MacViews: Implement Tab Dragging

BridgedNativeWidget::RunMoveLoop() drags the window by manually processing mouse move events.

Most of the tests in tab_drag_controller_interactive_uitest.cc pass, and I've added several Mac-specific ones to test for the edge cases.

BUG= 594079 
COLLABORATOR=tapted@chromium.org
Also reviewed in http://crrev.com/1259913003/

Review-Url: https://codereview.chromium.org/1747803003
Cr-Commit-Position: refs/heads/master@{#400463}

[modify] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
[modify] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/chrome/browser/ui/views/tabs/window_finder_mac.mm
[modify] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/chrome/chrome_tests.gypi
[modify] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/chrome/test/BUILD.gn
[modify] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/ui/base/test/ui_controls_mac.mm
[modify] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/ui/views/cocoa/bridged_native_widget.h
[modify] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/ui/views/cocoa/bridged_native_widget.mm
[add] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/ui/views/cocoa/cocoa_window_move_loop.h
[add] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/ui/views/cocoa/cocoa_window_move_loop.mm
[modify] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/ui/views/cocoa/views_nswindow_delegate.mm
[modify] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/ui/views/views.gyp
[modify] https://crrev.com/6984eac05884a72bc81697589817df83bcde0551/ui/views/widget/native_widget_mac.mm

BTW in macOS 10.11+ there's a new useful API for system drag'n'drop: https://developer.apple.com/reference/appkit/nswindow/1419386-performwindowdragwithevent

Esc-cancellation won't work, but the overall stability should be even better than the current setFrame: approach.

More polish remains (we should avoid destroying the window until mouse release, rather than every time an attach occurs). We should use performwindowdragwithevent too.

Using performwindowdragwithevent is hard because AFAIK we don't get a notification that the user stopped the drag operation. There could be a way to get it, but definitely requires more digging.

Managed to get it to work in a small test application on macOS 10.12+:

  [[[self view] window] performWindowDragWithEvent:event];

  // Possibly requires NSEventSubtypeWindowMoved (10.12+ SDK) in order to work.
  while ([NSEvent pressedMouseButtons] & 1) {
    [[[self view] window] nextEventMatchingMask:NSEventMaskAny];
  }

Note that, while this is implemented, it's still super janky and sometimes hard to trigger if you move quickly. Assigning to sdy@ for de-janking by M68.

 Issue 826498  has been merged into this issue.

** Bulk Edit **

FYI: Starting 04/13 M68 will be in canary, M68 Dev promotion will be on 04/26.

Since it is technically implemented, I'll track de-jankification in  issue 594079 .