Chris Morgan Reporter
	Description • 7 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170803134456

Steps to reproduce:

1. Use a mixed-DPI environment. For me, my primary display is 2× and two external displays are 1×.
2. Install a WebExtensions extension which opens a panel when you click on it. Examples: Container Tabs, Stylus and uBlock. Sorry, not sure of the precise terminology.
3. Click on the button so that it opens the panel


Actual results:

The doorhanger (or whatever you call it) is sized for the current display, but its contents are scaled for the primary monitor’s DPI.

Thus, on the 2× display all is fine, but on a 1× display you only get the top quarter of the contents, rendered at double size.


Expected results:

The current monitor’s DPI should be used.

Note that other doorhangers like bookmarks, downloads and the ≡ menu are fine, as was uBlock when it was a legacy extension—it seems to be only WebExtensions that are affected. (This has been the case for weeks, I just haven’t filed it until now.)

	Famikaj
	Comment 1 • 7 years ago

Issue seems to be similar to bug 1389819, where the contents of the webext will render based on the primary monitor's DPI. Issue is also confirmed to affect uMatrix, hence have reason to believe it affects functionaility of all webext. Does not affect Legacy addons.

Tested with: Firefox 56.0b2

	ober.sebastian
	Comment 5 • 7 years ago

The following is a workaround, but it is not perfect. It does not work if the popup is opened from a browser-internal tab for the first time. Then, it uses the last available scaling, which may or may not be correct.
The popup is scaled using CSS transform rules, the correct devicePixelRatio is found by calling it from a content script. The extension needs the activeTab permission in order to execute the script.

async function fixDPI() {
    if (window.navigator.userAgent.indexOf("Gecko/") === -1) {
        // Don't adjust for Chrome, as it has a different problem entirely
        return;
    }
    let dppx = 1;
    try {
        const res = await browser.tabs.executeScript(undefined, {code: "window.devicePixelRatio;"});
        dppx = Number(res[0]);
        browser.storage.local.set({popupdppx : dppx});
    } catch (error) {
        // This happens if we do not have permission to run execute script
        // For example on browser-internal pages such as about:addons or about:newtab
        const res = await browser.storage.local.get("popupdppx");
        if (res.hasOwnProperty("popupdppx")) {
            dppx = res.popupdppx;
        }
    }
    if (dppx !== window.devicePixelRatio) {
        document.body.style.transform = `scale(${dppx / window.devicePixelRatio})`;
        document.body.style.transformOrigin = "top left";
    }
}
fixDPI();

	Famikaj
	Comment 11 • 7 years ago

Workaround: use mouse wheel and Ctrl key to zoom out/in the rendered webext interface when using the browser on a different monitor.

Workaround tested on 58.0b1.

	Andy McKay
	Comment 14 • 7 years ago

Kats or Jonathan, would you be able to help us with this bug? You were suggested because you'd recently touched TabChild::RecvUIResolutionChanged. There are more people out there with multiple DPI setups than we thought and this rising towards the top of things that we'd like to fix.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 15 • 7 years ago

Do we have a regression range for this bug? Or has it always been this way?

	Kris Maglione [:kmag]
	Comment 16 • 7 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> Do we have a regression range for this bug? Or has it always been this way?

I believe it's always been this way.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 17 • 7 years ago

I don't have a setup currently that lets me reproduce this bug, so I just did a quick poke through of the code. I'm assuming that the problem occurs for out-of-process webextensions, which are hosted inside a PuppetWidget in a content process. From the looks of it, the content process ends up with a widget scale that corresponds to the primary display rather than the display it's actually on. However, when I trace through the code, it looks like PuppetWidget::mDPI and PuppetWidget::mDefaultScale are populated from the widget on the parent side, around [1]. (This trickles down to the child process via Send/RecvUIResolutionChanged). So the question is, what widget is being used on the parent side? It's coming from TabParent::GetWidget() which gets it via the TabParent's mFrameElement, and I have no idea what that's going to be in the case of OOP webextensions. Perhaps that element is attached to the widget on the primary display somehow, instead of the widget on the display that's actually showing the panel?

