Ryan VanderMeulen [:RyanVM] Reporter
	Description • 8 years ago

STR:
1. Have two monitors.
2. Maximize Firefox.
3. Open multiple tabs.
4. Tear off one of the tabs to the other monitor.

Expected Results: Tab should open a new maximized window on the monitor it was dragged to.

Actual Results: Tab opens a maximized window on the same monitor as the existing maximized window.

Reproduced on Windows 10, no idea if it reproduces on other platforms as well. Bisected the cause down to bug 1276183 via mozregression.

	Masatoshi Kimura [:emk]
	Comment 1 • 8 years ago

I could reproduce this with the following configuration.

Display 1 (primary): DPI=1.0, rect=(0,0,1280,960)
Display 2 (secondary): DPI=1.5, rect=(1280,0,1920,1440) in desktop pixels / (1280,0,1280,960) in CSS pixels

(eX,eY) was (1205,141) even if I released the mouse button on the left half of secondary monitor. Looks like the drop event does not calculate the mouse coordinates correctly.

I could drop the tab on the secondary monitor if I released the mouse on the right half of the secondary monitor (probably because the DPI scale of the secondary monitor is 1.5).

	Masatoshi Kimura [:emk]
	Comment 2 • 8 years ago

Maybe here?
https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/events/Event.cpp#935-938

It does not look correct correct on mixed-dpi systems.

	Ryan VanderMeulen [:RyanVM] Reporter
	Comment 3 • 8 years ago

FWIW, I have two identical monitors running with the same DPI settings on both.

	Masatoshi Kimura [:emk]
	Comment 4 • 8 years ago

What is your exact monitor configuration? Please open https://people.mozilla.org/~jkew/tests/devPixRatio.html on each monitor, maximize the window, and paste the result.

	Ryan VanderMeulen [:RyanVM] Reporter
	Comment 5 • 8 years ago

Dual 4K monitors running at native resolution with 200% scaling set at the Windows level (Win10).

Monitor 1:
Device Pixel Ratio: 2
innerWidth: 1920
innerHeight: 971
screenX: -7
screenY: -7

Monitor 2:
Device Pixel Ratio: 2
innerWidth: 1920
innerHeight: 971
screenX: 3834
screenY: -7

	Masatoshi Kimura [:emk]
	Comment 6 • 8 years ago

Confirmed. (eX, eY) looks like CSS pixels.

	Masatoshi Kimura [:emk]
	Comment 7 • 8 years ago

I applied this patch and got the following output from the browser console:
[eX, eY]: 1216,109
fullDesk: 0,0,1920,1440
availDesk: 0,0,1920,1360
scale: 0.5 = 1 / 2
availCSS: 0,0,960,680
window.outer: 973,693
win: 960,680
[left, top]: 0,0

	Masatoshi Kimura [:emk]
	Comment 8 • 8 years ago
wrong

So this caluculation is wrong. It compares CSS pixels with desktop pixels.
https://dxr.mozilla.org/mozilla-central/rev/462dc6b44adb4573e8ce6b0dd688c206ebd516f7/browser/base/content/tabbrowser.xml#6153-6156

	Masatoshi Kimura [:emk]
	Comment 9 • 8 years ago

The previous comment was wrong. screenForRect expects desktop pixels while (eX, eY) is CSS pixels.

Given that MouseEvent.screenX/Y is web-exposed, we can't change it. We will need screenForRectCSSPx to fix this.

	Jonathan Kew [:jfkthame] Assignee
	Comment 10 • 8 years ago

I think we also have a problem because the values of MouseEvent.screenX/Y are in CSS-pixel units, but not adjusted for the origin of the appropriate screen. This may be the main reason we end up with the wrong screen in some cases.

	Masatoshi Kimura [:emk]
	Comment 11 • 8 years ago

Ah, you are right. Secondary monitors should have >1920 x coordinates even in CSS pixels on per-monitor dpi systems.

	Masatoshi Kimura [:emk]
	Comment 12 • 8 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 13 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45e154084df9

	Jonathan Kew [:jfkthame] Assignee
	Comment 14 • 8 years ago

