Closed
Bug 1387340
Opened 7 years ago
Closed 7 years ago
Popup contents render at DPI of primary monitor, not current monitor
Categories
(WebExtensions :: Frontend, defect, P1)
Tracking
(firefox-esr52 wontfix, firefox57 wontfix, firefox58+ verified, firefox59 verified)
VERIFIED
FIXED
mozilla59
People
(Reporter: me, Assigned: kats)
References
Details
Attachments
(8 files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/png
|
Details | |
(deleted),
text/x-review-board-request
|
emk
:
review+
jfkthame
:
review+
jcristau
:
approval-mozilla-release+
|
Details |
(deleted),
image/jpeg
|
Details |
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.)
Updated•7 years ago
|
Summary: Doorhanger contents rendering at DPI of primary monitor, not current monitor → Popup contents render at DPI of primary monitor, not current monitor
Updated•7 years ago
|
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
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();
Updated•7 years ago
|
Priority: P2 → P1
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.
Updated•7 years ago
|
Flags: needinfo?(amckay)
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.
Flags: needinfo?(jfkthame)
Flags: needinfo?(bugmail)
Flags: needinfo?(amckay)
Assignee | ||
Comment 15•7 years ago
|
||
Do we have a regression range for this bug? Or has it always been this way?
Flags: needinfo?(bugmail)
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.
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
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.
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.
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 22•7 years ago
|
||
PuppetScreenManager thinks that there is always only one screen in the system. Is it related?
Comment 23•7 years ago
|
||
Using Windows Scaling in Windows 10, and having different scaling % will replicate this as well.
Comment 24•7 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?
Comment 25•7 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.
Comment 26•7 years ago
|
||
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)
Updated•7 years ago
|
Markus may have some thoughts on this.
Flags: needinfo?(milan) → needinfo?(mstange)
Assignee | ||
Comment 30•7 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.
Assignee: nobody → bugmail
Assignee | ||
Comment 31•7 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
Flags: needinfo?(mstange) → needinfo?(jfkthame)
Assignee | ||
Comment 32•7 years ago
|
||
Assignee | ||
Comment 33•7 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
Comment 34•7 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?
Assignee | ||
Comment 35•7 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.
Comment 36•7 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).
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 38•7 years ago
|
||
Comment 39•7 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.)
Attachment #8941105 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 40•7 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.)
Comment 41•7 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
Attachment #8941105 -
Flags: review?(VYV03354) → review+
Comment 42•7 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
Comment 43•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 44•7 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?
Comment 45•7 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!
Flags: needinfo?(mstriemer)
Reporter | ||
Comment 46•7 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!
Assignee | ||
Comment 47•7 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
Flags: needinfo?(bugmail)
Attachment #8941105 -
Flags: approval-mozilla-beta?
Comment 48•7 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.
Flags: needinfo?(mstriemer)
Comment 49•7 years ago
|
||
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)
Comment 51•7 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.
Comment 52•7 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.
Attachment #8941105 -
Flags: approval-mozilla-beta? → approval-mozilla-release+
Comment 53•7 years ago
|
||
bugherder uplift |
Whiteboard: https://hg.mozilla.org/releases/mozilla-beta/rev/15e1ac2b6b2d0acfe7879b53b731e3fcc98013a4 (FIREFOX_58b_RELBRANCH)
Comment 54•7 years ago
|
||
uplift |
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)
Comment 56•7 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.
Flags: qe-verify+
Updated•7 years ago
|
status-firefox57:
--- → wontfix
status-firefox-esr52:
--- → wontfix
Comment 57•7 years ago
|
||
Thanks a lot guys for getting the fix into FF 58 despite the short timeframe!
Comment 58•7 years ago
|
||
The bug is not fully resolved in Firefox 58.
Updated•7 years ago
|
Assignee | ||
Comment 59•7 years ago
|
||
That looks like bug 1354102.
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•