Closed Bug 1338776 Opened 5 years ago Closed 5 years ago

[e10s] can't move window to very right side of screen with window.moveTo

Categories

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

51 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox53 --- verified
firefox54 --- verified
firefox55 --- verified

People

(Reporter: stuff, Assigned: emk)

References

Details

Attachments

(3 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

When window.devicePixelRatio > 1.0 (e.g. by setting layout.css.devPixelsPerPx > 1.0 if -1.0 doesn't have this effect), moveTo does not move to the correct location.

Open the JavaScript console (or write a test file if you prefer) and run the following:

var mywin = window.open("about:blank", "MyWindow", "left="+(window.screen.availWidth - 300)+",top=0,width=300");

Notice that the window opens to the very right of your screen.
Then run this code:

mywin.moveTo(mywin.screenX,0);


Actual results:

The window opens to the very right of the screen.
After the moveTo call it moves to the left, since moveTo does not seem to respect the pixel ratio and uses native pixels. For a 1920x1080 screen an window.devicePixelRatio at 1.25 the horizontal resolution is 1536 pixels. moveTo will not handle any calls that have an x value larger than 1536 in that case but does not move the window according to the [0,1536] range but according to the [0,1920] range, resulting in an incorrect position.

I've tested on Windows with Firefox (51.0.1 x32) and on a different machine running Ubuntu linux. Same result on both machines.


Expected results:

The window opens to the right of the screen.
After the moveTo() call the position shouldn't have changed.
Component: Untriaged → Layout
Product: Firefox → Core
There are several issues here:

The moveTo moves to the left because screenX is 0 if directly called after window.open. Filed that as bug 1338831: some attributes of opened windows not directly available wrong if directly fetched after using window.open

After putting that call in an event listener:

mywin.addEventListener("DOMContentLoaded",
                       function() {
                         var moveToX = mywin.screenX;
                         alert(moveToX);
                         mywin.moveTo(moveToX,0);
                       });

There is still a shift to the left in Firefox 52 and newer. Filed:
Bug 1338829 can't move window to very right side of screen with window.moveTo if e10s (multiprocess) is active

Let's use this bug for the fact the script in comment 0 doesn't open the window at the top right if the device pixel ratio is not 1.

Fix:
var dpr = window.devicePixelRatio;
var mywin = window.open("about:blank", "MyWindow", "left="+(window.screen.availWidth - 300 / dpr)+",top=0,width=300");
Status: UNCONFIRMED → NEW
Component: Layout → DOM
Ever confirmed: true
Summary: High DPI causes erroneous behaviour in core JavaScript functions → need to take device pixel ratio into account to put window on right side of screen
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #1)
> There are several issues here:
> 
> The moveTo moves to the left because screenX is 0 if directly called after
> window.open. Filed that as bug 1338831: some attributes of opened windows
> not directly available wrong if directly fetched after using window.open ...
While this is true, this is not what I was trying to point out. BTW, when you call window.resizeTo() the new size (window.outerWidth, window.innerWidth, -Height) is also not immediately available after the call to resizeTo() returns. I don't know if this is the intended behaviour. I expected the new size to be available after the call completes.

> Let's use this bug for the fact the script in comment 0 doesn't open the
> window at the top right if the device pixel ratio is not 1.
That's actually no true in my case. The window _does_ open at the very top right to the screen. But if you try to move it via moveTo() to anywhere beyond ACTUAL_RESOLUTION/window.devicePixelRatio (for 1920 this is 1536 at dPR 1.25), this will not work. So you can't get it back to the very right portion of the screen.

> Fix:
> var dpr = window.devicePixelRatio;
> var mywin = window.open("about:blank", "MyWindow",
> "left="+(window.screen.availWidth - 300 / dpr)+",top=0,width=300");
This does not fix the bug I reported, since as I said the window opens perfectly fine for me at the far right of the screen, however it is impossible to move it there again via moveTo. Scaling via dpr does not fix this (I'm fully aware that you are only scaling the offset because window.screen.availWidth is already scaled by FF). It actually makes things worse. If I leave the window size at 300 and open it at (window.screen.availWidth - 350/dpr), then it is still at the very right of the screen. Opening it at (window.screen.availWidth - 350) opens it at the correct position. 

Try this:

var dpr = window.devicePixelRatio;
var mywin = window.open("about:blank", "MyWindow", "left="+(window.screen.availWidth - 350 / dpr)+",top=0,width=300");
mywin.addEventListener("DOMContentLoaded",
                       function() {
                         console.log("With dpr scaling: " + (window.screen.availWidth - mywin.screenX));
                       });
var otherwin = window.open("about:blank", "OtherWindow", "left="+(window.screen.availWidth - 350)+",top=0,width=300");
otherwin.addEventListener("DOMContentLoaded",
                       function() {
                         console.log("Without dpr scaling: " + (window.screen.availWidth - otherwin.screenX));
                       });



For me this results in the following:
With dpr scaling: 314 
Without dpr scaling: 350
Feel free to use this test code:

var dpr = window.devicePixelRatio;
var settings = [];
settings[settings.length] = {dprScale: true, offset: 350};
settings[settings.length] = {dprScale: false, offset: 350};
settings[settings.length] = {dprScale: true, offset: 300};
settings[settings.length] = {dprScale: false, offset: 300};

for(var i = 0; i < settings.length; i++) {
	var mywin = window.open("about:blank", "_blank", "left="+(window.screen.availWidth - settings[i].offset / (settings[i].dprScale ? dpr : 1.0))+",top=0,width=300");
	(function(dprScale, offset)mywin.addEventListener("DOMContentLoaded",
                       function() {
                         this.document.write("<h4>window.open offset: "+(-offset)+(dprScale ? " / dpr" : "" )+"</h4>");
                         this.document.write("<h4>devicePixelRatio (dpr): "+dpr+"</h4>");
						 this.document.write("<h4>availWidth - screenX:"+(this.window.screen.availWidth - this.window.screenX)+"</h4>");
                         this.document.write("<h4>outerWidth: "+this.window.outerWidth+"</h4>");
                         this.document.write("<h4>innerWidth: "+this.window.innerWidth+"</h4>");
                       })
	)(settings[i].dprScale, settings[i].offset);
}
Something went wrong in comment #4, here is the correct test code:


var dpr = window.devicePixelRatio;
var settings = [];
settings[settings.length] = {dprScale: true, offset: 350};
settings[settings.length] = {dprScale: false, offset: 350};
settings[settings.length] = {dprScale: true, offset: 300};
settings[settings.length] = {dprScale: false, offset: 300};

for(var i = 0; i < settings.length; i++) {
	var mywin = window.open("about:blank", "_blank", "left="+(window.screen.availWidth - settings[i].offset / (settings[i].dprScale ? dpr : 1.0))+",top=0,width=300");
	(function(dprScale, offset){
		mywin.addEventListener("DOMContentLoaded",
			function() {
				this.document.write("<h4>window.open offset: "+(-offset)+(dprScale ? " / dpr" : "" )+"</h4>");
				this.document.write("<h4>devicePixelRatio (dpr): "+dpr+"</h4>");
				this.document.write("<h4>availWidth - screenX: "+(this.window.screen.availWidth - this.window.screenX)+"</h4>");
				this.document.write("<h4>outerWidth: "+this.window.outerWidth+"</h4>");
				this.document.write("<h4>innerWidth: "+this.window.innerWidth+"</h4>");
			}
		);
	})(settings[i].dprScale, settings[i].offset);
}
According to my local testing, the patch 2.3 from bug 1288760 fixed this issue.
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #3)
> Created attachment 8836486 [details]
> screemshpt window open positions

(In reply to stuff from comment #4)
> Created attachment 8836492 [details]
> Opening positions on my machine

Maybe this is a difference between Windows 8.1 and Windows 10?
This patch will add sync IPC, but I dare to adopt this patch because I consider uplifting.
Comment on attachment 8838012 [details]
Bug 1338776 - Support GetContentsScaleFactor and GetDefaultCSSScaleFactor in ScreenProxy.

https://reviewboard.mozilla.org/r/113014/#review114566

Yes, this is clearly the correct thing to do, and should be safe enough, I think.
Attachment #8838012 - Flags: review?(jfkthame) → review+
Priority: -- → P3
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/c1d6a94e258e
Support GetContentsScaleFactor and GetDefaultCSSScaleFactor in ScreenProxy. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/c1d6a94e258e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Sorry, I mixed up this bug and bug 1338829. I'll swap the bug purpose after the fact because:
* The change is already merged with the bug number.
* The current subject of the bug is not reproducible to the original reporter (and me).
Summary: need to take device pixel ratio into account to put window on right side of screen → [e10s] can't move window to very right side of screen with window.moveTo
Transferring tracking flags from bug 1338829.
Assignee: nobody → VYV03354
See Also: → 1340775
Not sure if this is worth backporting to Beta at this point (the RC build for 52 is happening on Monday), but this seems at least worth an Aurora uplift. Feel free to request Beta approval too if you want to argue for 52 inclusion.
Flags: needinfo?(VYV03354)
I stopped Beta approval due to a regression (bug 1340775). I will request Aurora (53) approval once bug 1340775 is fixed.
Flags: needinfo?(VYV03354)
Comment on attachment 8838012 [details]
Bug 1338776 - Support GetContentsScaleFactor and GetDefaultCSSScaleFactor in ScreenProxy.

Approval Request Comment
[Feature/Bug causing the regression]: The initial PScreenManager implementation.
[User impact if declined]: Sometimes popup windows will open at unexpected positions on High DPI monitors.
[Is this code covered by automated tests?]: No, our test infrastructure is very poor about High DPI.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:
1. This test requires a High DPI monitor. Run Firefox under the High DPI configuration.
2. Make sure e10s is enabled.
3. Run the following script from Web Console.
   var mywin = window.open("about:blank", "MyWindow", "left="+(window.screen.availWidth - 300)+",top=0,width=300");
4. Wait until a popup window will open. If popup blocker prevented the popup from opening the window, whitelist the current domain and retry from the step 3.
5. Run the following script from Web Console.
   mywin.moveTo(mywin.screenX,0);
Expected result: The popup window will not move.
[List of other uplifts needed for the feature/fix]: a patch from bug 1338776.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is a correctness fix and the implementation pattern follows the existing other properties on nsIScreen.
[String changes made/needed]: No.
Attachment #8838012 - Flags: approval-mozilla-beta?
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
I tested this issue on Windows 10 x32 (Monitor Dell P2415Q) with FF Nightly 55.0a1(2017-03-16) and I can confirm the fix. I verified this issue following the steps from comment 17 and I manage to obtain the expected result.
Flags: needinfo?(brindusa.tot)
Also tested on Windows 10 x32 (Monitor Dell P2415Q) with FF Aurora 54.0a2(2017-03-16) and I can confirm the fix.
Status: RESOLVED → VERIFIED
Comment on attachment 8838012 [details]
Bug 1338776 - Support GetContentsScaleFactor and GetDefaultCSSScaleFactor in ScreenProxy.

Fix for high DPI monitors and popup placement, verified in nightly, let's take this for beta 53.
Attachment #8838012 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Let's make sure this works as intended on 53 as well.
Flags: qe-verify+
Reproduced the initial issue using Firefox 51.0 RC on Ubuntu 14.04 64bit using a Dell 4k monitor, verified that the issue does not reproduce anymore using Firefox 53 beta 5 on Ubuntu 14.04 64bit, Windows 8.1 64bit (both with Dell 4k monitor) and macOS 10.12.3 (MacBook Pro late 2015 with retina display).
Flags: qe-verify+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.