1239353 - Tearaway tab from lodpi to hidpi screen shows preview in wrong location

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P1)

Description

[Chris H-C :chutten]

Attachment: whatadrag.png

Nightly 46 2016-01-13

STR:

1) On a lodpi screen, tear a tab off to get that nice preview screenshot following the cursor
2) Move cursor to hidpi screen

AR: preview no longer close to the cursor but way off
ER: preview to continue following the cursor.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Out of curiosity, are you able to reproduce this on Release as well?

Comment 2

[Chris H-C :chutten]

Oddly, no. I get a different kind of preview, too. A bigger one with an opacity gradient.

And I noticed after filing this that the tab I tore away was scaled improperly when I dropped it, but was fine after I moved it a bit. But I can't reproduce that now.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Last test - can you reproduce this on Nightly with e10s disabled?

Comment 4

[Chris H-C :chutten]

E10s-disabled I get the opacity-gradient preview, which doesn't exhibit the issue (though it does exhibit a different one where the tearaway tab's window isn't visible. Not sure where it's positioned, but it isn't on either screen...)

E10s-enabled I get the fully-opaque (live?) preview, which does exhibit the issue.

Comment 5

[Tracy Walker [:tracy]]

On Mac, what I see is that the preview doesn't follow the cursor to the retina screen at all.  That or the preview is so far off that no matter where my mouse is on the retina screen the preview doesn't show up there.

Comment 6

[Chris H-C :chutten]

:tracy, I see that too on Win10. 

Note that this bug is ostensibly for the opposite direction: hidpi => lodpi. But, yes, there's a preview location coordinate scaling bug in both directions.

Comment 7

[Gabor Krizsanits (INACTIVE)]

I've tried this out today, this looks horrible, I definitely would flag this as m8. I'm going to look into this and see if I can figure out where is the issue is coming from.

Comment 8

[Gabor Krizsanits (INACTIVE)]

Something is really wrong here on all platforms. On OSX a weird re-scaling happening when the tab image reaches the end of the screen. I think it happens because on the other screen the css-pixel/screen-pixel ratio is different, so we need to adjust the frames. I don't think that operation is very successful but what's even worse that the coordinates of the dragged panel are going wild. So nothing appears on the new screen and if I drag it back it keeps the size of from the other screen. Better yet from this point on if I start a new dnd session with that tab, the size remains the invalid one from the previous session, and I can drag it out again making the effect even worse. I will continue debugging tomorrow. But do you have any idea what can go wrong? Or have the time maybe to look into it?

Here is a stack when the nsMenuPopupFrame first detects the resize:

* thread #1: tid = 0x777ff, 0x0000000105f0595f XUL`nsMenuPopupFrame::LayoutPopup(this=0x000000012434aea8, aState=0x00007fff5fbf7dc8, aParentMenu=0x0000000000000000, aAnchor=0x0000000000000000, aSizedToPopup=false) + 655 at nsMenuPopupFrame.cpp:471, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
  * frame #0: 0x0000000105f0595f XUL`nsMenuPopupFrame::LayoutPopup(this=0x000000012434aea8, aState=0x00007fff5fbf7dc8, aParentMenu=0x0000000000000000, aAnchor=0x0000000000000000, aSizedToPopup=false) + 655 at nsMenuPopupFrame.cpp:471
    frame #1: 0x0000000105f0f2b3 XUL`nsPopupSetFrame::DoLayout(this=0x0000000138740028, aState=0x00007fff5fbf7dc8) + 147 at nsPopupSetFrame.cpp:133
    frame #2: 0x0000000105ee9d26 XUL`nsIFrame::Layout(this=0x0000000138740028, aState=0x00007fff5fbf7dc8) + 166 at nsBox.cpp:509
    frame #3: 0x0000000105f23615 XUL`nsSprocketLayout::Layout(this=0x0000000126c85f00, aBox=0x0000000123ee94a8, aState=0x00007fff5fbf7dc8) + 3333 at nsSprocketLayout.cpp:484
    frame #4: 0x0000000105eec967 XUL`nsBoxFrame::DoLayout(this=0x0000000123ee94a8, aState=0x00007fff5fbf7dc8) + 167 at nsBoxFrame.cpp:911
    frame #5: 0x0000000105ee9d26 XUL`nsIFrame::Layout(this=0x0000000123ee94a8, aState=0x00007fff5fbf7dc8) + 166 at nsBox.cpp:509
    frame #6: 0x0000000105f27ad2 XUL`nsStackLayout::Layout(this=0x0000000126c85f20, aBox=0x0000000123ee9230, aState=0x00007fff5fbf7dc8) + 1298 at nsStackLayout.cpp:342
    frame #7: 0x0000000105eec967 XUL`nsBoxFrame::DoLayout(this=0x0000000123ee9230, aState=0x00007fff5fbf7dc8) + 167 at nsBoxFrame.cpp:911
    frame #8: 0x0000000105ee9d26 XUL`nsIFrame::Layout(this=0x0000000123ee9230, aState=0x00007fff5fbf7dc8) + 166 at nsBox.cpp:509
    frame #9: 0x0000000105eebf82 XUL`nsBoxFrame::Reflow(this=0x0000000123ee9230, aPresContext=0x0000000123e96800, aDesiredSize=0x00007fff5fbf82d8, aReflowState=0x00007fff5fbf81b0, aStatus=0x00007fff5fbf85bc) + 1394 at nsBoxFrame.cpp:707
    frame #10: 0x0000000105f12a66 XUL`nsRootBoxFrame::Reflow(this=0x0000000123ee9230, aPresContext=0x0000000123e96800, aDesiredSize=0x00007fff5fbf82d8, aReflowState=0x00007fff5fbf81b0, aStatus=0x00007fff5fbf85bc) + 86 at nsRootBoxFrame.cpp:174
    frame #11: 0x0000000105cf49f5 XUL`nsContainerFrame::ReflowChild(this=0x0000000123ee8ca0, aKidFrame=0x0000000123ee9230, aPresContext=0x0000000123e96800, aDesiredSize=0x00007fff5fbf82d8, aReflowState=0x00007fff5fbf81b0, aX=0, aY=0, aFlags=0, aStatus=0x00007fff5fbf85bc, aTracker=0x0000000000000000) + 277 at nsContainerFrame.cpp:1032
    frame #12: 0x0000000105e01b5a XUL`ViewportFrame::Reflow(this=0x0000000123ee8ca0, aPresContext=0x0000000123e96800, aDesiredSize=0x00007fff5fbf8568, aReflowState=0x00007fff5fbf8658, aStatus=0x00007fff5fbf85bc) + 762 at nsViewportFrame.cpp:309
    frame #13: 0x0000000105c4f15f XUL`PresShell::DoReflow(this=0x0000000123b07800, target=0x0000000123ee8ca0, aInterruptible=true) + 2687 at nsPresShell.cpp:8861
    frame #14: 0x0000000105c57cae XUL`PresShell::ProcessReflowCommands(this=0x0000000123b07800, aInterruptible=true) + 510 at nsPresShell.cpp:9034
    frame #15: 0x0000000105c5785f XUL`PresShell::FlushPendingNotifications(this=0x0000000123b07800, aFlush=(mFlushType = Flush_InterruptibleLayout, mFlushAnimations = false)) + 1903 at nsPresShell.cpp:4018
    frame #16: 0x0000000105c6869b XUL`PresShell::WillPaint(this=0x0000000123b07800) + 219 at nsPresShell.cpp:8401
    frame #17: 0x000000010569afec XUL`nsViewManager::CallWillPaintOnObservers(this=0x0000000123ecbfc0) + 364 at nsViewManager.cpp:1136
    frame #18: 0x0000000105699d2f XUL`nsViewManager::ProcessPendingUpdates(this=0x0000000123ecbfc0) + 111 at nsViewManager.cpp:1099
    frame #19: 0x0000000105699c34 XUL`nsViewManager::WillPaintWindow(this=0x0000000123ecbfc0, aWidget=0x000000011dfe7ab0) + 132 at nsViewManager.cpp:702
    frame #20: 0x0000000105695e29 XUL`nsView::WillPaintWindow(this=0x0000000120ff4c00, aWidget=0x000000011dfe7ab0) + 57 at nsView.cpp:1060
    frame #21: 0x0000000105709cc5 XUL`nsChildView::WillPaintWindow(this=0x00000001213f3800) + 149 at nsChildView.mm:1498
    frame #22: 0x0000000105717b67 XUL`-[ChildView viewWillDraw](self=0x000000012505cae0, _cmd="viewWillDraw") + 567 at nsChildView.mm:4031
    frame #23: 0x00007fff8d6f3f73 AppKit`-[NSView viewWillDraw] + 1138
    frame #24: 0x00007fff8d6f32b0 AppKit`-[NSView _sendViewWillDrawInRect:clipRootView:] + 1417
    frame #25: 0x00007fff8d6acf1a AppKit`-[NSView displayIfNeeded] + 1216
    frame #26: 0x00007fff8d85c287 AppKit`-[NSNextStepFrame displayIfNeeded] + 39
    frame #27: 0x00007fff8d6ea933 AppKit`-[NSWindow _setFrameCommon:display:stashSize:] + 3608
    frame #28: 0x00007fff8d857387 AppKit`-[NSWindow setFrameOrigin:] + 351
    frame #29: 0x0000000105777e34 XUL`nsCocoaWindow::Move(this=0x000000011dfe7ab0, aX=1446, aY=335) + 292 at nsCocoaWindow.mm:1206
    frame #30: 0x00000001056a1303 XUL`nsBaseWidget::MoveClient(this=0x000000011dfe7ab0, aX=1446, aY=335) + 163 at nsBaseWidget.cpp:1364
    frame #31: 0x0000000105693362 XUL`nsView::DoResetWidgetBounds(this=0x0000000120ff4c00, aMoveOnly=false, aInvalidateChangedSize=true) + 1106 at nsView.cpp:371
    frame #32: 0x0000000105692eae XUL`nsView::ResetWidgetBounds(this=0x0000000120ff4c00, aRecurse=false, aForceSync=true) + 110 at nsView.cpp:195
    frame #33: 0x0000000105698bdf XUL`nsViewManager::ProcessPendingUpdatesForView(this=0x0000000123ecbfc0, aView=0x0000000122fa4d80, aFlushDirtyRegion=false) + 431 at nsViewManager.cpp:384
    frame #34: 0x000000010569b07b XUL`nsViewManager::UpdateWidgetGeometry(this=0x0000000123ecbfc0) + 91 at nsViewManager.cpp:1115
    frame #35: 0x0000000105c5794b XUL`PresShell::FlushPendingNotifications(this=0x0000000123b07800, aFlush=(mFlushType = Flush_Layout, mFlushAnimations = true)) + 2139 at nsPresShell.cpp:4030
    frame #36: 0x0000000105c56313 XUL`PresShell::FlushPendingNotifications(this=0x0000000123b07800, aType=Flush_Layout) + 83 at nsPresShell.cpp:3865
    frame #37: 0x00000001033fa0cd XUL`nsDocument::FlushPendingNotifications(this=0x00000001215dc000, aType=Flush_Layout) + 989 at nsDocument.cpp:8145
    frame #38: 0x00000001031dd65c XUL`nsGlobalWindow::FlushPendingNotifications(this=0x0000000123a9f400, aType=Flush_Layout) + 108 at nsGlobalWindow.cpp:12442
    frame #39: 0x00000001031de890 XUL`nsGlobalWindow::GetInnerScreenRect(this=0x0000000123a9f400) + 256 at nsGlobalWindow.cpp:5093
    frame #40: 0x00000001031dea05 XUL`nsGlobalWindow::GetMozInnerScreenXOuter(this=0x0000000123a9f400) + 165 at nsGlobalWindow.cpp:5122
    frame #41: 0x00000001031deae3 XUL`nsGlobalWindow::GetMozInnerScreenX(this=0x0000000123aa0400, aError=0x00007fff5fbf9b90) + 179 at nsGlobalWindow.cpp:5129
    frame #42: 0x00000001040bf2b1 XUL`mozilla::dom::WindowBinding::get_mozInnerScreenX(cx=0x0000000123a9f800, obj=Handle<JSObject *> @ 0x00007fff5fbf9bc0, self=0x0000000123aa0400, args=JSJitGetterCallArgs @ 0x00007fff5fbf9bb8) + 49 at WindowBinding.cpp:4371
    frame #43: 0x00000001040ba57a XUL`mozilla::dom::WindowBinding::genericGetter(cx=0x0000000123a9f800, argc=0, vp=0x00007fff5fbf9d68) + 682 at WindowBinding.cpp:13225
    frame #44: 0x0000000108070f89 XUL`js::jit::DoCallNativeGetter(cx=0x0000000123a9f800, callee=JS::HandleFunction @ 0x00007fff5fbf9da0, obj=JS::HandleObject @ 0x00007fff5fbf9d98, result=JS::MutableHandleValue @ 0x00007fff5fbf9d90) + 345 at SharedIC.cpp:3668
    frame #45: 0x00000001007c8273