Comment on attachment 8775552 [details]
Bug 1288760 - Fix point conversion in nsDragService::StartInvokingDragSession.

https://reviewboard.mozilla.org/r/67732/#review64884

Thanks for looking into this. This looks OK, I think, and fixes the most glaring problem: it keeps the window on the correct screen. However, I'm still seeing issues where it doesn't actually appear at the expected drop location.

I'm testing a second patch that seems to help with that (but need to check behavior on other platforms as well, before I post additional patches for review).

	Masatoshi Kimura [:emk]
	Comment 15 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5373b0548946

	Pulsebot
	Comment 16 • 8 years ago

Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/10fccfc043b7
Fix point conversion in nsDragService::StartInvokingDragSession. r=jfkthame

	Masatoshi Kimura [:emk]
	Comment 17 • 8 years ago

I landed the first part earlier because it will fix the most obvious regression and it will only affect Windows.

	Jonathan Kew [:jfkthame] Assignee
	Comment 18 • 8 years ago

Yes, that makes sense, thanks.

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/10fccfc043b7

	Jonathan Kew [:jfkthame] Assignee
	Comment 20 • 8 years ago

So here's what I have at the moment: this patch to Event::GetScreenCoords results in much better behavior when dragging tabs across mixed-resolution screens; the resulting window appears at the proper drop position, at least in the configurations I've tested so far. Unfortunately, this breaks the positioning of popups (context menus, tooltips) on hi-dpi screens...

	Jonathan Kew [:jfkthame] Assignee
	Comment 21 • 8 years ago

Then this patch fixes the positioning of popups, but only in non-e10s. With e10s enabled, popups from content are still misplaced on hi-dpi screens, which suggests a separate codepath is involved which also needs a correction to its coordinate conversions, but I haven't identified where to do that yet.

	Jonathan Kew [:jfkthame] Assignee
	Comment 22 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=342e571e1177

	Jonathan Kew [:jfkthame] Assignee
	Comment 23 • 8 years ago

The problem with content popup positioning under e10s occurs because ScreenProxy fails to implement the GetContentsScaleFactor and GetDefaultCSSScaleFactor methods, so it just inherits the nsBaseScreen versions that return a fixed 1.0 scale. With this fixed, I'm seeing context menus and tooltips positioned consistently in both non-e10s and e10s modes. So I _think_ (subject to further testing) that the three patches 2.1-2.3 together fix the issues sufficiently that we could consider landing them.

	Masatoshi Kimura [:emk]
	Comment 24 • 8 years ago

Comment on attachment 8775992 [details] [diff] [review]
patch 2.1 - Fix point conversion in Event::GetScreenCoords to return global-desktop-based CSS pixels

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

::: dom/events/Event.cpp
@@ +938,2 @@
>      pt = pt.RemoveResolution(nsLayoutUtils::GetCurrentAPZResolutionScale(ps));
> +    aPoint = LayoutDevicePixel::FromAppUnitsRounded(pt, appPerDevFullZoom);

Could you use LayoutDevicePoint and CSSPoint for intermediate results to reduce rounding errors? (Same for following calculations in this patch.)

