1276183 - Windows can disappear from the screen and be no longer accessible after drag & drop

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Marco Castelluccio [:marco]]

Attachment: asd3.webm

If you are dragging & dropping some tabs from a window to another and you aren't careful enough, you could end up losing access to a window.

Comment 1

[Karl Tomlinson (:karlt)]

What happens when the drag misses the target is that the tab ends up being moved to a new window.  That new window may end up anywhere, but usually that would be on the screen.

Selecting the window that disappeared from the list of Firefox windows didn't seem to show any focused window, which makes me suspect that it is off screen or invisible.

I guess Ctrl+W would close the disappearing window when it disappears?

NSPR_LOG_MODULES=Widget:5 would log the position and size of the disappearing window.

Comment 2

[Marco Castelluccio [:marco]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #1)
> Selecting the window that disappeared from the list of Firefox windows
> didn't seem to show any focused window, which makes me suspect that it is
> off screen or invisible.
> 
> I guess Ctrl+W would close the disappearing window when it disappears?

Yes, I think it's off screen. I can still see the title of the window in the
Ubuntu top bar, so this means it is focused.

Ctrl+W closes it.

Comment 3

[Marco Castelluccio [:marco]]

Attachment: log1.txt

Log of a session similar to what I did in the video.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: log2.txt

Log when I focus the off screen window (after I've already created it and got it to go off screen).

Comment 5

[Karl Tomlinson (:karlt)]

It looks like Gecko is moving the window off the desktop.

nsWindow::SetSizeMode [7fc40fe45000] 0
nsWindow::NativeResize [7fc40fe45000] 1982 758
GetScreenBounds 898,84 | 1982x758
nsWindow::Move [7fc40fe45000] 2544.000000 84.000000

nsWindow::SetSizeMode [7fc40fe45000] 0
nsWindow::NativeResize [7fc40fe45000] 1982 758
GetScreenBounds 2313,122 | 1982x758
nsWindow::Move [7fc40fe45000] 2530.000000 84.000000
nsWindow::SetSizeMode [7fc40fe45000] 0

nsWindow::SetSizeMode [7fc40fe45000] 0
nsWindow::NativeResize [7fc40fe45000] 1982 758
GetScreenBounds 2530,84 | 1982x758
nsWindow::Move [7fc40fe45000] 2904.000000 84.000000
nsWindow::SetSizeMode [7fc40fe45000] 0

The last of these Move()s is outside this desktop and so the window ends up at

configure event [7fc40fe45000] 2880 133 1982 758

I'm suspicious of the way the window moves to the right each time, and seems
to move further to the right if it is "dropped" closer to the right.
It should be positioned where it is dropped.

Seems like something that could result from mis-applying a scaling factor somewhere.

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #5)
> It should be positioned where it is dropped.

At least that's what happens here at 96dpi, with constraining to the desktop or workarea.

Comment 7

[Marco Castelluccio [:marco]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b1806f5f3b8155a670f95a6e7f45d0cb4a044462&tochange=ea315d62656857d0e16cfde598ba14028813f2fc

Looks like this is a regression from bug 1240749, which is a fix for a regression from bug 890156.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jonathan, I gtb RC2 (desktop) in the next 2 hours. Should we consider backing out the offending patches from Fx47 similar to what we did in bug 1240749 for Fx46 release? Please let me know asap.

Comment 9

[Jonathan Kew [:jfkthame]]

We'd need to back out quite a lot (bug 890156 and everything that builds on top of it), which would in itself carry some risk (across all desktop platforms). While this issue could be an annoyance for the (relatively small number of) people who run into it, I don't think it's sufficiently critical to justify that extent of backout.

We should be able to come up with a patch here fairly promptly and aim to fix it in Fx48, I think.

Comment 10

[Jonathan Kew [:jfkthame]]

Actually, this does not seem to be a regression, or at least not a recent one (as from bug 1240749); I have just reproduced similar behavior with Firefox 44.0.2 on Ubuntu. (I can well believe that bug 1240749 and other dpi-related changes may have affected the precise conditions where it happens, but the issue already existed.)

What seems to be happening is that the dropped tab is (given the right circumstances, apparently depending on exactly where it's dropped) put into a new window that may be placed in a different workspace -- even though by default, Ubuntu doesn't have workspaces enabled. When this happens, I can locate and retrieve the "missing" window by going to System Settings / Appearance / Behavior, and checking "Enable workspaces"; then, bringing up the Workspace Switcher shows the rogue window in the next workspace over from the one that was being used.

From there, the window can be dragged back into the original workspace; or just enabling and then immediately disabling workspaces (even without opening up the Switcher) will cause it to gather the windows back onto the original workspace.

ISTM, then, that this is really a bug in the desktop manager, which should NOT be positioning windows in adjacent workspaces if the user doesn't have multiple workspaces enabled.

(It's still possible that there is a dpi-related scaling bug that's making this unfortunate behavior easier to trigger than in the past; I'll try to dig in a little deeper. But it's not new.)

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Jonathan for such a prompt and detailed investigation so far. Overholt and I have reviewed this bug and we agree that this is not release blocking. If this is an issue that was around since Fx44 (perhaps worsened a tiny bit) we might have to live with it for 6 more weeks as it is not an issue worth delaying Fx47 release.

If there is a Fx47 dot-release and this issue has a fix that has been verified on Nightly/Aurora, I will consider taking it as a ride-along.

Comment 12

[Marco Castelluccio [:marco]]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> Actually, this does not seem to be a regression, or at least not a recent
> one (as from bug 1240749); I have just reproduced similar behavior with
> Firefox 44.0.2 on Ubuntu. (I can well believe that bug 1240749 and other
> dpi-related changes may have affected the precise conditions where it
> happens, but the issue already existed.)

I've found the regression range with mozregression. I definitely can't reproduce
before and can reproduce after bug 1240749.
Maybe the issue was fixed after the build you tested and then bug 1240749
regressed it again.
Have you tried testing the two builds before and after bug 1240749?

> ISTM, then, that this is really a bug in the desktop manager, which should
> NOT be positioning windows in adjacent workspaces if the user doesn't have
> multiple workspaces enabled.
> 
> (It's still possible that there is a dpi-related scaling bug that's making
> this unfortunate behavior easier to trigger than in the past; I'll try to
> dig in a little deeper. But it's not new.)

I think it is a bug in Firefox. Like karlt says in comment 5, the window should
be dropped where I move the mouse and shouldn't move to the right.

To be extra clear, the behavior before bug 1240749 is that the window is dropped
exactly where I position the mouse. The behavior after bug 1240749 is that the
window isn't dropped exactly where I position the mouse, but moves to the right.
Sometimes the window goes off screen, sometimes it doesn't (but the fact that
the window moves is a bug in itself).

Comment 13

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #12)
> Maybe the issue was fixed after the build you tested and then bug 1240749
> regressed it again.

Yes, I've used mozregression again, this time to find a fix: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=69f7ba132363aa783954f185a5c88b16582da924&tochange=7d6e0141de4fab594b48d54663a4cc90fc78e248 (bug 1240085).

So looks like bug 1240085 fixed this issue and bug 1240749 regressed it again.

Comment 14

[Jonathan Kew [:jfkthame]]

Thanks, that's very helpful. So yes, it looks like this did regress in Fx47 (bug 1240749) after all, but it was also broken in Fx45 and earlier, and only fixed in Fx46 (bug 1240085) "by accident".

Given that the problem goes back many versions (I can reproduce at least as far back as Fx38), I don't think this really changes the assessment here that it doesn't block the current release; but we should try to re-fix properly -- and make it stick this time!

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I plan to keep this bug around for a few more weeks in case this becomes a dot-release driver/ride-along for Fx47. Otherwise, I will wontfix for Fx47 3 weeks from now.

Comment 16

[Marco Castelluccio [:marco]]

Updating the blocking field according to the findings in comment 13 and comment 14.

Comment 17

[Jim Mathies [:jimm]]

Karl, any plans to work on this?

Comment 18

[Karl Tomlinson (:karlt)]

I don't expect to have time to work on this in the near future.

Why are you asking?
If you are thinking of taking this bug or assigning to someone on your team that would be great, thanks.

Comment 19

[Jim Mathies [:jimm]]

This came up in platform triage.

Comment 20

[Andrew Comminos [:acomminos]]

This appears to be caused by interpreting the result of nsScreen::GetAvailRect as CSS pixels in browser/base/content/tabbrowser.xml when the units are actually device pixels. As a result, when the window scaling factor is > 1, the coordinates provided into nsWindow::Move have been scaled twice. I'll write up a patch.

Comment 21

[Andrew Comminos [:acomminos]]

Attachment: Bug 1276183 - Associate a widget with drag events wherever possible.

Review commit: https://reviewboard.mozilla.org/r/58582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58582/

Comment 22

[Andrew Comminos [:acomminos]]

This patch ensures that the GTK drag end coordinates are provided in logical display pixels, as windows and cocoa do. In addition, it transforms the screen rect obtained in tabbrowser.xml to logical display pixels as well, which should fix tab drag overshooting on cocoa (as discovered on mstange's mac). Thanks!

Comment 23

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/58582/#review55542

Thanks for looking at this.

::: browser/base/content/tabbrowser.xml:6121
(Diff revision 1)
> +        // Transform the screen rect into logical display pixels, which is the
> +        // coordinate system we handle events in. See bug 1276183.

I think you should get jfkthame to review this part.

I don't know what logical display pixels are.

I assume the event coords are in CSS pixels and need to be converted to global display coords for screenForRect. 

If the window remains within the same screen, then I assume the global display coords of the available rect need to be converted to CSS pixels using defaultCSSScaleFactor of that screen.

I don't know if or how resizeTo()/moveTo()/openDialog() can be used with another screen, but I would assume they use the CSS coordinates of the window object on which the method is called.  Therefore I assume defaultCSSScaleFactor needs to be obtained from the screen of window, unless it is available more directly from window.

I don't think referencing this bug is necessary or helpful, because this is not a workaround for any particular uncontrolled bug.  It should be sufficient to use the correct units.

::: widget/gtk/nsDragService.cpp:1427
(Diff revision 1)
>              gdk_display_get_pointer(display, nullptr, &x, &y, nullptr);
> -            SetDragEndPoint(LayoutDeviceIntPoint(x * scale, y * scale));
> +            // Leave the drag end point in display pixels- see bug 1276183.
> +            SetDragEndPoint(LayoutDeviceIntPoint(x, y));

This isn't quite right.

The GDK scale factor needs to be applied to GDK units to get device pixels.

Then device pixels need to be converted to CSS pixels.  These differ if the device/CSS scale factor is not from the GDK scale factor but from Xft.dpi, for example.

Also, the fact that the SetDragEndPoint() parameter is LayoutDeviceIntPoint means that it is expecting device pixels, so I wonder whether the conversion to CSS pixels is missing somewhere else.

Comment 24

[Andrew Comminos [:acomminos]]

Thanks for the feedback, Karl. I spent some time digging into the origins of the units used and things seem more clear.

(In reply to Karl Tomlinson (ni?:karlt) from comment #23)
> https://reviewboard.mozilla.org/r/58582/#review55542
> 
> I don't know what logical display pixels are.

Ah sorry, I was referring to CSS pixels here.

> I assume the event coords are in CSS pixels and need to be converted to
> global display coords for screenForRect.

Right, I believe so.
 
> I don't know if or how resizeTo()/moveTo()/openDialog() can be used with
> another screen, but I would assume they use the CSS coordinates of the
> window object on which the method is called.  Therefore I assume
> defaultCSSScaleFactor needs to be obtained from the screen of window, unless
> it is available more directly from window.

You're correct, we should use the dev pixel -> CSS scaling ratio of the screen the widget is on (as AFAICT it defines the scale of all screens to the widget).

> Also, the fact that the SetDragEndPoint() parameter is LayoutDeviceIntPoint
> means that it is expecting device pixels, so I wonder whether the conversion
> to CSS pixels is missing somewhere else.

This is likely the best route to go; I'm going to try and make this the case across platforms.

Comment 25

[Andrew Comminos [:acomminos]]

So because the WidgetDragEvent created by nsDragService lacks a widget, we are unable to obtain a CSS scale factor and thus fallback to returning the device pixels from Event::GetScreenCoords;

> // Doing a straight conversion from LayoutDeviceIntPoint to CSSIntPoint
> // seem incorrect, but it is needed to maintain legacy functionality.
> WidgetGUIEvent* guiEvent = aEvent->AsGUIEvent();
> if (!aPresContext || !(guiEvent && guiEvent->mWidget)) {
>   return CSSIntPoint(aPoint.x, aPoint.y);
> }

It would make sense then that the other widget backends prematurely scale the drop coordinates, as documented in bug 818927.

Jonathan, thoughts? Should we continue as other platforms do and prematurely convert to CSS pixels, back the drag event with a widget, or something else entirely?

Comment 26

[Karl Tomlinson (:karlt)]

(In reply to Andrew Comminos [:acomminos] from comment #25)
> So because the WidgetDragEvent created by nsDragService lacks a widget, we
> are unable to obtain a CSS scale factor

Drag end events are sent to the source element, so I guess they could be associated with the source widget?

Comment 27

[Jonathan Kew [:jfkthame]]

(In reply to Andrew Comminos [:acomminos] from comment #25)
> Jonathan, thoughts? Should we continue as other platforms do and prematurely
> convert to CSS pixels, back the drag event with a widget, or something else
> entirely?

Ideally, I think we should probably be working with "desktop pixel" coordinates here, as they're the coordinate type that is intended to be usable across a potentially multi-screen, mixed-resolution desktop space. I'm not sure how consistently they're currently handled in the gtk widget code, though; as we don't yet have mixed-resolution support there, things may not be fully developed.

Converting to CSS pixels would probably work OK as a stopgap, until we have full support for multiple resolutions.

Comment 28

[Andrew Comminos [:acomminos]]

Attachment: Bug 1276183 - Use correct units when interfacing with nsIScreenManager in tabbrowser.xml.

Review commit: https://reviewboard.mozilla.org/r/63728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63728/

Comment 29

[Andrew Comminos [:acomminos]]

Comment on attachment 8761346 [details]
Bug 1276183 - Associate a widget with drag events wherever possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58582/diff/1-2/

Comment 30

[Andrew Comminos [:acomminos]]

These patches fix the issue by associating a widget wherever possible with a drag event, which will downscale coordinates by using the widget's scale factor before exposing to the DOM. Units are corrected to be display pixels when interacting with nsIScreenManager and nsIScreen, and CSS pixels in other calculations. This fixes the issue quite nicely in my testing.

Thanks!

Comment 31

[Jonathan Kew [:jfkthame]]

https://reviewboard.mozilla.org/r/58582/#review61588

I've just been trying this on Windows 10 with multiple, mixed-dpi monitors, and unfortunately it has undesired effects there; specifically, I'm seeing the destination position (on an external lo-dpi monitor) improperly constrained to just a part of the screen, suggesting that an incorrect scale factor is being used at some point.

I'll try to dig in a bit and figure out where the problem arises, but wanted to note that as things stand, this regresses behavior on Windows, at least. :(

Comment 32

[Jonathan Kew [:jfkthame]]

In the process of looking into this, I've run across a somewhat similar window-placement and coordinate-scaling bug that reproduces on retina Macs. Filed as bug 1287734. We should include that patch in ongoing investigation here, although by itself it does not solve the Linux issue here.

Comment 33

[Jonathan Kew [:jfkthame]]

Attachment: Fix pixel-unit mismatch when constraining a dropped tab to the screen

AFAICT, this fixes the problem where the window may end up off-screen; it also causes the window to be correctly constrained to the available screen area on Windows hi-dpi systems, whereas currently it may be mostly off-screen when dropped. This does *not* fix the problem that the window may not appear at the expected drop location on secondary screens. That seems to be related to coordinates recorded in the drag event, but I haven't totally figured out how to deal with it yet. I think we should spin that off into a followup bug, though, as this patch fixes the most serious part of the issue. Andrew, can you confirm this prevents the 'lost window' problem here?

Comment 34

[Andrew Comminos [:acomminos]]

Comment on attachment 8772441 [details] [diff] [review]
Fix pixel-unit mismatch when constraining a dropped tab to the screen

Review of attachment 8772441 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this should do it; although windows may still appear on the wrong screen, it's a step in the right direction (at least they'll be bounded to it).

Comment 35

[Masatoshi Kimura [:emk]]

Comment on attachment 8772441 [details] [diff] [review]
Fix pixel-unit mismatch when constraining a dropped tab to the screen

Review of attachment 8772441 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +6129,5 @@
> +          screen.contentsScaleFactor / screen.defaultCSSScaleFactor;
> +        sX.value *= scaleFactor;
> +        sY.value *= scaleFactor;
> +        sWidth.value *= scaleFactor;
> +        sHeight.value *= scaleFactor;

Shouldn't this calculation be
  availRectCSSPx = (availRectDeskPx - rectDeskPx) * scaleFactor + rectDeskPx
?

Comment 36

[Jonathan Kew [:jfkthame]]

Hmm, yeah, seems like it should be. I'll need to re-test that across platforms...

Comment 37

[Jonathan Kew [:jfkthame]]

Attachment: Fix pixel-unit mismatch when constraining a dropped tab to the screen

OK, this should be more correct.

Comment 38

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5baa3f32ca4945ec50f726798774369e51abf64
Bug 1276183 - Fix pixel-unit mismatch when constraining a dropped tab to the screen. r=emk

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5baa3f32ca4

Comment 40

[Gerry Chang [:gchang]]

Hi :jfkthame,
It's too late for 48. Since this bug also affects 49, are you also considering to uplift this patch to 49?

Comment 41

[Jonathan Kew [:jfkthame]]

Comment on attachment 8761346 [details]
Bug 1276183 - Associate a widget with drag events wherever possible.

Clearing r? on the patches we didn't end up using here.

Comment 42

[Gerry Chang [:gchang]]

Hi :acomminos,
Per comment #40, are you considering to uplift this patch to 49?

Comment 43

[Jonathan Kew [:jfkthame]]

Comment on attachment 8772724 [details] [diff] [review]
Fix pixel-unit mismatch when constraining a dropped tab to the screen

Even after this fix, we still have some issues with positioning when dragging a tab across multi-screen systems (see bug 1288760), but I think the fix here is important enough that we should take it to resolve the "lost window" problem.

Approval Request Comment
[Feature/regressing bug #]: 1240749

[User impact if declined]: possible to "lose" a window (ends up entirely off-screen) when dragging across multiple monitors on Linux

[Describe test coverage new/current, TreeHerder]: tested manually (we don't have automated testing on multi-monitor configurations)

[Risks and why]: low, just corrects the pixel units being used when checking screen bounds

[String/UUID change made/needed]: none

Comment 44

[Gerry Chang [:gchang]]

Comment on attachment 8772724 [details] [diff] [review]
Fix pixel-unit mismatch when constraining a dropped tab to the screen

Review of attachment 8772724 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes a regression. Take it in 49 beta. Should be in 49 beta 4.

Comment 45

[Gerry Chang [:gchang]]

Hi :jfkthame,
This patch is already in 50 aurora. I think the uplift request should be 49 beta, right?

Comment 46

[Gerry Chang [:gchang]]

Comment on attachment 8772724 [details] [diff] [review]
Fix pixel-unit mismatch when constraining a dropped tab to the screen

Review of attachment 8772724 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is already in 50 aurora.

Comment 47

[Jonathan Kew [:jfkthame]]

Comment on attachment 8772724 [details] [diff] [review]
Fix pixel-unit mismatch when constraining a dropped tab to the screen

Oh, right - sorry, I lost track of version numbers briefly there!

OK, so consider that comment 43 refers to the mozilla-beta request.

Comment 48

[Gerry Chang [:gchang]]

Comment on attachment 8772724 [details] [diff] [review]
Fix pixel-unit mismatch when constraining a dropped tab to the screen

Review of attachment 8772724 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes a regression. Take it in 49 beta. Should be in 49 beta 5.

Comment 49

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7d71471a27e0

Comment 50

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I didn't manage to reproduce this issue. I used 49.0a1 (2016-05-27), 46.0a1 (2016-01-01) on Ubuntu 14.04 x86 and Ubuntu 16.04 x86, with a single monitor and also two monitors, with workspaces enabled and disabled, but no luck. Are there needed additional steps / settings?

Comment 51

[Jonathan Kew [:jfkthame]]

(In reply to Iulia Cristescu, QA [:IuliaC] from comment #50)
> I didn't manage to reproduce this issue. I used 49.0a1 (2016-05-27), 46.0a1
> (2016-01-01) on Ubuntu 14.04 x86 and Ubuntu 16.04 x86, with a single monitor
> and also two monitors, with workspaces enabled and disabled, but no luck.
> Are there needed additional steps / settings?

Are you using a hi-dpi display? (Quick check: what devicePixelRatio does https://people.mozilla.org/~jkew/tests/devPixRatio.html report? I think you want it to be at least 1.5 or more.)

Then, open a Firefox window with two tabs, and size/position it so that there's a bit of free desktop space to the right of it. Drag one of the tabs by its title (from the tab-bar) over to the desktop at the right of the screen, so that Firefox detaches it from the existing window and gives it a new window of its own. On a hi-dpi system suffering from this bug, that new window may be off-screen and therefore "lost".

Comment 52

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to Jonathan Kew (:jfkthame) from comment #51)
> (In reply to Iulia Cristescu, QA [:IuliaC] from comment #50)
> > I didn't manage to reproduce this issue. I used 49.0a1 (2016-05-27), 46.0a1
> > (2016-01-01) on Ubuntu 14.04 x86 and Ubuntu 16.04 x86, with a single monitor
> > and also two monitors, with workspaces enabled and disabled, but no luck.
> > Are there needed additional steps / settings?
> 
> Are you using a hi-dpi display? (Quick check: what devicePixelRatio does
> https://people.mozilla.org/~jkew/tests/devPixRatio.html report? I think you
> want it to be at least 1.5 or more.)
> 
> Then, open a Firefox window with two tabs, and size/position it so that
> there's a bit of free desktop space to the right of it. Drag one of the tabs
> by its title (from the tab-bar) over to the desktop at the right of the
> screen, so that Firefox detaches it from the existing window and gives it a
> new window of its own. On a hi-dpi system suffering from this bug, that new
> window may be off-screen and therefore "lost".

Thank you, Jonathan! It seems that the devicePixelRatio was the decisive factor in order to reproduce this bug. I confirm that the issue is not reproducible anymore on 
- latest Aurora 50.0a2 (2016-09-14)
- 49.0 build3 (20160912134115)
I will set the corresponding flags.