Comment 9

[Jonathan Kew [:jfkthame]]

For the Mac incarnation of this problem, did this regress recently (with the landing of bug 890156, 2016-01-14), or was it equally broken before that?

Comment 10

[Gabor Krizsanits (INACTIVE)]

(In reply to Jonathan Kew (:jfkthame) from comment #9)
> For the Mac incarnation of this problem, did this regress recently (with the
> landing of bug 890156, 2016-01-14), or was it equally broken before that?

It was broken before.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sticking in M8 and assigning to Gabor. Be careful what you wish for :-).

Comment 12

[Neil Deakin]

There are three problems that I have identified from my investigation:

1. nsMenuPopupFrame::SetPopupPosition shouldn't constrain the drag popup to the screen. This is easy to fix.

2. nsXULPopupManager::PopupMoved receives coordinates as arguments that it expects to be device pixels, but I think they may be supplied in display coordinates. A conversion using GetDesktopToDeviceScale seems to fix the size shift when changing displays.

3. The popup never appears if it is asked to be shown on a different screen than its parent window. This might be Mac specific, as I've only tested there.

Comment 13

[Neil Deakin]

Points 1 and 3 are easy to fix. The latter just needs the popup to be a top-level popup rather than a parent-level popup.

The issue with the panel resizing as it moves across screens isn't. The issue here is that the popup and its parent window appear on different screens with different scales, but since they share a single presshell, the scale is handled as if they are the same. One solution may be to just use a separate window isn't of a panel. I will investigate to see how feasible that is.

