Torn-off tab no longer opens on the screen it was dragged to

VERIFIED FIXED in Firefox 50

Status

()

defect
P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: RyanVM, Assigned: jfkthame)

Tracking

(Depends on 1 bug, {regression})

Trunk
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 unaffected, firefox48 unaffected, firefox49 unaffected, firefox50 verified, firefox51 verified)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

3 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.
Flags: needinfo?(jfkthame)
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).
(Reporter)

Comment 3

3 years ago
FWIW, I have two identical monitors running with the same DPI settings on both.
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.
Flags: needinfo?(ryanvm)
(Reporter)

Comment 5

3 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
Flags: needinfo?(ryanvm)
Confirmed. (eX, eY) looks like CSS pixels.
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
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.
(Assignee)

Comment 10

3 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.
Flags: needinfo?(jfkthame)
Ah, you are right. Secondary monitors should have >1920 x coordinates even in CSS pixels on per-monitor dpi systems.
(Assignee)

Comment 14

3 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).
Attachment #8775552 - Flags: review?(jfkthame) → review+

Comment 16

3 years ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/10fccfc043b7
Fix point conversion in nsDragService::StartInvokingDragSession. r=jfkthame
I landed the first part earlier because it will fix the most obvious regression and it will only affect Windows.
Keywords: leave-open
(Assignee)

Comment 18

3 years ago
Yes, that makes sense, thanks.
(Assignee)

Comment 20

3 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...
Attachment #8775992 - Flags: feedback?(VYV03354)
(Assignee)

Comment 21

3 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.
Attachment #8775995 - Flags: feedback?(VYV03354)
(Assignee)

Comment 23

3 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.
Attachment #8776054 - Flags: review?(VYV03354)
(Assignee)

Updated

3 years ago
Attachment #8775992 - Flags: feedback?(VYV03354) → review?(VYV03354)
(Assignee)

Updated

3 years ago
Attachment #8775995 - Flags: feedback?(VYV03354) → review?(VYV03354)
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?
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.)
Attachment #8775995 - Flags: review?(VYV03354) → review+
Attachment #8776054 - Flags: review?(VYV03354) → review+
(Assignee)

Comment 26

3 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.
(Assignee)

Comment 27

3 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...
(Assignee)

Comment 28

3 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).
Flags: needinfo?(VYV03354)
I'm afraid I can't test and verify the behavior on mixed-DPI Mac systems myself.
Flags: needinfo?(VYV03354)
(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?
Flags: needinfo?(VYV03354)
I'm waiting :jfkthame return from PTO.
Flags: needinfo?(VYV03354)
(Assignee)

Comment 32

3 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.
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Attachment #8775992 - Attachment is obsolete: true
Attachment #8775992 - Flags: review?(VYV03354)
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!
Attachment #8781947 - Flags: review?(VYV03354) → review+
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.
Attachment #8781948 - Flags: review?(VYV03354) → review+
(Assignee)

Comment 37

3 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!
(Assignee)

Comment 38

3 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
(Assignee)

Updated

3 years ago
Keywords: leave-open
Hi Jonathan, should we uplift the fix to Aurora50 given that this is a P1 regression?
Flags: qe-verify+
Flags: needinfo?(jfkthame)
(Assignee)

Comment 41

3 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
Flags: needinfo?(jfkthame)
Attachment #8781947 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8781948 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8775995 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8776054 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(jfkthame)
Hi Ryan, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(ryanvm)
(Reporter)

Comment 44

3 years ago
Yeah, working great again.
Flags: needinfo?(ryanvm)
Status: RESOLVED → VERIFIED
(Reporter)

Comment 45

3 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?
(Assignee)

Comment 46

3 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.
Flags: needinfo?(jfkthame)
(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.
Attachment #8775995 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8776054 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8781947 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8781948 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(Reporter)

Updated

3 years ago
Depends on: 1301694
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.
Flags: qe-verify+
QA Contact: cornel.ionce

Updated

3 years ago
Depends on: 1305278
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.
I filed bug 1306309 to get the 4 patches relanded.

Updated

2 years ago
Depends on: 1326877
You need to log in before you can comment on or make changes to this bug.