Closed Bug 1387340 Opened 3 years ago Closed 2 years ago

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

Categories

(WebExtensions :: Frontend, defect, P1)

57 Branch
defect

Tracking

(firefox-esr52 wontfix, firefox57 wontfix, firefox58+ verified, firefox59 verified)

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 + verified
firefox59 --- verified

People

(Reporter: me, Assigned: kats)

References

Details

Attachments

(8 files)

Attached image 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.)
Summary: Doorhanger contents rendering at DPI of primary monitor, not current monitor → Popup contents render at DPI of primary monitor, not current monitor
Priority: -- → P2
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
Duplicate of this bug: 1389819
Duplicate of this bug: 1397323
Duplicate of this bug: 1403233
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();
Duplicate of this bug: 1406082
Duplicate of this bug: 1410756
Duplicate of this bug: 1412768
Duplicate of this bug: 1416695
Priority: P2 → P1
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.
Duplicate of this bug: 1417274
Duplicate of this bug: 1417474
Flags: needinfo?(amckay)
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.
Flags: needinfo?(jfkthame)
Flags: needinfo?(bugmail)
Flags: needinfo?(amckay)
Do we have a regression range for this bug? Or has it always been this way?
Flags: needinfo?(bugmail)
(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.
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
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.
Duplicate of this bug: 1420157
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 1424776
PuppetScreenManager thinks that there is always only one screen in the system. Is it related?
Using Windows Scaling in Windows 10, and having different scaling % will replicate this as well.
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?
Attached image 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.
If this is happening to more than WebExtension popups, would someone on your team be more appropriate to take this bug on?
Flags: needinfo?(jfkthame) → needinfo?(milan)
See Also: → 1428475
Duplicate of this bug: 1428324
Duplicate of this bug: 1427967
Markus may have some thoughts on this.
Flags: needinfo?(milan) → needinfo?(mstange)
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.
Assignee: nobody → bugmail
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
Flags: needinfo?(mstange) → needinfo?(jfkthame)
Attached patch WIPSplinter Review
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
(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?
(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.
Attached image 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).
Flags: needinfo?(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.)
Attachment #8941105 - Flags: review?(jfkthame) → review+
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 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
Attachment #8941105 - Flags: review?(VYV03354) → review+
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
https://hg.mozilla.org/mozilla-central/rev/9b2d90369dd3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
[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?
Flags: needinfo?(bugmail)
(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!
Flags: needinfo?(mstriemer)
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 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
Flags: needinfo?(bugmail)
Attachment #8941105 - Flags: approval-mozilla-beta?
(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.
Flags: needinfo?(mstriemer)
Hi Florin,
can you help find some QA to verify this ASAP? I would like to include this in RC.
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Track 58+ as broken UX for users with multiple displays
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(florin.mezei)
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.
Attachment #8941105 - Flags: approval-mozilla-beta? → approval-mozilla-release+
https://hg.mozilla.org/releases/mozilla-beta/rev/15e1ac2b6b2d
Whiteboard: https://hg.mozilla.org/releases/mozilla-beta/rev/15e1ac2b6b2d0acfe7879b53b731e3fcc98013a4 (FIREFOX_58b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-beta/rev/15e1ac2b6b2d0acfe7879b53b731e3fcc98013a4 (FIREFOX_58b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/fd110250bcd776577d5e7694d322fda23b485ed3
Whiteboard: https://hg.mozilla.org/releases/mozilla-beta/rev/15e1ac2b6b2d0acfe7879b53b731e3fcc98013a4 (FIREFOX_58b_RELBRANCH)
Duplicate of this bug: 1396807
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.
Flags: qe-verify+
Thanks a lot guys for getting the fix into FF 58 despite the short timeframe!
Attached image Screenshot (18)_LI.jpg
The bug is not fully resolved in Firefox 58.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.