Comment 14

[Jonathan Kew [:jfkthame]]

:chutten, are you still seeing drag images badly misplaced (on Windows?) I can't seem to reproduce this with a Win8.1 laptop configured with a hidpi (devicePixelRatio=2) built-in screen and a lodpi (devicePixelRatio=1) external monitor; the drag preview stays nicely below-and-right from the mouse pointer as I drag between the displays.

It does change (physical) size as it crosses the boundary, because it stays the same size in device pixels, but that's a somewhat lesser problem than being wildly out of position, IMO.

I tried this with both Nightly (47) and Dev Ed (46); in both cases, the drag image seemed to follow the mouse OK. If you're still seeing it broken, please give details of your display setup so I can try to reproduce - thanks.

Comment 15

[Chris H-C :chutten]

Attachment: tearawayScalingPadding.png

On latest Nightly, the positioning problem no longer occurs. Yay!

Sadly, other problems creep up. Specifically, when I tear from hidpi to lodpi it pads the bottom of the screenshot window. If I take it slowly enough and go back and forward over the threshold enough, it will scale very very strangely so that it scales up to display on the lodpi screen. This can continue until the tearaway fills the entire lodpi screen.

If I tear from lodpi to hidpi, the padding problem doesn't happen (maybe because of the different aspect ratios in my displays) but the scaling problem is even worse.

I am running Windows 10 on a Lenovo ThinkPad X1 Carbon with its display at 2560x1440 (which I believe is native for that panel). Attached to it via a miniDP->DVI cable is an ancient xplio monitor running at its native resolution of 1280x1024.

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to Chris H-C :chutten from comment #15)
> Created attachment 8723709 [details]
> tearawayScalingPadding.png
> 
> On latest Nightly, the positioning problem no longer occurs. Yay!
> 
> Sadly, other problems creep up. Specifically, when I tear from hidpi to
> lodpi it pads the bottom of the screenshot window. If I take it slowly
> enough and go back and forward over the threshold enough, it will scale very
> very strangely so that it scales up to display on the lodpi screen. This can
> continue until the tearaway fills the entire lodpi screen.

What's the devicePixelRatio of each of your displays? (Easy way to check: load https://people.mozilla.org/~jkew/tests/devPixRatio.html, and see what it reports on each of your monitors.)

Comment 17

[Chris H-C :chutten]

2 for the laptop, 1 for the monitor.

Comment 18

[Jonathan Kew [:jfkthame]]

