943668 - window.screenX and .screenY return device pixels instead of CSS pixels

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

STR:
* Find a device where screenPixelsPerCSSPixel != 1.
* In a browser scratchpad, evaluate window.screenX

Actual:
* Value is in device pixels

Expected:
* Value is in CSS pixels.

The problem here is that the implementation in nsGlobalWindow is:

    return DevToCSSIntPixels(GetScreenXY(aError).x);

GetScreenXY() is forwarded to the outer window, but the inner window is what does the DevToCSSIntPixels.  The inner window has a null mDocShell, so a 1:1 mapping is assumed.

This problem does *not* occur for, eg, window.outerWidth as the entire call, including the DevToCSSIntPixels() call, it forwarded to the outer window, which does have a docShell, so the correct screenPixelsPerCSSPixel is used.

The fix doesn't look too hard, but CCing bz as he is likely to have insights I don't (I would have needinfo?, but I can't set that flag on a new bug, and anyway, bz is likely to be asked for review)

Comment 1

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #0)
> The fix doesn't look too hard, but CCing bz as he is likely to have insights
> I don't (I would have needinfo?, but I can't set that flag on a new bug, and
> anyway, bz is likely to be asked for review)

After all that, I forgot to CC bz.  So needinfo it is :)

Comment 2

[Boris Zbarsky [:bzbarsky]]

You can in fact needinfo on a new bug: you have to click the "Set bug flags" button, and needinfo is the second flag in the second column.

Fixing this sounds like the right thing to do to me, but worth double-checking with roc, since he's thought about this more.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-943668-ensure-window.screenX-and-.screenY-return.patch

As GetScreenXY seems to only be used internally, the simplest approach seems to be to forward the ScreenX and ScreenY methods instead of GetScreenXY.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8339773 [details] [diff] [review]
0001-Bug-943668-ensure-window.screenX-and-.screenY-return.patch

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

::: dom/base/nsGlobalWindow.cpp
@@ -4716,5 @@
>  
>  nsIntPoint
>  nsGlobalWindow::GetScreenXY(ErrorResult& aError)
>  {
> -  FORWARD_TO_OUTER_OR_THROW(GetScreenXY, (aError), aError, nsIntPoint(0, 0));

Please add a

MOZ_ASSERT(IsOuterWindow());

instead, and add a a comment in the header.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Ms2ger from comment #4)
> Comment on attachment 8339773 [details] [diff] [review]
> 0001-Bug-943668-ensure-window.screenX-and-.screenY-return.patch
> 
> Review of attachment 8339773 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ -4716,5 @@
> >  
> >  nsIntPoint
> >  nsGlobalWindow::GetScreenXY(ErrorResult& aError)
> >  {
> > -  FORWARD_TO_OUTER_OR_THROW(GetScreenXY, (aError), aError, nsIntPoint(0, 0));
> 
> Please add a
> 
> MOZ_ASSERT(IsOuterWindow());
> 
> instead, and add a a comment in the header.

I'm not sure what you mean here.  The initial call is made on the inner window, and we need to forward it to the outer window to ensure there is a docShell for the "dv pixels per css pixels" ratio.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

GetScreenXY is only called on the outer window, because of the forwarding. Assert that.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated to add assertion and header comment

(In reply to :Ms2ger from comment #6)
> GetScreenXY is only called on the outer window, because of the forwarding.
> Assert that.

Ah, gotchya, thanks.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Try at https://tbpl.mozilla.org/?tree=Try&rev=c7576f75d279

Comment 9

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d996ae7fc46

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d996ae7fc46

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8341531 [details] [diff] [review]
Updated to add assertion and header comment

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Windows machines with a non-default DPI setting will have the window position restored (by session restore) to the incorrect position.  See bug 942019 for more detail (but the fix for that bug is this patch)
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): Risk seems low, currently backing on m-c
String or IDL/UUID changes made by this patch: None

Note this bug seems to go back a long way - at least back to 25 - but at this stage I'm just requesting Aurora.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5b7edf143247

Comment 13

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0

Verified as fixed on Windows 8.1 x64 using latest Aurora 28.0a2 (buildID: 20131219004003) and 27 beta 2 (buildID: 20131216183647).

Comment 14

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/Window.screenX
https://developer.mozilla.org/en-US/docs/Web/API/Window.screenY
and
https://developer.mozilla.org/en-US/Firefox/Releases/28#Interfaces.2FAPIs.2FDOM