818927 - [HiDPI] Clicking a tab may unexpectedly promote it to a new window

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Greg Edwards]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20121204 Firefox/19.0
Build ID: 20121204042015

Steps to reproduce:

Set your Windows DPI to something higher than 96.
Open multiple tabs in Firefox.
Click an inactive tab while the mouse is moving.
(Release right away. Do not hold down the mouse button for any length of time or it won't happen.)


Actual results:

The tab you click will be promoted to a new window, located much too far to the right edge of the screen. (On my 192DPI setup, it was all the way beyond the right edge and I had to select Move from the taskbar.)


Expected results:

The tab should have become active in the existing window.

It could be related to Bug 489729, but is specific to high DPI configurations. Aurora on OSX does not exhibit this bug, even in high DPI mode.

Comment 1

[Ioana (away)]

Can you reproduce this issue in safe mode or with a clean profile?
http://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode
http://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

I can't reproduce it on the 12/10 Nightly, on Windows 7 64bit.

Comment 2

[Greg Edwards]

I can replicate in Safe Mode.

When I create a blank profile, the issue cannot be replicated because layout.css.devPixelsPerPx defaults to 1.0 on Windows, disabling the high DPI feature and making everything microscopic. Setting it to -1.0 or 2.0 allows me to replicate the issue on an otherwise fresh profile.

To replicate, you need to swipe the mouse very quickly and release it before the thumbnail preview appears.

I am running Windows 7 x64 on Parallels Desktop 8 under OSX Mountain Lion 10.8.2. I will verify that the issue can be replicated under Boot Camp later this evening.

Comment 3

[Greg Edwards]

To further clarify, I can only replicate with devPixelsPerPx of 1.5 or greater. 2.0 makes it extremely easy to replicate. With 1.5, I can replicate with tabs close to the right side of the screen.

Comment 4

[Ed Avis]

I also see the behaviour of tabs unexpectedly jumping to a new window, running under Windows 7 with 200% font scaling and layout.css.devPixelsPerPx=-1.  I don't have thumbnail previews in Firefox (presumably because Aero is turned off) but I notice that when switching between tabs with several tabs open, one tab unexpectedly gets promoted to a new window.

Comment 5

[Greg Edwards]

I just discovered a bulletproof replication method that doesn't require fast reflexes:
1. Set layout.css.devPixelsPerPx to 2.0.
2. Drag the tab downward into the main view of the window.

The new window spawned will be situated too far down/right. The amount is proportional to how far from the top left the cursor is, by a factor of devPixelsPerPx.

Comment 6

[Greg Edwards]

Added as blocking bug 844604, as per Jonathan's instructions in win-hidpi. This is now officially a regression with 844604 landed.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: patch, convert drop position to logical coordinates for Windows

This appears to fix the problem - we just need to convert the drop position from device pixels (used within Gecko's drag-tracking) to logical coordinates for passing back to Windows.

Try build in progress at https://tbpl.mozilla.org/?tree=Try&rev=899eb5aac31b.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 731290 [details] [diff] [review]
patch, convert drop position to logical coordinates for Windows

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

::: widget/windows/nsDragService.cpp
@@ +307,3 @@
>    DWORD pos = ::GetMessagePos();
> +  FLOAT scaleFactor = 96.0f /
> +    GetDeviceCaps(gfxWindowsPlatform::GetPlatform()->GetScreenDC(), LOGPIXELSY);

You've used this a couple times, can we move this into WinUtils?

Comment 9

[Jonathan Kew [:jfkthame]]

Sure, could do.

Though actually, if it's OK with you, I think I'd favor putting it in gfxWindowsPlatform. That already has a reference to the screen device context on hand. WinUtils would either need to depend on gfxWindowsPlatform for that anyway, or it'd need to get (and release) the DC itself, which is extra work we don't need to do.

I'll factor it out and post an updated patch tomorrow.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - factor out a GetDPIScale method and use in place of computations based on GetDeviceCaps(dc, LOGPIXELSY)

This applies on top of the original patch here; it implements GetDPIScale in gfxWindowsPlatform, and uses this to replace all the cases where widget code gets LOGPIXELSY and computes a scale from this. (No change in behavior, just tidier code.)

Comment 11

[Greg Edwards]

Are you sure it's Windows logical pixels and not Firefox CSS pixels?

Comment 12

[Jonathan Kew [:jfkthame]]

When we're passing mouse positions to/from Windows, and managing the position and size of windows on the screen, yes - Windows logical pixels make sense.

(I suspect that we do have some places where we fail to make that distinction properly, and we may need to be more careful about it. Though in the default case they should be the same, I think.)

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: part 2, v2 - factor out a GetDPIScale method and use in place of computations based on GetDeviceCaps(dc, LOGPIXELSY)

This turned out to cause a couple of PNG-related reftests to fail with some pixels that end up a fractionally different color than expected:
  https://tbpl.mozilla.org/?tree=Try&rev=3733cbfa8448
The failures are consistent across all the Windows test platforms, and persist across multiple runs, but seem apparently inexplicable.

The differences are imperceptible to the eye, and not related to any actual change here; I propose to mark them as fuzzy-if(winWidget,...) here, as there's no reason for them to block this work; we can file a followup about investigating them further.

Note that the exact same test failures can be triggered by a patch that does *nothing* except add a call to gfxWindowsPlatform::GetPlatform() in nsWindow::GetDefaultScaleInternal, which by rights should have no visible effect whatsoever. See:
  https://tbpl.mozilla.org/?tree=Try&rev=068acae8c6c5
I think this shows that they do not indicate a problem in the patch here, but perhaps some kind of fragility in the testcases.

Comment 14

[Jonathan Kew [:jfkthame]]

FTR, I've filed bug 856452 about the PNG test failures.

Comment 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0517b64b90d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/82c8aaa1e909

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0517b64b90d2
https://hg.mozilla.org/mozilla-central/rev/82c8aaa1e909