Seems like a UI issue

I could repro on 63. afakhry@, can you take a look?

I was able to repro on M-64 as well. I will start looking into it.

oshima@, this is a result of setting the bounds on the browser widget before we run the move loop here [1].

Since the majority of the widget bounds intersects with the external display, NativeWidgetAura::SetBounds() tries to find the matching destination screen here [2], which will be in this case the external display (even though the window didn't move there yet), but this will change its root window.

This will make the event's target window's root is the external display root window, which will result in getting the wrong host and the wrong native point here [3]. Hence warping fails.

[1]: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_drag_controller.cc?type=cs&q=TabDragController::Drag&l=389

[2]: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?type=cs&q=NativeWidgetAura::SetBounds&l=472

[3]: https://cs.chromium.org/chromium/src/ash/display/extended_mouse_warp_controller.cc?type=cs&q=ExtendedMouseWarpController::WarpMouseCursor&l=149

sadrul@ and sky@: Please advise.

This bug can be naively fixed by this CL: https://chromium-review.googlesource.com/c/chromium/src/+/795174/2

However it's probably better to fix at the platform level rather in tab_drag_controller.cc, but I have no idea how.

The problem is that calling Widget::SetBounds() will change the widget's root window to the other display root window (even though the window still resides in the original display) ... See NativeWidgetAura::SetBounds() [1]. This happens when the majority of the widget's bounds intersects the other display.

After that events will have targets with roots == to the other display root, but the locations of the native events are still sent relative to the original display. 


[1]: https://cs.chromium.org/chromium/src/ui/views/widget/native_widget_aura.cc?type=cs&q=NativeWidgetAura::SetBounds&l=471

I thought the problem is that the event's native event can be originated from different platform window than the target?

That video looks to me more like mouse warping is failing. Are you sure that isn't the issue?

Yes, mouse warping is failing because of what I described earlier.

Here's the relevant mouse warping code: https://cs.chromium.org/chromium/src/ash/display/extended_mouse_warp_controller.cc?q=ExtendedMouse&sq=package:chromium&l=142-150


  gfx::Point point_in_native =
      ui::EventSystemLocationFromNative(event->native_event());

  // TODO(dnicoara):  crbug.com/415680  Move cursor warping into Ozone once Ozone
  // has access to the logical display layout.
  // Native events in Ozone are in the native window coordinate system. We need
  // to translate them to get the global position.
  point_in_native.Offset(target->GetHost()->GetBoundsInPixels().x(),
                         target->GetHost()->GetBoundsInPixels().y());


Let me give an example: Assume two displays with the following native bounds: (Note that this is the *native* layout. The actual logical layout is Display A : Display B side by side).

(0,0)
  +---------------+
  |               |
  |  2400 x 1600  |
  |               | Display A
  |               |
  +---------------+
  
(0,1660)
  +-------------+
  |             |
  | 1920 x 1200 |  Display B
  |             |
  +-------------+

When cursor is dragging the tab in display A where the majority of the widget's bounds intersects with display B, Widget::SetBounds() changes the widget's root to be Display B's root window. When cursor reaches Display A's right side border, point_in_native is still relevant to display A (For example {2399,300}).

target is event->target(), which now is a child of display B's root window. target->GetHost() is display B's host (with pixel bounds = {0,1660 1920x1200}). When we do the above offset, point_in_native becomes {2399,1960}. This native point is *not* contained in any display warp boundary edges, and hence warping fails.


So in summary, native event's location is still relative to original display, while the event's target is in another display host.

Hope this explains it.

Thanks for the analysis and excellent details.

I tend to think the bug is in NativeWidgetAura::SetBounds(). Specifically it seems as though SetBounds() should favor the current display when possible:

Display current_display = Screen::GetDisplayNearestWindow(window_);
if (!current_display.bounds().Intersects(bounds)) {
  // existing code
  return;
}
window_->SetBounds(bounds);

Re #12: This works too. Thanks for suggesting! 

Is there something else we can do to the native event itself to avoid having this problem showing up somewhere else? I mean, is there a way to make sure that the location of the native event is correct (relative to the correct display) if the event's target changes to a different root?

If you go with that path, please make sure that it will not cause the situation where only 1px is visible (which makes it invisible).

My preference is to fix the event system because the native event location is simply wrong in this scenario.

> Is there something else we can do to the native event itself to avoid having this problem showing up somewhere else?

The problem is the event isn't directly tied to the window, so when updating the bounds of the window you have no way to know there is an event that may need its target updated.

I think the core problem with the event location is that the event is relative to the display it comes in on, *not* screen (608547). If the location was in screen coordinates then ExtendedMouseWarpController wouldn't need to do conversion, and it wouldn't matter if the window was moved to a different display.

Without fixing 608547 the only thing I can think of is to add a display_id parameter to events. But this feels less than satisfying.

crbug.com/608547 is different issue. root_location is in the root window's coordinate, not host, and what we need is the native location.

If we can update the event location of the native event to the host's coordinates, it should fix this issue. 

I think it's related. What you are calling native location is relative to the display the event came from (hence the need to offset the location in the code afakhry mentions). If the native location were in screen coordinates no conversion would be necessary.

Native location isn't relative to the display, which may be rotated or scaled. It is relative to the platform window.

sky@ and sadrul@

Here's another CL that implements oshima's suggestion to change the native event's location if the hosts of the source and target windows are different.

https://chromium-review.googlesource.com/c/chromium/src/+/804954

This fixes the issue. Is this an acceptable solution?

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

commit 8a80410c4b95c87230d137f35d13776b6fbd4e75
Author: Ahmed Fakhry <afakhry@google.com>
Date: Wed Dec 06 18:00:55 2017

Re-enable dual display tab dragging tests

Fix a bug in UIControlsOzone that tracked the mouse button status for
each display host separately rather than one global state. This caused
all dual display tab drag tests to fail once the mouse warps to the other
display as it used to send a mouse move event rather than drag event to
continue the drag in the other host.

Also, tab dragging between two displays with touch is impossible, so no need
to test that as it will always fail.

BUG= 714578 
TEST=interactive_ui_tests --enable-pixel-output-in-tests --gtest_filter=*DetachToBrowserInSeparateDisplayTabDragControllerTest.*

Change-Id: I08132f41aa4923d2a2ae4faf6573068c1e2fcb08
Reviewed-on: https://chromium-review.googlesource.com/809714
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522134}
[modify] https://crrev.com/8a80410c4b95c87230d137f35d13776b6fbd4e75/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
[modify] https://crrev.com/8a80410c4b95c87230d137f35d13776b6fbd4e75/ui/aura/test/ui_controls_factory_ozone.cc

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

commit 9bd7bf4a531877f04848c9a1251984be460f7904
Author: Ahmed Fakhry <afakhry@google.com>
Date: Mon Dec 11 19:10:32 2017

Fix tab dragging to external display

Convert the native event location to the target's host if it's different
from the source's host.

BUG= 714578 

Change-Id: I09d64fe043c9d872331dd35babd41ca62280f9c7
Reviewed-on: https://chromium-review.googlesource.com/804954
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523153}
[modify] https://crrev.com/9bd7bf4a531877f04848c9a1251984be460f7904/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
[modify] https://crrev.com/9bd7bf4a531877f04848c9a1251984be460f7904/ui/aura/window.cc
[modify] https://crrev.com/9bd7bf4a531877f04848c9a1251984be460f7904/ui/aura/window.h
[modify] https://crrev.com/9bd7bf4a531877f04848c9a1251984be460f7904/ui/aura/window_targeter.cc