@@ +944,5 @@
> +  nsCOMPtr<nsIScreenManager> screenMgr =
> +    do_GetService("@mozilla.org/gfx/screenmanager;1");
> +  if (screenMgr) {
> +    nsCOMPtr<nsIScreen> screen;
> +    screenMgr->ScreenForRect(aPoint.x, aPoint.y, 1, 1, getter_AddRefs(screen));

Can layout pixels uniquely identify a monitor and a position on mixed-DPI mac systems? (Part 1 did not have this problem because it was Windows specific.) Shouldn't we take the screen from guiEvent->mWidget?

	Masatoshi Kimura [:emk]
	Comment 25 • 8 years ago

Comment on attachment 8775995 [details] [diff] [review]
patch 2.2 - Make InitializePopupAtScreen handle position specified in global CSS-pixel values

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

::: layout/xul/nsMenuPopupFrame.cpp
@@ +792,5 @@
> +  if (screenMgr) {
> +    // aXPos and aYPos are "global desktop" coordinates in units of
> +    // CSS pixels, but nsMenuPopupFrame wants CSS pixels that are
> +    // directly scaled from device pixels, without being offset for
> +    // the screen origin. So we need to undo the offset that was

Shouldn't nsMenuPopupFrame be fixed? (A follow-up bug is fine though.)

	Jonathan Kew [:jfkthame] Assignee
	Comment 26 • 8 years ago

(In reply to Masatoshi Kimura [:emk] from comment #24)
> Comment on attachment 8775992 [details] [diff] [review]
> Could you use LayoutDevicePoint and CSSPoint for intermediate results to
> reduce rounding errors? (Same for following calculations in this patch.)

I've changed it to use a LayoutDevicePoint for the intermediate device-px value. There's no benefit to making cssPt a float, because the method result is a CSSIntPoint and the only thing we're going to do with cssPt after its initial computation is to add an integer value to it. So rounding early or late makes no difference to the result there.

> Can layout pixels uniquely identify a monitor and a position on mixed-DPI
> mac systems? (Part 1 did not have this problem because it was Windows
> specific.) Shouldn't we take the screen from guiEvent->mWidget?

Ah, I think you're right, we should use ScreenForNativeWidget instead. I guess I hadn't tested with the right Mac screen configuration to actually run into this problem.

	Jonathan Kew [:jfkthame] Assignee
	Comment 27 • 8 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #26)
> > Can layout pixels uniquely identify a monitor and a position on mixed-DPI
> > mac systems? (Part 1 did not have this problem because it was Windows
> > specific.) Shouldn't we take the screen from guiEvent->mWidget?
> 
> Ah, I think you're right, we should use ScreenForNativeWidget instead. I
> guess I hadn't tested with the right Mac screen configuration to actually
> run into this problem.

Hmm, but that doesn't work when content in e10s windows wants to show a context menu or tooltip. :(

Oh, but wait a minute... nsIScreenManager::ScreenForRect expects the coordinates in desktop pixels anyway. So what we need to do is to convert the layout px that we're given into desktop px -- which we can do using the scale from guiEvent->mWidget -- and then call ScreenForRect. Let me try it that way...

	Jonathan Kew [:jfkthame] Assignee
	Comment 28 • 8 years ago

Ugh, I'm still seeing problems on a mixed-DPI Mac setup, e.g. configured with a hi-dpi primary screen and a lo-dpi secondary monitor to the right of it.

Unfortunately, I'm on PTO and off-line for the next week, and won't have a chance to try and get this working properly right now. Masatoshi, if you have time to look into this further, and want to try and finish up the pt 2.1 patch so that it's ready to land, that would be awesome; otherwise, I'll pick this up again after I'm back (around Aug 8th).

	Masatoshi Kimura [:emk]
	Comment 29 • 8 years ago

I'm afraid I can't test and verify the behavior on mixed-DPI Mac systems myself.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 30 • 8 years ago

(In reply to Masatoshi Kimura [:emk] from comment #24)
> Comment on attachment 8775992 [details] [diff] [review]
> patch 2.1 - Fix point conversion in Event::GetScreenCoords to return
> global-desktop-based CSS pixels
> 
> Review of attachment 8775992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/events/Event.cpp
> @@ +938,2 @@
> >      pt = pt.RemoveResolution(nsLayoutUtils::GetCurrentAPZResolutionScale(ps));
> > +    aPoint = LayoutDevicePixel::FromAppUnitsRounded(pt, appPerDevFullZoom);
> 
> Could you use LayoutDevicePoint and CSSPoint for intermediate results to
> reduce rounding errors? (Same for following calculations in this patch.)
> 
> @@ +944,5 @@
> > +  nsCOMPtr<nsIScreenManager> screenMgr =
> > +    do_GetService("@mozilla.org/gfx/screenmanager;1");
> > +  if (screenMgr) {
> > +    nsCOMPtr<nsIScreen> screen;
> > +    screenMgr->ScreenForRect(aPoint.x, aPoint.y, 1, 1, getter_AddRefs(screen));
> 
> Can layout pixels uniquely identify a monitor and a position on mixed-DPI
> mac systems? (Part 1 did not have this problem because it was Windows
> specific.) Shouldn't we take the screen from guiEvent->mWidget?

Is there more that you are planning on reviewing here or can the review flag be changed to r-, r+ or cleared?

	Masatoshi Kimura [:emk]
	Comment 31 • 8 years ago

I'm waiting :jfkthame return from PTO.

	Jonathan Kew [:jfkthame] Assignee
	Comment 32 • 8 years ago

I've realized there's a better way to get the widget's current screen: we already have a GetWidgetScreen() method defined in nsBaseWidget, so we shouldn't go re-implementing that functionality. All we need to do is move that up to the nsIWidget interface (which is trivial, as its implementation doesn't depend on anything that isn't already declared in nsIWidget) and then we can directly query the guiEvent->mWidget for its screen.

This simplifies things a bit in Event::GetScreenCoords, and so far it seems to be working as expected; I will re-test across platforms once my try build (https://treeherder.mozilla.org/#/jobs?repo=try&revision=c83e58736a6e) is ready to use.

	Masatoshi Kimura [:emk]
	Comment 35 • 8 years ago

Comment on attachment 8781947 [details] [diff] [review]
patch 2.0 - Move nsBaseWidget::GetWidgetScreen() up to the nsIWidget interface, to make it usable from DOM Event code

Nice!

	Masatoshi Kimura [:emk]
	Comment 36 • 8 years ago

Comment on attachment 8781948 [details] [diff] [review]
patch 2.1 - Fix point conversion in Event::GetScreenCoords to return global-desktop-based CSS pixels

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

::: dom/events/Event.cpp
@@ +939,2 @@
>      pt = pt.RemoveResolution(nsLayoutUtils::GetCurrentAPZResolutionScale(ps));
> +    devPt = LayoutDevicePixel::FromAppUnitsRounded(pt, appPerDevFullZoom);

Shouldn't it be FromAppUnits (without rounded)?
r=me with this fixed or explained.

	Jonathan Kew [:jfkthame] Assignee
	Comment 37 • 8 years ago

(In reply to Masatoshi Kimura [:emk] from comment #36)
> Shouldn't it be FromAppUnits (without rounded)?

Oops, yes, of course it should - thanks for catching!

	Jonathan Kew [:jfkthame] Assignee
	Comment 38 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/844995477d5693876d8297e31fab5bbc4fd21aa5
Bug 1288760 patch 2.0 - Move nsBaseWidget::GetWidgetScreen() up to the nsIWidget interface, to make it usable from DOM Event code. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d7c99e08e06113cb369eb79b90be396f9b9761f
Bug 1288760 patch 2.1 - Fix point conversion in Event::GetScreenCoords to return global-desktop-based CSS pixels. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c2de759518a613fbd938d20613c7753c0a48ed6
Bug 1288760 patch 2.2 - Make InitializePopupAtScreen handle position specified in global CSS-pixel values. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb5775fd334887121a7a779a27e6727acdcb25f
Bug 1288760 patch 2.3 - Support GetContentsScaleFactor and GetDefaultCSSScaleFactor in ScreenProxy. r=emk

	Ryan VanderMeulen [:RyanVM] Reporter
	Comment 39 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/844995477d56
https://hg.mozilla.org/mozilla-central/rev/5d7c99e08e06
https://hg.mozilla.org/mozilla-central/rev/9c2de759518a
https://hg.mozilla.org/mozilla-central/rev/4eb5775fd334

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 40 • 8 years ago

Hi Jonathan, should we uplift the fix to Aurora50 given that this is a P1 regression?

	Jonathan Kew [:jfkthame] Assignee
	Comment 41 • 8 years ago

Comment on attachment 8781947 [details] [diff] [review]
patch 2.0 - Move nsBaseWidget::GetWidgetScreen() up to the nsIWidget interface, to make it usable from DOM Event code

Note that the most serious regression - the window not appearing on the correct destination screen - was fixed by the first patch (attachment 8775552 [details]), which landed for mozilla-50. The subsequent patches, which landed for mozilla-51, address the remaining issue of the new window potentially being misplaced relative to the drop location, but IMO this is a less serious part of the overall problem.


Approval Request Comment
(for the complete set of patches, parts 2.0-2.3)

[Feature/regressing bug #]: 1276183

[User impact if declined]: Possibly misplaced window when dragging a tab out to the desktop (across multiple monitors)

[Describe test coverage new/current, TreeHerder]: Manually tested

[Risks and why]: moderate risk, the patches do potentially affect coordinate management across all platforms -- though for the majority of users (on single-monitor configurations) there should be no effect

[String/UUID change made/needed]: no

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 42 • 8 years ago

Hi Jonathan, if the first patch addresses the regression in a way that is satisfactory and uplifting all the other patches adds more risk to Aurora50, then I'd be happy to leave things as is. The fact that status-firefox50 flag still shows "affected" brought this one to my attention.

Do you still think we should uplift the remaining patches? I think we can live with the misplaced window locations vs. missing window.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 43 • 8 years ago

Hi Ryan, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

	Ryan VanderMeulen [:RyanVM] Reporter
	Comment 44 • 8 years ago

Yeah, working great again.

	Ryan VanderMeulen [:RyanVM] Reporter
	Comment 45 • 8 years ago

I can also confirm that Aurora50 is behaving reasonably well with just the patch from comment 19, at least with my two 4K monitors. Maybe the location issue is more pronounced with screens at different resolutions or DPIs?

	Jonathan Kew [:jfkthame] Assignee
	Comment 46 • 8 years ago

(In reply to Ritu Kothari (:ritu) from comment #42)
> Hi Jonathan, if the first patch addresses the regression in a way that is
> satisfactory and uplifting all the other patches adds more risk to Aurora50,
> then I'd be happy to leave things as is. The fact that status-firefox50 flag
> still shows "affected" brought this one to my attention.
> 
> Do you still think we should uplift the remaining patches? I think we can
> live with the misplaced window locations vs. missing window.

Although the patches have landed safely on Nightly, and I'm not aware of any regressions, I think this is a sufficiently tricky area (and one that we can't currently cover with automated tests) that no changes can be considered risk-free.

So I'd be OK with letting the "part 2" set of patches ride the trains, if you're more comfortable with that; I don't think the positioning glitch that remains is particularly critical, and it should only affect a small minority of users.

status-firefox50 should be "partially fixed", I guess, but we don't support that option. :)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #45)
> I can also confirm that Aurora50 is behaving reasonably well with just the
> patch from comment 19, at least with my two 4K monitors. Maybe the location
> issue is more pronounced with screens at different resolutions or DPIs?

Yes, it would be -- I'm not sure offhand whether it would occur at all with matching resolutions.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 47 • 8 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #46)
> (In reply to Ritu Kothari (:ritu) from comment #42)

> 
> So I'd be OK with letting the "part 2" set of patches ride the trains, if
> you're more comfortable with that; I don't think the positioning glitch that
> remains is particularly critical, and it should only affect a small minority
> of users.
> 
> status-firefox50 should be "partially fixed", I guess, but we don't support
> that option. :)
> 

Great! Thanks as always for providing such a detailed risk analysis. I will mark this as fixed (in absence of partially fixed since the severe bit of "missing window" problem is fixed) and we will let the full set of patches ride the 51 train.

	Cornel Ionce [:noni] [Hubs QA]
	Comment 48 • 8 years ago

Confirming that this issue no longer reproduces on Win 10x64 using latest 50.0a2 Aurora, build ID 20160913004005.
Tested using the STR from the description on a setup with two FHD monitors.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 49 • 8 years ago

In bug 1300421 I backed out the last 4 patches from this bug (part 2.0, 2.1, 2.2, and 2.3) to fix a regression.

My understanding is that this bug is still "fixed" because the first patch fixes the main part of this bug, so I'm going to leave this bug as resolved, and file a follow-up for getting the other 4 patches relanded.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 50 • 8 years ago

I filed bug 1306309 to get the 4 patches relanded.