(In reply to Chris H-C :chutten from comment #15)
> Created attachment 8723709 [details]
> tearawayScalingPadding.png
> 
> On latest Nightly, the positioning problem no longer occurs. Yay!

I think we can safely assume this is fixed by the per-monitor DPI work in bug 890156 and followups, so it's fixed on 47 (nightly), but remains a problem on 46 (aurora) -- I can definitely reproduce such positioning issues there (contrary to what I said in comment 14; I was confused, or didn't test the right configuration, or something...).

> Sadly, other problems creep up. Specifically, when I tear from hidpi to
> lodpi it pads the bottom of the screenshot window. If I take it slowly
> enough and go back and forward over the threshold enough, it will scale very
> very strangely so that it scales up to display on the lodpi screen. This can
> continue until the tearaway fills the entire lodpi screen.
> 
> If I tear from lodpi to hidpi, the padding problem doesn't happen (maybe
> because of the different aspect ratios in my displays) but the scaling
> problem is even worse.

So far, I haven't been able to reproduce this scaling issue on my Win8.1 system. The tearaway remains at its original (device-pixel) size for the source monitor, which means that it looks smaller when dragged to a higher-dpi screen, or larger when dragged to a lower-dpi one, but I don't get the continual-scaling-up described here.

Do you have any add-ons or preference settings that might be relevant? (Can you reproduce the same behavior with a fresh profile on Nightly?)

Or maybe it's a Win10-related thing, to do with window frame dimensions or something like that. If others with Win8.1 and/or Win10 and mixed-dpi displays could try to reproduce, and report their success/failure, that might be helpful -- thanks.

Comment 19

[Chris H-C :chutten]

No addons, fresh profile, Nightly 47 :S

Tried with a second, much more modern, monitor at home. Still happens, making me think it might be a Windows 10 thing.

Comment 20

[Jim Mathies [:jimm]]

multimonitor tab thumbnail issues are P1, but don't block initial rollout.

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to Chris H-C :chutten from comment #19)
> No addons, fresh profile, Nightly 47 :S
> 
> Tried with a second, much more modern, monitor at home. Still happens,
> making me think it might be a Windows 10 thing.

I was able to reproduce this locally on a netbook after updating to Win10; still haven't reproduced on 8.1. Pretty sure it's Win10-specific. I'm intending to try and debug it, but no guarantees at this point...

Comment 22

[Jonathan Kew [:jfkthame]]

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

Comment 23

[Jonathan Kew [:jfkthame]]

Attachment: Don't try to change DPI on the fly for popup windows, they remain connected to their parent's presShell and therefore need to share its resolution

This should prevent the bad resizing. At some point, we might want to consider making the tearaway an actual toplevel window, which would allow it to rescale appropriately when crossing a monitor boundary; but for the time being, at least, this will fix the current screwy behavior.

Comment 24

[Jonathan Kew [:jfkthame]]

Chris, if you could confirm this fixes things on your system, that would be great (it looks OK in my testing, AFAICT). There's a try build in progress, see comment 22.

Comment 25

[Masatoshi Kimura [:emk]]

Comment on attachment 8726159 [details] [diff] [review]
Don't try to change DPI on the fly for popup windows, they remain connected to their parent's presShell and therefore need to share its resolution

r=me assuming this patch actually fixes the problem.

Comment 26

[Jonathan Kew [:jfkthame]]

BTW, the patch here will NOT have any effect on the OS X problem mentioned above (comment 5). I believe that's a separate issue, and should have its own bug.

Comment 27

[Jonathan Kew [:jfkthame]]

FTR, just filed bug 1253294 for the Mac problem.

Comment 28

[Chris H-C :chutten]

Works for me! ( ./mach mozregression --repo try --launch e6c7a80daf78a806d515693cc58dff3673895e25 if anyone else wants to try it out )

Something I noticed: have the thumbnails always been the top-left corner of the page instead of a scaled representative of what the window actually shows?

Comment 29

[Jonathan Kew [:jfkthame]]

Note that the shape of the thumbnail is fixed, it bears no relation to the current shape of your window. So it can't necessarily show the exact same content.

Comment 30

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa89bbe833b9baeb2ecac29fe3aeeb80563e042
Bug 1239353 - Don't try to change DPI on the fly for popup windows, they remain connected to their parent's presShell and therefore need to share its resolution. r=emk

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fa89bbe833b