[1] https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/dom/ipc/TabParent.cpp#2744

	Kris Maglione [:kmag]
	Comment 18 • 7 years ago

The <browser> element is a child of a <popup>, which has its own widget. If I had to guess, I'd say we wind up doing this calculation before the popup is shown, which means we can't calculate its screen position accurately.

I'd expect it to be updated when the popup is shown/resized, though.

	Giorgio Maone [:ma1]
	Comment 20 • 7 years ago

Stating the obvious, but NoScript, ABP+, Stylish, Foxyproxy, Video Download Helper, i.e. most if not all the WebExtensions featuring a browserAction popup are badly affected.

	Masatoshi Kimura [:emk]
	Comment 22 • 7 years ago

PuppetScreenManager thinks that there is always only one screen in the system. Is it related?

	dan.parker
	Comment 23 • 6 years ago

Using Windows Scaling in Windows 10, and having different scaling % will replicate this as well.

	Giorgio Maone [:ma1]
	Comment 24 • 6 years ago

I'm receiving several complaints from many users, and the suggested work-around is unreliable leading to severe side effects for people using custom zoom level in content (e.g. because visually impaired).
Any progress?

	Mark Striemer [:mstriemer]
	Comment 25 • 6 years ago

There is a similar issue with the download dialog. I can make it very small with my laptop as primary monitor (this is real bad, as attached) or very large with the external display as the primary monitor (I'm unsure of the DPI of these monitors, but it works both ways). This is when you have Firefox on the secondary monitor and the dialog appears on the primary.

I'm not sure if this is related but figured I'd mention it.

	Andy McKay
	Comment 26 • 6 years ago

If this is happening to more than WebExtension popups, would someone on your team be more appropriate to take this bug on?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 29 • 6 years ago

Markus may have some thoughts on this.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 30 • 6 years ago

I'm able to reproduce this problem now (at least as described in comment 0) by hooking up an external monitor to my surface pro. I'll try to take a look at it this week. If somebody else has cycles to investigate please feel free to steal.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 31 • 6 years ago

I investigated what's going on and have some results. What seems to happen is that when the nsWindow is first created, it has a null mWnd pointer. This means that if GetDefaultScaleInternal() is called on the nsWindow, it calls [1] with that null HWND and that always returns the scale on the primary monitor (because of the MONITOR_DEFAULTTOPRIMARY argument passed to MonitorFromWindow). That's one part of the problem.

This behaviour occurs even on the top-level nsWindow object during startup, because GetDefaultScaleInternal() is called before the mWnd is populated. However, in the case of the top-level nsWindow object, once the mWnd is populated we get a WM_DPICHANGED message, which calls OnDPIChanged [2] which resets mDefaultScale back to -1.0, so the next time we try to get the default scale we recompute it using the correct monitor. This effectively works around the problem for the root window (and arguably is the only way it could work at all).

For popup windows, however, when OnDPIChanged is called, they bounce right out because of the check at [3], and so they effectively get stuck with the primary monitor's scale forever. It's not really clear to me what the expected behaviour is in this scenario, :jfkthame - since you worked on this stuff most recently, and in particular added the check at [3], do you have any suggestions?

I was able to fix at least one set of STR simply by resetting mDefaultScale = -1.0 right after mWnd is populated at [4], but I'm not sure if that negates the effects of the check at [3] or not. I haven't yet tried different variations of the STR so this fix is likely not comprehensive.

For future reference, the STR I was using was to install the container tabs add-on into firefox, and move firefox over into a secondary monitor which has a 1.0 scale, while my primary monitor has a 1.5 scale. Moving firefox onto the secondary monitor is not required on subsequent restarts since the window will already be there. But in any case, opening the container tabs panel exhibited the 1.5 scaling issue.

[1] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/widget/windows/WinUtils.h#216
[2] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/widget/windows/nsWindow.cpp#7378
[3] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/widget/windows/nsWindow.cpp#7372
[4] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/widget/windows/nsWindow.cpp#833

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 33 • 6 years ago

This WIP actually seems to fix all the STRs that I've been able to reproduce. I kicked off a win64 try build with it at [1]. Once it's done, it would be useful if people who were seeing this problem test this build to see if they still run into the problem.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=867f44a3e3f776de42103651e8a0a1d74e19f2f6

	Masatoshi Kimura [:emk]
	Comment 34 • 6 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #33)
> This WIP actually seems to fix all the STRs that I've been able to
> reproduce. I kicked off a win64 try build with it at [1]. Once it's done, it
> would be useful if people who were seeing this problem test this build to
> see if they still run into the problem.
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=867f44a3e3f776de42103651e8a0a1d74e19f2f6

The check at [3] was added for bug 1239353. Does this patch regress bug 1239353?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 35 • 6 years ago

(In reply to Masatoshi Kimura [:emk] from comment #34)
> The check at [3] was added for bug 1239353. Does this patch regress bug
> 1239353?

I see the same behaviour in vanilla m-c as I do if I revert the fix for bug 1239353. In both cases, when I drag a tab from a lodpi monitor to a hidpi monitor, the tab preview disappears on the hidpi monitor. Dragging from hidpi to lodpi works fine. The same behaviour also happens with my patch, so I can't really tell if it's regressing that bug or not. It might be that the bug manifests differently for me because of how my monitors are arranged (secondary lodpi monitor is to the left of the primary hidpi monitor).

If you were able to reproduce that bug, you can try on the build from comment 33 to see if it got regressed.

	Andy McKay
	Comment 36 • 6 years ago

This fixes uBlock Origin in some quick testing on Windows 10 for me. Attached uBlock Origin with the nightly (bottom) and the build from comment 33 (top).

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 38 • 6 years ago

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

	Jonathan Kew [:jfkthame]
	Comment 39 • 6 years ago
mozreview-review

Comment on attachment 8941105 [details]
Bug 1387340 - Ensure popup windows spawned on non-primary windows get a correct scale.

https://reviewboard.mozilla.org/r/211376/#review217546

Makes sense to me. (FWIW, I can't seem to reproduce the issue described in bug 1239353 any more, even if I revert the change to OnDPIChanged from that bug, so it's possible other changes (in either Gecko or Windows) have made it unnecessary, though I'm not at all confident of that. But in any case, this patch looks reasonable, given the explanation here.)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 40 • 6 years ago

Thanks. I'm going to wait for :emk to also review before landing, since I'd feel better with two reviewers signing off on this. (:emk, feel free to redirect it somebody else if you think they would be a better reviewer.)

	Masatoshi Kimura [:emk]
	Comment 41 • 6 years ago
mozreview-review

Comment on attachment 8941105 [details]
Bug 1387340 - Ensure popup windows spawned on non-primary windows get a correct scale.

https://reviewboard.mozilla.org/r/211376/#review217670

	Pulsebot
	Comment 42 • 6 years ago

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b2d90369dd3
Ensure popup windows spawned on non-primary windows get a correct scale. r=emk,jfkthame

	Andrei Ciure[:aciure]
	Comment 43 • 6 years ago
bugherder

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

	Jonathan Kew [:jfkthame]
	Comment 44 • 6 years ago

[Tracking Requested - why for this release]:
This causes pretty badly broken UX for users with multiple displays / mixed DPI; we shouldn't allow another release to ship with this problem.

:kats, are you intending to request beta uplift here?

	Jonathan Kew [:jfkthame]
	Comment 45 • 6 years ago

(In reply to Mark Striemer [:mstriemer] from comment #25)
> Created attachment 8940302 [details]
> download-dialog.png
> 
> There is a similar issue with the download dialog. I can make it very small
> with my laptop as primary monitor (this is real bad, as attached) or very
> large with the external display as the primary monitor (I'm unsure of the
> DPI of these monitors, but it works both ways). This is when you have
> Firefox on the secondary monitor and the dialog appears on the primary.

Your screenshot looks like it's macOS, right? The fix here is in Windows widget code, so it won't have any effect there; please file a separate bug for your issue, so it doesn't get forgotten here. Thanks!

	Chris Morgan Reporter
	Comment 46 • 6 years ago

The download dialog scaling issue (also seen in the Help → About Nightly… dialog) has, to the best of my knowledge, always been there, and affects Windows as well (though on Windows the window is resized rather than truncating as in that macOS screenshot). For some reason it’s never really occurred to me to file a bug about that, because it’s always been like that, and didn’t really do me any *harm*. I filed this one because it was a *regression* that was occurring in the WebExtensions migration, and that truncating made it rather difficult to use!

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 47 • 6 years ago

Comment on attachment 8941105 [details]
Bug 1387340 - Ensure popup windows spawned on non-primary windows get a correct scale.

Approval Request Comment
[Feature/Bug causing the regression]: unknown
[User impact if declined]: some kinds of popup windows (notably web extension panels) show up using the primary monitor's scaling, even when they are opened on a secondary monitor with different scaling. Based on the number of duplicate bugs filed about this issue, it seems like a lot of people are running into it
[Is this code covered by automated tests?]: nope. in general we don't have much testing for different dpi configurations, much less multi-monitor setups
[Has the fix been verified in Nightly?]: yes, at least for the primary STR on this bug.
[Needs manual test from QE? If yes, steps to reproduce]: yes, please verify that all the bugs duped to this one are also fixed in the latest nightly. If any of them are not fixed, those bugs should be reopened.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: i don't think so
[Why is the change risky/not risky?]: it's a one-line fix that's easy to back out if needed. the nature of the change is just clearing a cached value so that it gets recomputed correctly, which should be a benign change in pretty much all cases. there is a slight risk of it regressing bug 1239353 but based on our testing it doesn't seem to. Even if it does, I think it would be better to take this fix and regress that bug considering this one is higher visibility.
[String changes made/needed]: none

	Andy McKay
	Comment 48 • 6 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #45)
> Your screenshot looks like it's macOS, right? The fix here is in Windows
> widget code, so it won't have any effect there; please file a separate bug
> for your issue, so it doesn't get forgotten here. Thanks!

Thats bug 1428475.

	Gerry Chang [:gchang]
	Comment 49 • 6 years ago

Hi Florin,
can you help find some QA to verify this ASAP? I would like to include this in RC.

	Gerry Chang [:gchang]
	Comment 50 • 6 years ago

Track 58+ as broken UX for users with multiple displays

	Ciprian Georgiu, Desktop QA
	Comment 51 • 6 years ago

I've reproduced this issue using the STR from comment 0, on an affected Nightly build 57.0a1 (2017-08-04).

I can confirm that it's no longer repro on latest Nightly 59.0a1 (2018-01-11) running Windows 10 x64/x86. Tested with the following WebExtensions:  uBlock Origin, Stylus, LastPass: Free Password Manager and Firefox Multi-Account Containers. I will also verify this bug on FF 58, leaving the qe+ in place until then.

	Julien Cristau [:jcristau]
	Comment 52 • 6 years ago

Comment on attachment 8941105 [details]
Bug 1387340 - Ensure popup windows spawned on non-primary windows get a correct scale.

fix an issue with mixed dpi on windows, for 58 rc1.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 53 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/15e1ac2b6b2d

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 54 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/15e1ac2b6b2d0acfe7879b53b731e3fcc98013a4 (FIREFOX_58b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/fd110250bcd776577d5e7694d322fda23b485ed3

	Ciprian Georgiu, Desktop QA
	Comment 56 • 6 years ago

Verified fixed on 58.0 (20180115093319) as well, running Windows 10 x64/x86. I've tested this bug using the same WebExtensions mentioned in comment 51, on a mixed dpi system.

	C. Martin
	Comment 57 • 6 years ago

Thanks a lot guys for getting the fix into FF 58 despite the short timeframe!

	Hossein Mortazavi
	Comment 58 • 6 years ago

The bug is not fully resolved in Firefox 58.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 59 • 6 years ago

That looks like bug 1354102.