Closed Bug 943668 Opened 11 years ago Closed 11 years ago

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

Categories

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

x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 - verified
firefox28 --- verified

People

(Reporter: markh, Assigned: markh)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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)
(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 :)
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky) → needinfo?(roc)
As GetScreenXY seems to only be used internally, the simplest approach seems to be to forward the ScreenX and ScreenY methods instead of GetScreenXY.
Attachment #8339773 - Flags: feedback?(roc)
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.
(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.
GetScreenXY is only called on the outer window, because of the forwarding. Assert that.
(In reply to :Ms2ger from comment #6)
> GetScreenXY is only called on the outer window, because of the forwarding.
> Assert that.

Ah, gotchya, thanks.
Assignee: nobody → mhammond
Attachment #8339773 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8341531 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/9d996ae7fc46
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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.
Attachment #8341531 - Flags: approval-mozilla-aurora?
Attachment #8341531 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
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).
Status: RESOLVED → VERIFIED
Keywords: verifyme
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: