1387340 - Popup contents render at DPI of primary monitor, not current monitor

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Chris Morgan]

Attachment: Screenshot of the bug

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.)

Comment 1

[Famikaj]

Attachment: uMatrix_DPI_webext_render.png

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

Comment 5

[ober.sebastian]

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();

Comment 9

[C. Martin]

Attachment: Several Screenshots in one image, showing the problem

Comment 11

[Famikaj]

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.

Comment 14

[Andy McKay]

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.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

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

Comment 16

[Kris Maglione [:kmag]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

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

Comment 18

[Kris Maglione [:kmag]]

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.

Comment 20

[Giorgio Maone [:ma1]]

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.

Comment 22

[Masatoshi Kimura [:emk]]

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

Comment 23

[dan.parker]

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

Comment 24

[Giorgio Maone [:ma1]]

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?

Comment 25

[Mark Striemer [:mstriemer]]

Attachment: 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.

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

Comment 26

[Andy McKay]

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

Comment 29

[Milan Sreckovic [:milan] (needinfo for best results)]

Markus may have some thoughts on this.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

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

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

Comment 33

[Kartikaya Gupta (email:[e-mail])]

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

Comment 34

[Masatoshi Kimura [:emk]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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?

Comment 35

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 36

[Andy McKay]

Attachment: dpi.png

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).

Comment 37

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1387340 - Ensure popup windows spawned on non-primary windows get a correct scale.

Review commit: https://reviewboard.mozilla.org/r/211376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211376/

Comment 38

[Kartikaya Gupta (email:[e-mail])]

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

Comment 39

[Jonathan Kew [:jfkthame]]

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.)

Comment 40

[Kartikaya Gupta (email:[e-mail])]

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.)

Comment 41

[Masatoshi Kimura [:emk]]

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

Comment 42

[Pulsebot]

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

Comment 43

[Andrei Ciure[:aciure]]

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

Comment 44

[Jonathan Kew [:jfkthame]]

[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?

Comment 45

[Jonathan Kew [:jfkthame]]

(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!

Comment 46

[Chris Morgan]

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!

Comment 47

[Kartikaya Gupta (email:[e-mail])]

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

Comment 48

[Andy McKay]

(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.

Comment 49

[Gerry Chang [:gchang]]

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

Comment 50

[Gerry Chang [:gchang]]

Track 58+ as broken UX for users with multiple displays

Comment 51

[Ciprian Georgiu, Desktop QA]

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.

Comment 52

[Julien Cristau [:jcristau]]

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.

Comment 53

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 54

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 56

[Ciprian Georgiu, Desktop QA]

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.

Comment 57

[C. Martin]

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

Comment 58

[Hossein Mortazavi]

Attachment: Screenshot (18)_LI.jpg

The bug is not fully resolved in Firefox 58.

Comment 59

[Kartikaya Gupta (email:[e-mail])]

That looks like bug 1354102.