Support per-monitor DPI on Windows 8.1 & 10 desktop

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: emk, Assigned: jfkthame)

Tracking

(Depends on 5 bugs, Blocks 1 bug, {feature})

unspecified
mozilla47
All
Windows 8
Points:
---

Firefox Tracking Flags

(firefox46 wontfix, firefox47+ fixed)

Details

()

Attachments

(22 attachments, 44 obsolete attachments)

2.43 KB, text/plain
Details
42.62 KB, patch
Details | Diff | Splinter Review
15.26 KB, patch
Details | Diff | Splinter Review
26.51 KB, patch
Details | Diff | Splinter Review
12.35 KB, image/png
Details
21.33 KB, image/png
Details
20.69 KB, patch
Details | Diff | Splinter Review
4.14 KB, image/png
Details
3.14 KB, patch
Details | Diff | Splinter Review
6.04 KB, patch
Details | Diff | Splinter Review
18.51 KB, patch
Details | Diff | Splinter Review
13.60 KB, patch
Details | Diff | Splinter Review
7.23 KB, patch
Details | Diff | Splinter Review
23.88 KB, patch
Details | Diff | Splinter Review
32.01 KB, patch
Details | Diff | Splinter Review
17.78 KB, patch
Details | Diff | Splinter Review
39.80 KB, patch
Details | Diff | Splinter Review
1.20 KB, patch
Details | Diff | Splinter Review
2.40 KB, patch
Details | Diff | Splinter Review
9.71 KB, patch
kats
: review+
Details | Diff | Splinter Review
2.01 KB, patch
emk
: review+
Details | Diff | Splinter Review
19.39 KB, patch
mstange
: review+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
No description provided.
Reporter

Comment 1

6 years ago
Posted patch patch (obsolete) — Splinter Review
I moved GetDPIScale from gfxWindowsPlatform because all callers are under widget/windows.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #771752 - Flags: review?(jfkthame)
Assignee

Comment 3

6 years ago
Comment on attachment 771752 [details] [diff] [review]
patch

Review of attachment 771752 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for tackling this! I haven't tested it myself, as I don't have a Win8.1 preview system set up, but it looks good overall.

Before we go ahead and land this, I'd like to at least know what approach we're going to take to the "FIXME" item in nsScreenManagerWin, even if we leave the actual fix for a followup.

::: widget/windows/nsScreenManagerWin.cpp
@@ +88,5 @@
>    // convert coordinates from logical to device pixels for MonitorFromRect
>    double dpiScale = nsIWidget::DefaultScaleOverride();
>    if (dpiScale <= 0.0) {
> +    //FIXME: need an appropriate monitor
> +    dpiScale = WinUtils::GetDPIScale(GetPrimaryMonitor());

I think we need to handle this somehow, otherwise there's probably a risk that in certain edge cases we might restore a window onto the wrong monitor, for example.

How does Windows handle the coordinate space across multiple monitors with varying DPI scale? For global window placement, we really want to be using a coordinate system that spans all the screens in a meaningful and unambiguous way. On OS X, that's the global "Cocoa pixels" coordinate space (whereas "device pixel" coordinates are only valid within a single device), but I wonder whether Windows has an equivalent concept. Or does it treat device pixels as the primary, continuous space, and then "logical DPI" is just a scale factor that applications apply on a per-screen basis? In that case, the "logical pixels" space may have discontinuities at the screen boundaries.

::: widget/windows/nsWindowBase.h
@@ +64,5 @@
>    {
>      return (mInputContext.mIMEState.mEnabled == IMEState::PLUGIN);
>    }
>  
> +  void ChangedDPI();

I don't see a definition of this method; is there a new nsWindowBase.cpp file that you forgot to "hg add" to the patch?
Assignee

Comment 4

6 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> ::: widget/windows/nsWindowBase.h
> @@ +64,5 @@
> >    {
> >      return (mInputContext.mIMEState.mEnabled == IMEState::PLUGIN);
> >    }
> >  
> > +  void ChangedDPI();
> 
> I don't see a definition of this method; is there a new nsWindowBase.cpp
> file that you forgot to "hg add" to the patch?

Oh, never mind - I see it's in nsWindow.cpp. Maybe it'd be good to move it to the top of the file rather than in the midst of the nsWindow methods?
Reporter

Comment 5

6 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> Comment on attachment 771752 [details] [diff] [review]
> patch
> 
> Review of attachment 771752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for tackling this! I haven't tested it myself, as I don't have a
> Win8.1 preview system set up, but it looks good overall.
> 
> Before we go ahead and land this, I'd like to at least know what approach
> we're going to take to the "FIXME" item in nsScreenManagerWin, even if we
> leave the actual fix for a followup.
> 
> ::: widget/windows/nsScreenManagerWin.cpp
> @@ +88,5 @@
> >    // convert coordinates from logical to device pixels for MonitorFromRect
> >    double dpiScale = nsIWidget::DefaultScaleOverride();
> >    if (dpiScale <= 0.0) {
> > +    //FIXME: need an appropriate monitor
> > +    dpiScale = WinUtils::GetDPIScale(GetPrimaryMonitor());
> 
> I think we need to handle this somehow, otherwise there's probably a risk
> that in certain edge cases we might restore a window onto the wrong monitor,
> for example.
> 
> How does Windows handle the coordinate space across multiple monitors with
> varying DPI scale? For global window placement, we really want to be using a
> coordinate system that spans all the screens in a meaningful and unambiguous
> way. On OS X, that's the global "Cocoa pixels" coordinate space (whereas
> "device pixel" coordinates are only valid within a single device), but I
> wonder whether Windows has an equivalent concept.

On Windows 8.1, global screen coordinates depend on the DPI awareness of the app.
For example, consider the following monitor settings:
  Monitor 0 (primary): (0,0)-(2560,1440), 192 dpi
  Monitor 1 : (2560,0)-(3584,768), 96 dpi
Per-monitor DPI aware apps will see the above coordinates as-is.
For DPI unaware apps, all coordinates will be scaled to 96 dpi:
  Monitor 0 (primary): (0,0)-(1280,720), 96 dpi
  Monitor 1 : (2560,0)-(3584,768), 96 dpi
For system DPI aware apps, all coordinates will be scaled to the system dpi:
  Monitor 0 (primary): (0,0)-(2560,1440), 192 dpi
  Monitor 1 : (5120,0)-(7158,1536), 192 dpi

> Or does it treat device
> pixels as the primary, continuous space, and then "logical DPI" is just a
> scale factor that applications apply on a per-screen basis? In that case,
> the "logical pixels" space may have discontinuities at the screen boundaries.

Yes, logical coordinates may be discontinuities as shown in the above example.

Comment 6

6 years ago
(In reply to Masatoshi Kimura [:emk] from comment #5)
> On Windows 8.1, global screen coordinates depend on the DPI awareness of the
> app.
> For example, consider the following monitor settings:
>   Monitor 0 (primary): (0,0)-(2560,1440), 192 dpi
>   Monitor 1 : (2560,0)-(3584,768), 96 dpi
> Per-monitor DPI aware apps will see the above coordinates as-is.
This doesn't sit right with me. Shouldn't inactive monitors be virtualized to the DPI of the active monitor? (Screen coordinates would depend on the current DPI of the window.) That's what the word document seemed to be saying. Too bad I don't have Windows 8.1 to test.
Reporter

Comment 7

6 years ago
(In reply to Greg Edwards from comment #6)
> This doesn't sit right with me. Shouldn't inactive monitors be virtualized
> to the DPI of the active monitor?

What is "active monitor"? The word doesn't appear in the whitepaper.

> (Screen coordinates would depend on the
> current DPI of the window.)

The window is used to identify the DPI-awareness of the owner app.

> Too bad I don't have Windows 8.1 to test.

I actually tested the multimonitor configuration and wrote the result here.
Reporter

Comment 8

6 years ago
Posted file Test program, FTR

Comment 9

6 years ago
I'm not doubting you, it just seems like a strange design choice by Microsoft. The formula to convert to logical coordinates would be weird and depend on the monitor arrangement.
Reporter

Comment 10

6 years ago
Posted patch WIP2 (obsolete) — Splinter Review
- Use a suggested size on WM_DPICHANGE.
- Implement nsScreenManagerWin::ScreenForRect using logical coordinates.
- Use GetDpiForMonitor only when the process is per-monitor DPI aware because
  GetDpiForMonitor is not virtualized regardless of the DPI awareness.

Opened windows are not positioned correctly yet on secondary monitors.
Attachment #771752 - Attachment is obsolete: true
Attachment #771752 - Flags: review?(jfkthame)
Duplicate of this bug: 995859
Minor question -- does "True/PM" in the manifest work properly on older windows versions?  i.e. 7 etc where we just want it to behave as if it was "true"?
Reporter

Comment 13

5 years ago
Yes. Microsoft carefully chose the string so that it would work on down-level systems.
Reporter

Comment 14

5 years ago
If the developers don't care about older systems, they can use the string "Per Monitor".
Reporter

Comment 15

5 years ago
Unassigning because I couldn't sort out the comment #5 issue.

If a high-dpi monitor is on the right side of a low-dpi monitor, the global logical display pixel will overlap on Windows 8.1 per-monitor dpi environments. This will lead a wrong choice of the screen for pop-ups. This is the opposite of bug 814434 on Mac OS X.

For example, consider the following configuration:
Primary monitor: (0,0)-(1024,768), 96 dpi
Secondary monitor: (1024,0)-(3072,1536), 192 dpi

The global display coordinates of the second monitor would be (512,0)-(1536,768). So ScreenForRect() is unimplementable on Windows 8.1 per-monitor dpi environments.

To resolve this issue, either
1. Use physical device pixels for Move() and Resize() on Windows, or
2. Make logical-to-physical conversion more complex than the simple multiplication by the scale factor.

I tried both without success and I gave up.
Assignee: VYV03354 → nobody
Status: ASSIGNED → NEW
Reporter

Comment 16

5 years ago
Attachment #775103 - Attachment is obsolete: true

Comment 19

5 years ago
(In reply to Masatoshi Kimura [:emk] from comment #15)

> Unassigning because I couldn't sort out the comment #5 issue.

I tested Firefox 34.0.5 with the <dpiAware>True/PM</dpiAware> setting [1] and the fonts are much more readable.
Is it possible to make this default?


[1] http://msdn.microsoft.com/en-us/library/windows/desktop/dn469266(v=vs.85).aspx#declaring_dpi_awareness
[2] http://technet.microsoft.com/en-us/library/dn528847.aspx
Reporter

Comment 22

5 years ago
(In reply to mm.bugzilla from comment #19)
> I tested Firefox 34.0.5 with the <dpiAware>True/PM</dpiAware> setting [1]
> and the fonts are much more readable.
> Is it possible to make this default?

This is not as easy as just adding the manifest.
1. If two monitors have different DPI, Firefox will not change the size when the window is moved across the monitors.
2. DPI can be changed dynamically on Win8.1. Again, Firefox will not respond the DPI change until the next sign on.
We will have to add a message handler for WM_DPICHANGE. But I'm stuck on the technical problem written in comment #5.

Comment 23

5 years ago
(In reply to Masatoshi Kimura [:emk] from comment #22)
> This is not as easy as just adding the manifest.
> 1. If two monitors have different DPI, Firefox will not change the size when
> the window is moved across the monitors.

Please look at the screenshots I attached. I'm using two displays, fist 1920x1080 with 12.5" and second 2560x1440 with 27". The left instance of Firefox has the patched manifest.
So somehow the DPI gets adjusted right in firefox or am I missing something?

> 2. DPI can be changed dynamically on Win8.1. Again, Firefox will not respond
> the DPI change until the next sign on.
I prefer a one time relogin to a permanent display error.

> We will have to add a message handler for WM_DPICHANGE. But I'm stuck on the
> technical problem written in comment #5.

My uneducated guess are the LogicalToPhysicalPointForPerMonitorDPI and PhysicalToLogicalPointForPerMonitor functions, with them you may be able to get the "right" coordinates.
Reporter

Comment 24

5 years ago
(In reply to mm.bugzilla from comment #23)
> Please look at the screenshots I attached. I'm using two displays, fist
> 1920x1080 with 12.5" and second 2560x1440 with 27". The left instance of
> Firefox has the patched manifest.
> So somehow the DPI gets adjusted right in firefox or am I missing something?

I need to know the DPI value for each your monitor to know what is the expected behavior, but I'm assuming your first monitor has the higher DPI than the second. Firefox should scale down the window in this case when you dragged the window to the second monitor. That is, the screenshot shows exactly the lacking feature.

> > 2. DPI can be changed dynamically on Win8.1. Again, Firefox will not respond
> > the DPI change until the next sign on.
> I prefer a one time relogin to a permanent display error.

Display error will be fixed on relogin anyway.

> > We will have to add a message handler for WM_DPICHANGE. But I'm stuck on the
> > technical problem written in comment #5.
> 
> My uneducated guess are the LogicalToPhysicalPointForPerMonitorDPI and
> PhysicalToLogicalPointForPerMonitor functions, with them you may be able to
> get the "right" coordinates.

The problem is an architectural difference between OS X and Windows. On OS X. Screen coordinates are managed using "virtual" coordinates adjusted to the same DPI on all display. On Windows, it is managed by the physical display pixels. Since High-DPI support for Firefox was introduced on OS X first, Firefox assumes the virtual screen coordinates model. It is really hard (if at all possible) to accommodate the model on Windows.

Comment 25

5 years ago
(In reply to Masatoshi Kimura [:emk] from comment #24)
> I need to know the DPI value for each your monitor to know what is the
> expected behavior [...]
Display: physical dpi (windows dpi [1])
First display: 176dpi (120)
Second display: 108dpi (96)

[1] GetDpiForMonitor --> https://emoacht.wordpress.com/2013/10/

Comment 27

4 years ago
Hi all I'm having an issue. If I create a single window the size of all monitors and position it at top left most point there are no issues:

http://i.imgur.com/qlWmwZo.png

But if I create a window per monitor the non-primary monitor is bigger then it should be:
http://i.imgur.com/48fHFxP.png

Does anyone know why?

Heres more info my dilemma: http://stackoverflow.com/questions/31499545/understanding-multi-monitor-dpi/31500103#31500103
http://imgur.com/qlWmwZo,48fHFxP

Comment 28

4 years ago
Actually the why is the non-dpi awareness as explored in that stackoverflow topic, is there a chance that we can land a fix to this bug soon? :)

Updated

4 years ago
Flags: firefox-backlog?

Comment 29

4 years ago
I'm working around the dilemma by two methods:
1) prevent rescale when moved to another monitor by making the window i open on other monitors resized so its big enough that it reaches and covers at least 50% of primary monitor - i dont know why but this makes it prevent rescaling
2) im figuring out the scale factor then drawing things that much smaller/bigger: http://stackoverflow.com/questions/31508754/in-win81-non-dpi-aware-process-figure-out-scale-factor?noredirect=1#comment50988819_31508754
It sounds like we need to define our own DPI-independent screen coordinate space for Windows, and have a complex mapping between that and Windows' internal screen coordinates. I guess it's a bit tricky but not too hard. Is it hard for some reason I can't see?
Flags: needinfo?(VYV03354)

Comment 32

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> It sounds like we need to define our own DPI-independent screen coordinate
> space for Windows, and have a complex mapping between that and Windows'
> internal screen coordinates. I guess it's a bit tricky but not too hard. Is
> it hard for some reason I can't see?

There are two patches provided above, they do change the approach and implement runtime change of scaling per window. I've rebased the main patch on top of a more recent version in comment 26. I did find a couple issues though, e.g. context menus aren't sized correctly.
dolske, trying to get this on the fx team's radar. If the front end team considers this bug a priority the platform team might be able to rearrange some schedules to find some developer time. At this point though no one has the free cycles.
Flags: needinfo?(VYV03354) → needinfo?(dolske)
Reporter

Comment 34

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> It sounds like we need to define our own DPI-independent screen coordinate
> space for Windows, and have a complex mapping between that and Windows'
> internal screen coordinates. I guess it's a bit tricky but not too hard. Is
> it hard for some reason I can't see?

It was hard mainly because of my (lack of) ability. I'd appreciate it if you take this bug.
(In reply to Jim Mathies [:jimm] from comment #33)
> dolske, trying to get this on the fx team's radar. If the front end team
> considers this bug a priority the platform team might be able to rearrange
> some schedules to find some developer time. At this point though no one has
> the free cycles.

I don't have any specific data to help guide prioritization (like how many users have multiple-display setups with differing DPIs), but I do think this is valuable. We improved a lot of hidpi support in Firefox 40 (by adding hidpi assets for the Windows theme), and when users encounters situations where the browser is being scaled it sure looks bad.

This is basically a Windows 10 issue, no? AIUI Microsoft didn't support per-monitor DPI settings until Windows 10, previously one could only specify a single scaling factor to be used on all displays.
Flags: needinfo?(dolske)
Per-monitor DPI was introduced in Windows 8.1.
Assignee

Comment 37

4 years ago
I've been trying to make some headway here, and am currently leaning towards the opinion that on per-monitor-dpi Windows systems, we need to use the "native" device pixels as the coordinate space in which we manage window positions.

There's a try build in progress at https://treeherder.mozilla.org/#/jobs?repo=try&revision=84a132f3e7d7 that includes some (hacky) patches to make this work for menus/popups, and this seems to be a step in the right direction. It still has problems with window positioning (e.g. when restoring saved window positions or putting up an alert on a secondary display) as much of that code hasn't yet been touched.

Hope to have a more complete patch available for testing fairly soon...
Assignee

Comment 38

4 years ago
Here's my current WIP on this bug.... it's not nearly review-ready yet, but shows some progress at least.
Assignee

Comment 39

4 years ago
:kip, I understand you have a multi-monitor/mixed-DPI configuration; would you be willing to try including the WIP patch above in your build, and see how things go?

A couple of issues that I know are still broken:

- When dragging a window between monitors with different DPI, the menu bar is mis-rendered (we leave too much or too little height for it). You can hide and re-show the menu bar to fix this, or maximize/restore the window.

- When a window is opened on a secondary display (either restoring a session, or opening an alert, etc) the positioning is often wrong.

Things that *should* work OK include menus (both the menubar and in-content), as well as fullscreen mode (F11) on both primary and secondary displays. And content should be rendered at the native resolution, which is particularly noticeable in the form of crisper text.
Flags: needinfo?(kgilbert)
Yes, my home machine has this setup -- I'd be glad to try this out tonight.

Comment 41

4 years ago
I just wanted to share my finds when working with multi monitor before landing this patch.

I found that if you move the window to the other monitor it gets magnified, but if you drag the corner of that window to resize it (dont move it) so that a majority of that window is covering the primary monitor, then it will not be magnified. I thought that was interesting.

For right now though I put my window to the other window and I drew on a canvas and scaled it. Did this in my addon NativeShot: https://addons.mozilla.org/en-US/firefox/addon/nativeshot/

Just found it interesting, would be nice to take that work around out :)
I have a mixed-DPI Windows machine as my main working platform. I'm glad to test this as well. ni? myself so that I remember to test once I have time.
Flags: needinfo?(quanxunzhen)
I did some test on Windows 10, and what I found the following issues:

1. The titlebar in hidpi monitor is smaller than normal (for e.g. Library, Browser Console, or the browser window with "browser.tabs.drawInTitlebar" set to false).

2. Scrollbar are narrow than normal in hidpi monitor.

3. The scroll wheel is only effective when the pointer is in some areas (it is an existing issue even when we do not use per-monitor DPI).

4. On hidpi monitor, the fullscreen transition animation failed to cover the whole screen (which is probably something I should figure out why).

5. When a window is opened on the boundary of two different dpi monitors, the window may be larger than normal. To reproduce this issue, open "View Certificates" in Options / Advanced / Certificates, and drag the window to the left of some boundary, then choose any certificate and click "View". The certificate window opened across the boundary, which is larger than it should be.

6. There's a chance that drag window across the dpi boundary back and forth could incorrectly make the window larger. (This issue is easy to reproduce for me on Connection Settings window opened from Options / Advanced / Network.)

7. The checkboxs and radioboxs in the Connection Settings window are smaller than normal on hidpi monitor.

8. The About window is broken (larger than it should be) on hidpi monitor.


That's what I found by now.

Thanks for working on this, jfkthame!
Flags: needinfo?(quanxunzhen)
Assignee

Comment 44

4 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #43)
> I did some test on Windows 10, and what I found the following issues:

Thanks for the notes. A couple comments:

> 1. The titlebar in hidpi monitor is smaller than normal (for e.g. Library,
> Browser Console, or the browser window with "browser.tabs.drawInTitlebar"
> set to false).

AFAICT, I think this is expected behavior (though I don't like it): according to [1], "Note that the non-client area of a per monitor–DPI aware application is not scaled by Windows, and will appear proportionately smaller on a high DPI display."

I see a similarly small titlebar (and matching small menu when I right-click it) with an Internet Explorer window on the secondary, high-dpi display.

Not sure if there's anything we can do to change this at present...

> 2. Scrollbar are narrow than normal in hidpi monitor.

Again, I think this may be expected.... I see the same size of scrollbars in IE11.

> 3. The scroll wheel is only effective when the pointer is in some areas (it
> is an existing issue even when we do not use per-monitor DPI).

I'm not seeing this at the moment with my setup. How are your monitors arranged, and which is the primary screen?

This is probably worth a separate bug, anyhow, if it's an existing issue.

> 4. On hidpi monitor, the fullscreen transition animation failed to cover the
> whole screen (which is probably something I should figure out why).

Again, this seems to WFM at present, but may depend on the arrangement of the monitors.


[1] https://msdn.microsoft.com/en-us/library/windows/desktop/dn469266%28v=vs.85%29.aspx
(In reply to Jonathan Kew (:jfkthame) from comment #44)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #43)
> > I did some test on Windows 10, and what I found the following issues:
> 
> Thanks for the notes. A couple comments:
> 
> > 1. The titlebar in hidpi monitor is smaller than normal (for e.g. Library,
> > Browser Console, or the browser window with "browser.tabs.drawInTitlebar"
> > set to false).
> 
> AFAICT, I think this is expected behavior (though I don't like it):
> according to [1], "Note that the non-client area of a per monitor–DPI aware
> application is not scaled by Windows, and will appear proportionately
> smaller on a high DPI display."
> 
> I see a similarly small titlebar (and matching small menu when I right-click
> it) with an Internet Explorer window on the secondary, high-dpi display.
> 
> Not sure if there's anything we can do to change this at present...
> 
> > 2. Scrollbar are narrow than normal in hidpi monitor.
> 
> Again, I think this may be expected.... I see the same size of scrollbars in
> IE11.

Interesting. It is not what I see on some native applications on Windows 10 at least.

It seems to me the calcualtor has the normal titlebar size on both monitors, and cmd has both normal titlebar size and normal scrollbar.

But yes, I see the scrollbars in Edge become smaller. Microsoft themselves must haven't been completely clear with how to deal with per-monitor DPI :)

> > 3. The scroll wheel is only effective when the pointer is in some areas (it
> > is an existing issue even when we do not use per-monitor DPI).
> 
> I'm not seeing this at the moment with my setup. How are your monitors
> arranged, and which is the primary screen?

My primary screen is not a hidpi screen, this is probably related. I'll upload an attachment for my arrangement of monitors.

> This is probably worth a separate bug, anyhow, if it's an existing issue.

Probably yes.

> > 4. On hidpi monitor, the fullscreen transition animation failed to cover the
> > whole screen (which is probably something I should figure out why).
> 
> Again, this seems to WFM at present, but may depend on the arrangement of
> the monitors.
See the attachment. All my tests are currently based on this setting.

Monitor 1 is a HiDPI monitor, while monitor 2 and 3 are not. Monitor 2 is my primary monitor. Resolutions are: 1: 2880x1620, 2: 1920x1200, 3: 1200x1920

Comment 47

4 years ago
> AFAICT, I think this is expected behavior (though I don't like it): according to [1], "Note that the non-client area of a per monitor–DPI aware application is not scaled by Windows, and will appear proportionately smaller on a high DPI display."

Note that this will make the scrollbars/titlebar huge on low-dpi displays when your primary display is hi-dpi.
Reporter

Comment 48

4 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #45)
> It seems to me the calcualtor has the normal titlebar size on both monitors,
> and cmd has both normal titlebar size and normal scrollbar.

I don't know about the calculator, but cmd is not per-monitor DPI aware.

> > > 3. The scroll wheel is only effective when the pointer is in some areas (it
> > > is an existing issue even when we do not use per-monitor DPI).

AFAIK it is by design on Windows 10. Windows 10 will send a wheel message to the window which the mouse cursor points, not to the active window.
Assignee

Comment 49

4 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #45)

> Interesting. It is not what I see on some native applications on Windows 10
> at least.
> 
> It seems to me the calcualtor has the normal titlebar size on both monitors,
> and cmd has both normal titlebar size and normal scrollbar.

AFAICS on Windows 8.1, at least (I haven't done the update to Win10 yet), common Windows applications such as Calculator have not been made per-monitor-dpi-aware.

If you use Magnifier to look closely at Calculator on a secondary, high-dpi display (with the primary display being lower dpi), you can see that it is rendering low-dpi text and then being bitmap-scaled; compare the text within the window (lo-dpi) with the application name in the title bar (rendered at high dpi). It's much blockier.

There's a shortage of true per-monitor-dpi applications to compare with at present....
Assignee

Comment 50

4 years ago
(In reply to LordJZ from comment #47)
> > AFAICT, I think this is expected behavior (though I don't like it): according to [1], "Note that the non-client area of a per monitor–DPI aware application is not scaled by Windows, and will appear proportionately smaller on a high DPI display."
> 
> Note that this will make the scrollbars/titlebar huge on low-dpi displays
> when your primary display is hi-dpi.

Right; this is also what I see with IE11.
On Windows 10, it seems to me most builtin apps have been per-monitor DPI aware.

Comment 52

4 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #51)
> On Windows 10, it seems to me most builtin apps have been per-monitor DPI
> aware.

Note that on Windows 10 some built-in apps like Calculator are not being re-scaled correctly when transitioned to a different DPI monitor.

I have a small app that does position recalculation mostly correctly: (mouse pointer is a little bit off sometimes)
https://github.com/LordJZ/WindowPositions/releases/tag/v1.1
It also has issues with scrollbars (not titlebar as it draws it's own titlebar).
(In reply to LordJZ from comment #52)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #51)
> > On Windows 10, it seems to me most builtin apps have been per-monitor DPI
> > aware.
> 
> Note that on Windows 10 some built-in apps like Calculator are not being
> re-scaled correctly when transitioned to a different DPI monitor.

It works... sometimes... Yes, I can confirm that Calculator has the same issue 6 I mentioned in comment 43, and it is another "likely" buggy one. But Command Prompt works mostly fine at least.

Comment 54

4 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #53)
> Command Prompt works mostly fine at least.

It is not PM-DPI-aware.
(In reply to LordJZ from comment #54)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #53)
> > Command Prompt works mostly fine at least.
> 
> It is not PM-DPI-aware.

It is PM-DPI-aware. See the screenshots (since not really related to this bug, I use imgur instead of attachments): http://i.imgur.com/GysxVJ9.png (LoDPI) http://i.imgur.com/BlCCArn.png (HiDPI)
Reporter

Comment 56

4 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #55)
> (In reply to LordJZ from comment #54)
> > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #53)
> > > Command Prompt works mostly fine at least.
> > 
> > It is not PM-DPI-aware.
> 
> It is PM-DPI-aware.

No it is not. System will automatically stretch non-per-monitor DPI aware apps.

> See the screenshots (since not really related to this
> bug, I use imgur instead of attachments): http://i.imgur.com/GysxVJ9.png
> (LoDPI) http://i.imgur.com/BlCCArn.png (HiDPI)

Beware that https://i.imgur.com/GysxVJ9.png is a bit blurry.
But that window was opened in the LoDPI monitor.

If it is possible that we can always use HiDPI backend and let the system scale down to LoDPI, I guess we probably should take that approach instead of implementing the "true" PM-DPI support. I heard it is how OS X handles this.
OK, yes, you are right. The manifest of cmd indicates it is DPI-aware but not PM-DPI-aware. Then it's interesting how it could have the HiDPI setting even when my primary monitor is LoDPI.
Reporter

Comment 59

4 years ago
The primary monitor is always Monitor 1 (that is, the High DPI monitor on your configuration). It is not relevant what monitor the window was opened in.
All other non-PM-DPI-aware applications (including Firefox) uses LoDPI on my machine whatever monitor the window was opened in.
Reporter

Comment 61

4 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #60)
> All other non-PM-DPI-aware applications (including Firefox) uses LoDPI on my
> machine whatever monitor the window was opened in.

How did you confirm? For Firefox, probably it is just Firefox does not have High-DPI assets. So Firefox stretches images and it looks blurry. For other apps, probably they are not even system-DPI aware.
(In reply to Masatoshi Kimura [:emk] from comment #61)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #60)
> > All other non-PM-DPI-aware applications (including Firefox) uses LoDPI on my
> > machine whatever monitor the window was opened in.
> 
> How did you confirm? For Firefox, probably it is just Firefox does not have
> High-DPI assets. So Firefox stretches images and it looks blurry. For other
> apps, probably they are not even system-DPI aware.

All text in Firefox is blurry in my monitor 1... This is also true for GVIM, Paint.NET, etc...

When I do not use my external monitors, they work perfectly with the HiDPI laptop screen.

Also, it is easy to confirm whether an app is system-DPI-aware: you just need to open the exe file in notepad, and search for "dpiAware".
Summary: Support per-monitor DPI on Windows 8.1 desktop → Support per-monitor DPI on Windows 8.1 & 10 desktop
Assignee

Comment 63

4 years ago
I've submitted a tryserver job with updated WIP patches for this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4c9b3b18a7c.

:xidorn, :emk, :kip, others... if you could give that build a try and see what does/doesn't work, that would be helpful. AFAICT it should be quite a bit better than before, anyhow.

There are still some issues with remembering the positioning of windows such as Library, etc., when re-opening them if the parent window has been moved between displays. And it still has a problem with the About Firefox window being somewhat too tall on a secondary hi-dpi display. 

(And the patches currently include some hacks that will break behavior on other platforms, so don't even bother trying that for the time being.)
Flags: needinfo?(quanxunzhen)
Flags: needinfo?(VYV03354)
Assignee

Comment 64

4 years ago
Updated WIP... contains various ugly hacks, not ready for review.
Assignee

Updated

4 years ago
Attachment #8668578 - Attachment is obsolete: true
Assignee

Comment 65

4 years ago
WIP pt 2, updates to windows native-theme code.
Assignee

Comment 66

4 years ago
Try build with an improved set of patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d83dd1115274

I'll update the patches here after some more cleanup, but in the meantime any testing of the build above would be appreciated.
(In reply to Xidorn Quan [:xidorn] (UTC+9) from comment #43)
> I did some test on Windows 10, and what I found the following issues:
> 
> 1. The titlebar in hidpi monitor is smaller than normal (for e.g. Library,
> Browser Console, or the browser window with "browser.tabs.drawInTitlebar"
> set to false).

Not fixed unfortunately. Not very important anyway if it is hard to fix.

> 2. Scrollbar are narrow than normal in hidpi monitor.

This seems to be fixed.

> 3. The scroll wheel is only effective when the pointer is in some areas (it
> is an existing issue even when we do not use per-monitor DPI).

Later I confirmed this was a bug of some software else I installed, not ours.

> 4. On hidpi monitor, the fullscreen transition animation failed to cover the
> whole screen (which is probably something I should figure out why).

This is magically fixed.

> 5. When a window is opened on the boundary of two different dpi monitors,
> the window may be larger than normal. To reproduce this issue, open "View
> Certificates" in Options / Advanced / Certificates, and drag the window to
> the left of some boundary, then choose any certificate and click "View". The
> certificate window opened across the boundary, which is larger than it
> should be.

Cannot reproduce anymore, but if that window is opened in my HiDPI display, the whole window is in its low dpi state. Dragging the window to a low dpi display and back makes everything works fine then.

> 6. There's a chance that drag window across the dpi boundary back and forth
> could incorrectly make the window larger. (This issue is easy to reproduce
> for me on Connection Settings window opened from Options / Advanced /
> Network.)

Cannot reproduce anymore.

> 7. The checkboxs and radioboxs in the Connection Settings window are smaller
> than normal on hidpi monitor.

The checkboxs and radioboxs are no longer small, but when I open this window in a low dpi display, and drag to the hidpi display, the checkboxs and radioboxs become blurred as if they are scaled.

> 8. The About window is broken (larger than it should be) on hidpi monitor.

The About window is no longer larger, but like the certificate view dialog, it is in its low dpi state even if it is opened in a hi-dpi display. Dragging to a low dpi display and back probably corrects its size, but the three links on the bottom are overflow and invisible then.
Flags: needinfo?(quanxunzhen)
Assignee

Comment 68

4 years ago
Still some (relatively minor) issues to try and fix up here, but I'm uploading my current set of WIP patches...
Assignee

Updated

4 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee

Updated

4 years ago
Attachment #8681279 - Attachment is obsolete: true
drive-by comment: It would be *really* nice if we could add a type for "desktop pixels" into layout/base/Units.h and use strongly typed units throughout this code rather than plain double/int values which are going to get confusing real quick.
Assignee

Updated

4 years ago
Attachment #8683835 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8683836 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8683837 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8683838 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8683839 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8683840 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8683841 - Attachment is obsolete: true
Assignee

Comment 86

4 years ago
I've posted my current patches here, and would appreciate any testing people can do. In theory, we should be able to land everything up to the manifest change without affecting existing behavior; it's only when we add per-monitor-awareness to the manifest that things happen.

A bit of this patch set has been updated to work with a new DesktopPixel type, but there's scope to extend that further and replace some more of the untyped pixel/scaling code, I think.

(With these patches, I'm still seeing a problem with the About window size, as well as some other minor issues, so still hope to improve things before we're ready to actually ship this.)
Assignee

Updated

4 years ago
Attachment #8689656 - Attachment is obsolete: true
Assignee

Comment 88

4 years ago
I'm going to attach an updated copy of my patches here (probably not all have changed since the last set, but a number of them have). These seem to work fairly well in my testing on Win8.1; however, there are some undesired side-effects on an OS X multi-monitor setup, so I need to investigate that further before we're ready to review stuff here.
Assignee

Updated

4 years ago
Attachment #8689654 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8689655 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8689724 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8689657 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8689659 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8689660 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8689661 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8689662 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8689663 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8692976 - Attachment is obsolete: true
Assignee

Comment 100

4 years ago
Try build with latest patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=100e1a534ba7

In addition to the usual smattering of intermittents, I expect there will be a reftest failure (in indic-clusters-1.html) due to an unrelated patch in my local queue, so that can be ignored here.

Once builds are ready, testing would be very welcome -- both on multimonitor/mixed-dpi Windows (8.1 or 10) systems, and on older Windows and other platforms to check for any accidental regressions there.


(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #67)

> ...if that window is opened in my HiDPI display,
> the whole window is in its low dpi state. Dragging the window to a low dpi
> display and back makes everything works fine then.

I suspect this might be related to running with e10s enabled; could you try disabling e10s and see if that affects the behavior? Thanks.

> The About window is no longer larger, but like the certificate view dialog,
> it is in its low dpi state even if it is opened in a hi-dpi display.

Again, does disabling e10s affect this?


These two issues...

> 1. The titlebar in hidpi monitor is smaller than normal (for e.g. Library,
> Browser Console, or the browser window with "browser.tabs.drawInTitlebar"
> set to false).

> The checkboxs and radioboxs are no longer small, but when I open this window
> in a low dpi display, and drag to the hidpi display, the checkboxs and
> radioboxs become blurred as if they are scaled.

will not be fixed, because AFAICT they're not currently fixable through the available Windows APIs. I'm hoping to discuss this with some people at MS to explore possible ways forward.

(If you make the hidpi monitor as your primary display, and log out/in to Windows, the behavior will be a bit different -- e.g. instead of the titlebar being small on hidpi, it'll be correct there but oversized on lodpi. Still the same underlying problem, though.)
Flags: needinfo?(quanxunzhen)
Immediately see a major issue: all popups are mispositioned in my hidpi monitor, including dropdowns, tooltips, and context menus from both the toolbar and the content. It seems this is only a rendering issue, because it seems the widgets correctly get hovered in the position it should be (so different from where it is displayed.) And if the window is initially open in the hidpi monitor, the main menu (from the three bar button) keeps empty. This issue is there whether e10s is enabled or not.

Another issue is that, if I put the devtools into a separate window, that window doesn't have titlebar. It is not affected by e10s as well.

When moving the browser window from hidpi monitor to lowdpi monitor, the toolbar buttons may be collapsed to an right arrow even if they shouldn't (which means if I resize the window larger to expand them, and then back to the original size, the buttons keep being expanded instead of collapsing again.) It happens if the window is narrow enough but not too narrow. This isn't affected by e10s. Probably we dispatch resize event in a wrong time?

About the resize event, it seems a little too many resize events are dispatched. There are consistently three resize events dispatched to the content for moving across boundary in both direction. During handling the first and second events, content would get the inner window size in the new size with the previous dpi (which means it is double-sized or half-sized for a short while), and the last one is correct. It is a bit better for e10s because the content window is not directly affected by the dpi change, but the content's inner window size is still affected by the incorrectly-sized UI components. (Also there is a three pixels difference for the outer window size in different monitors, which may be because of the titlebar size change.)

(In reply to Jonathan Kew (:jfkthame) from comment #100)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #67)
> 
> > ...if that window is opened in my HiDPI display,
> > the whole window is in its low dpi state. Dragging the window to a low dpi
> > display and back makes everything works fine then.
> 
> I suspect this might be related to running with e10s enabled; could you try
> disabling e10s and see if that affects the behavior? Thanks.
> 
> > The About window is no longer larger, but like the certificate view dialog,
> > it is in its low dpi state even if it is opened in a hi-dpi display.
> 
> Again, does disabling e10s affect this?

I do not see these two issues with the latest build, with or without e10s enabled.

But if I followed the steps to open the certificate details window, when I drag it from the hidpi monitor to the lowdpi monitor, it becomes double sized with the extra area black-filled (the content is in normal size). And if I move that window back to hidpi monitor, the window size keeps doubled, and the content refill the whole window (so it's now really double sized). It seems this happens for other dialogs as well. And it is not affected by e10s.
Flags: needinfo?(quanxunzhen)
Assignee

Comment 102

4 years ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #101)
> Immediately see a major issue: all popups are mispositioned in my hidpi
> monitor, including dropdowns, tooltips, and context menus from both the
> toolbar and the content.

Ugh -- yes, what a mess. Sorry about the brokenness! I made a "trivial" last-minute change to fix an issue I was seeing on OS X, but failed to re-test it properly on Windows, and of course it broke.... :(

Here's a build (in progress) that should be much better:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fcd69e54f03

> Another issue is that, if I put the devtools into a separate window, that
> window doesn't have titlebar. It is not affected by e10s as well.

Huh, that's very strange. Yes, I can reproduce that (with latest build, too).
Assignee

Updated

4 years ago
Attachment #8693350 - Attachment is obsolete: true

Comment 104

4 years ago
Using this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fcd69e54f03 on primary 4K 175%, secondary 1080p 100%, Windows 10 10240

1. Scaling goes all sorts of nuts if "Disable DPI scaling" Windows compatibility option is enabled

2. When full-sized (Windows+Up Arrow or double-click the title bar) on a secondary display, a few pixels are chopped off off the top of the window.

3. Can confirm multiple redraws when window is moved to other display -- it also sometimes re-scales back to the ratio of the monitor it's been dragged from, effectively not re-scaling at all.

Thanks!

Comment 105

4 years ago
4. The <select> dropdown is slightly misplaced on hi-dpi display: http://www.w3schools.com/tags/tryit.asp?filename=tryhtml_select (few pixels to the left)
(In reply to LordJZ from comment #105)
> 4. The <select> dropdown is slightly misplaced on hi-dpi display:
> http://www.w3schools.com/tags/tryit.asp?filename=tryhtml_select (few pixels
> to the left)

if enabled, that might be e10s related.
Assignee

Updated

4 years ago
Attachment #8692972 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8692974 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8692975 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8693527 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8692979 - Attachment is obsolete: true
Assignee

Comment 112

4 years ago
(In reply to LordJZ from comment #104)
> Using this:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fcd69e54f03 on
> primary 4K 175%, secondary 1080p 100%, Windows 10 10240
> 
> 1. Scaling goes all sorts of nuts if "Disable DPI scaling" Windows
> compatibility option is enabled

Hmm. I have an idea how that can be fixed, in principle, but so far I've not succeeded in doing it without some unwelcome side effects under normal use. (Is there any significant use-case for that option, given a properly DPI-aware application? I'd be happy to just disable it altogether if we could.)

> 2. When full-sized (Windows+Up Arrow or double-click the title bar) on a
> secondary display, a few pixels are chopped off off the top of the window.

That's if your primary display is hi-dpi and the secondary is lo-dpi, right? (If they're the other way around, you see a few pixels of extra border at the top.)

This seems to be related to how Windows does not scale the non-client area of the window as one would expect (hence the tiny titlebars on a secondary hi-dpi screen, or extra-large ones on a secondary lo-dpi screen). I'd consider it a relatively minor cosmetic issue -- obviously we should try to fix/workaround it if we can, but it's not a blocker.

> 3. Can confirm multiple redraws when window is moved to other display

Yes.... this is more noticeable with e10s enabled, I think, though it can be a bit untidy even without.

> also sometimes re-scales back to the ratio of the monitor it's been dragged
> from, effectively not re-scaling at all.

I can confirm that occasionally the content doesn't get scaled correctly -- it seems like we've lost a DPI-change event somewhere -- though I can't reproduce this consistently. When it does happen, simply dragging the window back and forth across the monitors again should cause it to refresh properly.

(In reply to LordJZ from comment #105)
> 4. The <select> dropdown is slightly misplaced on hi-dpi display:
> http://www.w3schools.com/tags/tryit.asp?filename=tryhtml_select (few pixels
> to the left)

As jimm suggested in comment 106, I think this is an e10s issue (even without per-monitor DPI support, the dropdown tends to be slightly misplaced).


(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #101)
> Another issue is that, if I put the devtools into a separate window, that
> window doesn't have titlebar. It is not affected by e10s as well.

I'm no longer seeing this.

> When moving the browser window from hidpi monitor to lowdpi monitor, the
> toolbar buttons may be collapsed to an right arrow even if they shouldn't
> (which means if I resize the window larger to expand them, and then back to
> the original size, the buttons keep being expanded instead of collapsing
> again.) It happens if the window is narrow enough but not too narrow. This
> isn't affected by e10s. Probably we dispatch resize event in a wrong time?

Yes, I can reproduce this sometimes; I don't have a solution yet.

> But if I followed the steps to open the certificate details window, when I
> drag it from the hidpi monitor to the lowdpi monitor, it becomes double
> sized with the extra area black-filled (the content is in normal size)....

This should be fixed now.


New try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfaeddfb508e
Assignee

Updated

4 years ago
Attachment #8694909 - Attachment is obsolete: true

Comment 114

4 years ago
Using this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfaeddfb508e and the previous version,
I have a window on 4K 175% primary display, then disable this display in settings, and the window gets moved to 1080p 100% secondary display, and the scaling goes nuts, without obvious ways to fix it -- it stays broken until I enable the primary display again.
Assignee

Comment 115

4 years ago
(In reply to LordJZ from comment #114)
> Using this:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfaeddfb508e and the
> previous version,
> I have a window on 4K 175% primary display, then disable this display in
> settings, and the window gets moved to 1080p 100% secondary display, and the
> scaling goes nuts, without obvious ways to fix it -- it stays broken until I
> enable the primary display again.

AFAICT, Windows (at least up through Win8.1, the latest version described at [1]) does not fully support dynamic changes like that: if you disable the primary display, I think you have (implicitly) changed the system DPI (because the formerly-secondary display is now the only, hence primary, display on the system); and this is one of the changes that requires a Log Off / Log On to be properly recognized.

In some cases you might find that resizing the window will help refresh its content with proper scaling, but I suspect that various things (such as menus) will remain improperly scaled until you log off and on again, at which point the system and monitor DPI values will be consistent again.

[1] https://msdn.microsoft.com/en-gb/library/windows/desktop/dn469266(v=vs.85).aspx

Comment 116

4 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #115)
> 
> AFAICT, Windows (at least up through Win8.1, the latest version described at
> [1]) does not fully support dynamic changes like that: if you disable the
> primary display, I think you have (implicitly) changed the system DPI
> (because the formerly-secondary display is now the only, hence primary,
> display on the system); and this is one of the changes that requires a Log
> Off / Log On to be properly recognized.
> 

Right, in such a scenario system DPI will change on log off or reboot. However, all apps (including DWM-scaled and PM-DPI-aware like Edge or File Explorer) handle this scenario correctly. My own PM-DPI-aware apps also handle this correctly, and I didn't do anything specific for display disconnect to work. So I think this might be a bug in Firefox implementation.
Assignee

Comment 117

4 years ago
AFAICS there are two basic issues that make it currently broken. First, the window content doesn't get refreshed at the new scale. Manually resizing the window fixes this (at least for me; can you confirm?), so that actual webpage content looks OK; and once it's fixed, it stays fixed.

The second problem is that various UI-related things are not scaled properly; in particular, all system font sizes (e.g. text in tab titles and URL bar, menus, dialogs, etc) are wrong. And this persists until you actually log out and back in again; in particular, quitting and restarting the browser (without logging out of Windows) is NOT sufficient to fix it. Does this match your observation?

Comment 118

4 years ago
Right. I also see very similar behavior if the "Disable DPI scaling" compatibility option is enabled. These things may be related.
Assignee

Comment 119

4 years ago
This should improve the behavior of a maximized window on a secondary display, such that it fits properly at the top of the screen. Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd5824a494ed
Assignee

Comment 120

4 years ago
Experimental patch to work better under the Disable Scaling for HiDPI compatibility setting; but this may give inferior results in some cases under default settings (in particular, alert windows may be mis-sized in some situations). Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a99e3fe99d7
Assignee

Updated

4 years ago
Attachment #8692970 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8694907 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8692968 - Flags: review?(bugmail.mozilla)
Assignee

Updated

4 years ago
Attachment #8692969 - Flags: review?(bugmail.mozilla)
Assignee

Updated

4 years ago
Attachment #8692971 - Flags: review?(VYV03354)
Assignee

Updated

4 years ago
Attachment #8695927 - Flags: review?(VYV03354)
Assignee

Updated

4 years ago
Attachment #8694906 - Flags: review?(VYV03354)
Assignee

Updated

4 years ago
Attachment #8695933 - Flags: review?(VYV03354)
Assignee

Updated

4 years ago
Attachment #8694908 - Flags: review?(VYV03354)
Assignee

Updated

4 years ago
Attachment #8695263 - Flags: review?(VYV03354)
Assignee

Updated

4 years ago
Attachment #8692977 - Flags: review?(VYV03354)
Assignee

Updated

4 years ago
Attachment #8694910 - Flags: review?(VYV03354)
Assignee

Updated

4 years ago
Attachment #8695403 - Flags: review?(VYV03354)
Assignee

Comment 123

4 years ago
Most recent try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2791f0d4176

There are still some rough edges here -- in particular, the issue with occasionally failing to resize content properly when moving window across displays -- but I'd like to start getting patches reviewed so we can land this in Nightly sometime soon (probably hold it back from release channels until it's had more testing), and work on the remaining issues in followups if necessary.
Comment on attachment 8692968 [details] [diff] [review]
patch 0.0 - Declare a DesktopPixel type in Units.h, to be used for the coordinate system used by the host system to manage the desktop space

Review of attachment 8692968 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the detailed comment!
Attachment #8692968 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8692969 [details] [diff] [review]
patch 0.1 - Switch from 'display pixels' to 'desktop pixels' terminology in widget code

Review of attachment 8692969 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like a straightforward substitution, LGTM.
Attachment #8692969 - Flags: review?(bugmail.mozilla) → review+
Reporter

Updated

4 years ago
Flags: needinfo?(VYV03354)
Attachment #8692971 - Flags: review?(VYV03354) → review+
Reporter

Updated

4 years ago
Attachment #8692977 - Flags: review?(VYV03354) → review+
Reporter

Updated

4 years ago
Attachment #8694906 - Flags: review?(VYV03354) → review+
Reporter

Comment 126

4 years ago
Comment on attachment 8694908 [details] [diff] [review]
patch 5 - Make Windows native-theme code handle per-monitor DPI scaling when necessary

Review of attachment 8694908 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsNativeThemeWin.cpp
@@ +2000,5 @@
>    HANDLE theme = GetTheme(aWidgetType);
> +  nsresult rv = NS_OK;
> +  if (!theme) {
> +    rv = ClassicGetWidgetBorder(aContext, aFrame, aWidgetType, aResult);
> +    goto SCALE_AND_RETURN;

Please add a conversion function instead of using goto.
Attachment #8694908 - Flags: review?(VYV03354) → review+
Reporter

Updated

4 years ago
Attachment #8694910 - Flags: review?(VYV03354) → review+
Reporter

Updated

4 years ago
Attachment #8695263 - Flags: review?(VYV03354) → review+
Reporter

Comment 127

4 years ago
Comment on attachment 8695403 [details] [diff] [review]
patch 9 - Adjustment to non-client margin at top of maximized window on secondary display

Review of attachment 8695403 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +2280,5 @@
> +    // Any modifications here need to be tested for both high- and low-dpi
> +    // secondary displays, and for windows both with and without the titlebar
> +    // and/or menubar displayed.
> +    double ourScale = WinUtils::LogToPhysFactor(mWnd);
> +    double sysScale = WinUtils::LogToPhysFactor(WinUtils::GetPrimaryMonitor());

Is the primary monitor dpi is always equal to the system dpi? (I don't think so.)
We will need a WinUtil function to obtain the system dpi using ::GetDeviceCaps() even in per-monitor dpi mode.
Reporter

Updated

4 years ago
Attachment #8695927 - Flags: review?(VYV03354) → review+
Reporter

Comment 128

4 years ago
Comment on attachment 8695933 [details] [diff] [review]
patch 4 - Update widget/windows code for per-monitor DPI support

Review of attachment 8695933 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +184,5 @@
>  #endif
>  
> +#if !defined(WM_DPICHANGED)
> +#define WM_DPICHANGED 0x02E0
> +#endif

IIRC Win 8.1 SDK is the requirement now.
Attachment #8695933 - Flags: review?(VYV03354) → review+
Assignee

Comment 129

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #125)
> Comment on attachment 8692969 [details] [diff] [review]
> patch 0.1 - Switch from 'display pixels' to 'desktop pixels' terminology in
> widget code
> 
> Review of attachment 8692969 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems like a straightforward substitution, LGTM.

A bunch of recent changes in widget code (e.g. bugs 1229237, 1228125, 1225007) have conflicted with this, such that I'll need to post a fresh patch here after I get home from Mozlando and can re-test across multiple platforms and screen configurations.
CC'ing nick as well so that he's aware of the changes happening here, and can maybe avoid bitrotting your patches further.
Reporter

Comment 131

4 years ago
Comment on attachment 8695403 [details] [diff] [review]
patch 9 - Adjustment to non-client margin at top of maximized window on secondary display

Waiting for unbitrotted patches.
Attachment #8695403 - Flags: review?(VYV03354)
Assignee

Comment 132

4 years ago
(In reply to Masatoshi Kimura [:emk] from comment #127)
> Is the primary monitor dpi is always equal to the system dpi? (I don't think
> so.)
> We will need a WinUtil function to obtain the system dpi using
> ::GetDeviceCaps() even in per-monitor dpi mode.

I don't think we need that; AFAICT, getting the system dpi from GetDeviceCaps will always give the same result as getting the primary monitor's dpi.

Note that as far as "system dpi" is concerned, MSDN says:

"On a multiple monitor system, if hdc is the desktop, GetDeviceCaps returns the capabilities of the primary monitor."

I'm assuming that using the desktop HDC would be equivalent to getting "system dpi"; it's not clear to me that any other concept of "system dpi" exists for per-monitor-aware apps.
Assignee

Updated

4 years ago
Attachment #8692968 - Attachment is obsolete: true
Assignee

Comment 134

4 years ago
Updated/unbitrotted patch, carrying over r=kats.
Assignee

Updated

4 years ago
Attachment #8692969 - Attachment is obsolete: true
Assignee

Comment 135

4 years ago
Updated/unbitrotted patch, carrying over r=emk.
Assignee

Updated

4 years ago
Attachment #8695927 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8692971 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8694906 - Attachment is obsolete: true
Assignee

Comment 138

4 years ago
Updated/unbitrotted patch, carrying over r=emk.
Assignee

Updated

4 years ago
Attachment #8695933 - Attachment is obsolete: true
Assignee

Comment 139

4 years ago
Updated/unbitrotted patch, carrying over r=emk.
Assignee

Updated

4 years ago
Attachment #8694908 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8695263 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8694910 - Attachment is obsolete: true
Assignee

Comment 142

4 years ago
Updated/unbitrotted patch, carrying over r=emk.
Assignee

Updated

4 years ago
Attachment #8692977 - Attachment is obsolete: true
Assignee

Comment 143

4 years ago
This is just a bit of cleanup to make the next patch simpler.
Attachment #8706363 - Flags: review?(bugmail.mozilla)
Assignee

Comment 144

4 years ago
And this introduces a DesktopPixel-based version of nsIWidget::Create, because that's what we need to pass in some cases; this fixes the misuse of a LayoutDevice-pixel parameter that was introduced when it replaced the old (untyped) nsIntRect here.
Attachment #8706366 - Flags: review?(bugmail.mozilla)
Assignee

Comment 145

4 years ago
This is functionally the same as the previous patch 9, but I used the term 'primary' instead of 'system' DPI as that may be clearer. AFAICT this results in the appropriate sizing for maximized windows whether on lower- or higher-dpi secondary screens.
Attachment #8706368 - Flags: review?(VYV03354)
Assignee

Updated

4 years ago
Attachment #8695403 - Attachment is obsolete: true
Assignee

Comment 146

4 years ago
(In reply to Masatoshi Kimura [:emk] from comment #128)
> Comment on attachment 8695933 [details] [diff] [review]
> patch 4 - Update widget/windows code for per-monitor DPI support
> 
> Review of attachment 8695933 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindow.cpp
> @@ +184,5 @@
> >  #endif
> >  
> > +#if !defined(WM_DPICHANGED)
> > +#define WM_DPICHANGED 0x02E0
> > +#endif
> 
> IIRC Win 8.1 SDK is the requirement now.

FTR, I've left this as-is in the patch, as just using the 8.1 SDK isn't sufficient; we'd also need to change the value that WINVER gets #defined to during the build, otherwise this symbol still isn't available. I'm not sure what the ramifications of that would be -- seems like it would allow us to more easily inadvertently write code that will fail on pre-Win8.1 systems -- so I think we should leave this for now.
Attachment #8706363 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8706366 [details] [diff] [review]
patch 0.3 - Create a version of nsIWidget::Create that takes Desktop pixels, because that's what we actually need to pass in some cases

Review of attachment 8706366 [details] [diff] [review]:
-----------------------------------------------------------------

Some questions below. The changes to the cocoa widgetry seem more involved and maybe it would be good to have Markus take a look instead.

::: widget/PluginWidgetProxy.h
@@ +40,5 @@
> +  NS_IMETHOD Create(nsIWidget* aParent, nsNativeWidget aNativeParent,
> +                    const DesktopIntRect& aRect,
> +                    nsWidgetInitData* aInitData = nullptr) override
> +  {
> +    return PuppetWidget::Create(aParent, aNativeParent, aRect, aInitData);

I don't understand the purpose of this override. Can we just get rid of it and have the callers directly invoke the superclass's version?

::: widget/PuppetWidget.h
@@ +62,5 @@
> +                    nsNativeWidget aNativeParent,
> +                    const DesktopIntRect& aRect,
> +                    nsWidgetInitData* aInitData = nullptr) override
> +  {
> +    return nsBaseWidget::Create(aParent, aNativeParent, aRect, aInitData);

Ditto here

::: widget/android/nsWindow.h
@@ +85,5 @@
> +                      nsNativeWidget aNativeParent,
> +                      const DesktopIntRect& aRect,
> +                      nsWidgetInitData* aInitData) override
> +    {
> +        return nsBaseWidget::Create(aParent, aNativeParent, aRect, aInitData);

Ditto here, and in the gonk/gtk versions also

::: widget/cocoa/nsChildView.mm
@@ +562,5 @@
> +                             nsWidgetInitData* aInitData)
> +{
> +  DesktopToLayoutDeviceScale scale(GetDefaultScaleInternal());
> +  LayoutDeviceIntRect deviceRect = RoundedToInt(aRect * scale);
> +  return Create(aParent, aNativeParent, deviceRect, aInitData);

How is this different from the nsIWidget version?
Attachment #8706366 - Flags: review?(bugmail.mozilla)
Reporter

Updated

4 years ago
Attachment #8706368 - Flags: review?(VYV03354) → review+
Assignee

Comment 148

4 years ago
Comment on attachment 8706366 [details] [diff] [review]
patch 0.3 - Create a version of nsIWidget::Create that takes Desktop pixels, because that's what we actually need to pass in some cases

Review of attachment 8706366 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/PluginWidgetProxy.h
@@ +40,5 @@
> +  NS_IMETHOD Create(nsIWidget* aParent, nsNativeWidget aNativeParent,
> +                    const DesktopIntRect& aRect,
> +                    nsWidgetInitData* aInitData = nullptr) override
> +  {
> +    return PuppetWidget::Create(aParent, aNativeParent, aRect, aInitData);

Huh, odd. I originally tried that, but ended up having to do this here (and elsewhere) as a result of getting compile errors without it. Seemed like the compiler insisted that if I was overriding one version of Create in a subclass, I had to do them both or it couldn't resolve the method properly. But trying it again now, it seems to build fine; so maybe I was doing something else wrong at the time.

I'll re-test, and assuming all goes well, post a cut-down version of the patch for Markus to review.
Assignee

Comment 149

4 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #148)
> Comment on attachment 8706366 [details] [diff] [review]
> patch 0.3 - Create a version of nsIWidget::Create that takes Desktop pixels,
> because that's what we actually need to pass in some cases
> 
> Review of attachment 8706366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/PluginWidgetProxy.h
> @@ +40,5 @@
> > +  NS_IMETHOD Create(nsIWidget* aParent, nsNativeWidget aNativeParent,
> > +                    const DesktopIntRect& aRect,
> > +                    nsWidgetInitData* aInitData = nullptr) override
> > +  {
> > +    return PuppetWidget::Create(aParent, aNativeParent, aRect, aInitData);
> 
> Huh, odd. I originally tried that, but ended up having to do this here (and
> elsewhere) as a result of getting compile errors without it. Seemed like the
> compiler insisted that if I was overriding one version of Create in a
> subclass, I had to do them both or it couldn't resolve the method properly.
> But trying it again now, it seems to build fine; so maybe I was doing
> something else wrong at the time.

OK, now I remember. (Thanks to tryserver for reminding me of the failures: e.g. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a8c87b609ae.)

The issue is that when there's a polymorphic virtual function in a base class like nsIWidget, if a subclass overrides only one of the versions, this will have the effect of hiding the other signature(s), so that an attempt to call the non-overridden method on a subclass will fail to compile.

(In some cases, we may not actually need to call the hidden base method, and so in theory we'd be OK in such cases; but even then -Werror=overloaded-virtual would issue a warning, and combine that with warnings-as-errors.....)

So even though many of these overrides simply call the inherited version, they do have to be present in the source. The nsChildView case I believe really *is* redundant, so I'll remove that from the patch.
(In reply to Jonathan Kew (:jfkthame) from comment #149)
> The issue is that when there's a polymorphic virtual function in a base
> class like nsIWidget, if a subclass overrides only one of the versions, this
> will have the effect of hiding the other signature(s), so that an attempt to
> call the non-overridden method on a subclass will fail to compile.

|using| can import the base version into the derived class.
See bug 1238404.
Assignee

Comment 151

4 years ago
(In reply to Karl Tomlinson (ni?:karlt) from comment #150)
> |using| can import the base version into the derived class.

Oh yes, of course - thanks. That'll make things cleaner.
Assignee

Comment 152

4 years ago
This is basically fixing some "abuse" of LayoutDeviceIntRect that was introduced when bug 1229237 replaced (potentially ambiguous) nsIntRect usage in widget code with strongly-typed values. We used to use the nsIntRect parameter to nsIWidget::Create to pass either device pixels OR desktop pixels, depending on the specific usage; so here, we end up with separate methods for the two types.
Attachment #8706911 - Flags: review?(mstange)
Assignee

Updated

4 years ago
Attachment #8706366 - Attachment is obsolete: true
Comment on attachment 8706911 [details] [diff] [review]
patch 0.3 - Create a version of nsIWidget::Create that takes Desktop pixels, because that's what we actually need to pass in some cases

Review of attachment 8706911 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'm super excited about this!

::: widget/cocoa/nsCocoaWindow.mm
@@ +194,1 @@
>          nsCocoaUtils::CocoaRectToGeckoRect([screen visibleFrame]));

Should nsCocoaUtils::CocoaRectToGeckoRect return a DesktopIntRect?
Attachment #8706911 - Flags: review?(mstange) → review+
Assignee

Comment 154

4 years ago
(In reply to Markus Stange [:mstange] from comment #153)
> Should nsCocoaUtils::CocoaRectToGeckoRect return a DesktopIntRect?

Yes, that would make sense. Not only that; similarly, GeckoRectToCocoaRect should take one as its parameter, and GeckoRectToCocoaRectDevPix should take a LayoutDeviceIntRect. I'll file a followup bug to fix all of these (and update their callsites accordingly); should be a straightforward cleanup with no effect on behavior.
Assignee

Comment 155

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d6a782970351f4c8b09aa244dc3a2d18af7d523
Bug 890156 - patch 0.0 - Declare a DesktopPixel type in Units.h, to be used for the coordinate system used by the host system to manage the desktop space. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c868d65f75d92d68a34c49ef326fb493ff0c59
Bug 890156 - patch 0.1 - Switch from 'display pixels' to 'desktop pixels' terminology in widget code. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/87b35034828d7c4fc4f1cfd0e6cba4e9ce0b1f92
Bug 890156 - patch 0.2 - Remove the (unused) aRect parameter from nsBaseWidget::BaseCreate. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b5097c00c6373b07ec3140a1c0741732beaeda
Bug 890156 - patch 0.3 - Create a version of nsIWidget::Create that takes Desktop pixels, because that's what we actually need to pass in some cases. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc09fb02f2c9191098ff2ab9ba97ed57162e806b
Bug 890156 - patch 1 - Add nsIWidget::GetDesktopToDeviceScale() method. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/20c8f6d967569c5f878621c6ee15cbc1f3c51017
Bug 890156 - patch 2 - Expose DevicePixelsPerDesktopPixel through nsIBaseWindow and its implementations. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/443207a1d886ce2d13bc7328e51dbca2300f8fcd
Bug 890156 - patch 3 - Remove gfxWindowsPlatform::GetDPIScale and replace it with methods in WinUtils, ready for per-monitor DPI support. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/68d1b9a375b0b03f976d254aee2012a8d2b0ddb6
Bug 890156 - patch 4 - Update widget/windows code for per-monitor DPI support. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/c35bafe04d6f6c8e75b9435097edd11d323bfb97
Bug 890156 - patch 5 - Make Windows native-theme code handle per-monitor DPI scaling when necessary. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/153848bbb30d26dacc0972a379417dcd4f02249f
Bug 890156 - patch 6 - Update window placement code to work with desktop pixels, for per-monitor DPI support on Windows. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/1be1d936e2a2a2d83f446285a8cd65ccdf632906
Bug 890156 - patch 8 - Explicitly set line-height in the About dialog, to avoid discrepancies in spacing across screens with different DPI. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1764c8c51803b9a0676f8b1be97cdba0484670
Bug 890156 - patch 7 - Declare that we support Windows per-monitor DPI via the app manifest. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bde73f95eadfd2149890e0cb2b9dad871ffa1e3
Bug 890156 - patch 9 - Adjustment to non-client margin at top of maximized window on secondary display. r=emk
Depends on: 1239683
Depends on: 1239855

Updated

4 years ago
Depends on: 1240174
Depends on: 1240180

Updated

4 years ago
Depends on: 1240328
Depends on: 1240749
Depends on: 1242449

Updated

4 years ago
Depends on: 1242720
Are we going to ship this in Firefox 46 with all the regressions? Or are we going to uplift fixes for all the regressions from 47 to 46?
Assignee

Comment 158

4 years ago
(In reply to Marco Castelluccio [:marco] from comment #157)
> Are we going to ship this in Firefox 46 with all the regressions? Or are we
> going to uplift fixes for all the regressions from 47 to 46?

It's not yet clear to me where we'll end up. I'm hoping to have patches for some of the issues this week, and that they'll be safe enough to uplift to 46, given that we're reasonably early in the cycle. But if it doesn't look like it'll be good enough, we could back out the firefox.exe.manifest change so that we revert to system-wide DPI instead of per-monitor.

I figure we can afford to wait a couple weeks before deciding if we need to take that route; if we still have serious regressions at that point, we should hold back the feature.
Depends on: 1244957

Updated

3 years ago
Depends on: 1245442
IIUC from bug 1240085, this could have a site-compat impact for sites using APIs referencing screen coordinates.
Keywords: site-compat
Reporter

Comment 160

3 years ago
In bug 1240085, we are going to make this change invisible from content.
Reporter

Updated

3 years ago
Depends on: 1246346
Reporter

Updated

3 years ago
Depends on: 1246389

Updated

3 years ago
Depends on: 1246382
Assignee

Updated

3 years ago
Depends on: 1244129

Updated

3 years ago
Depends on: 1246815
Assignee

Updated

3 years ago
Depends on: 1247335
Jonathan, what is your thinking here on whether to back this out?  I see there are still 8 open regressions. Do you think you can fix most of them while 46 is still in aurora?
Flags: needinfo?(jfkthame)
Assignee

Comment 162

3 years ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #161)
> Jonathan, what is your thinking here on whether to back this out?  I see
> there are still 8 open regressions. Do you think you can fix most of them
> while 46 is still in aurora?

I'm working on patches at the moment, but I'm thinking that given the complexity of the issues (and the difficulty I have with testing across the diverse range of platforms and configurations that may be affected by them), it may be safest to back this out of Aurora now rather than waiting until close to the end of the cycle. We'll continue with fixes on mozilla-47, but that would remove the pressure to rush fixes into 46 when they may not be adequately tested.

This will also involve backing out several followups that have landed on top of the patches here; by my reckoning, we'll back out bug 1239007, bug 1239683, bug 1239855, bug 1240180, and bug 1242720 in addition to the patches on this bug.

(There's also bug 1240085, which is currently only on trunk but has a pending uplift request. If we go with the Aurora backout, though, that will no longer be relevant.)

I prepped a set of patches for these backouts yesterday, as it happens:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca94de688e03 (main try run)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d738c147264 (fixing the Windows build failure)

:emk, I think you've been following this most closely; would you agree with this approach?

:lizzard, if we agree on this, I'll go ahead and land the backouts on Aurora with a=backout -- OK?
Flags: needinfo?(lhenry)
Flags: needinfo?(jfkthame)
Flags: needinfo?(VYV03354)
Assignee

Updated

3 years ago
Flags: needinfo?(kgilbert)
That sounds ok to me. Thank you for planning for backout. If you really think that we will be in better shape by keeping these changes and pushing harder on fixing the regressions, then let me know.  We could also think about keeping this work on aurora for another cycle if that isn't more difficult than it's worth.
Flags: needinfo?(lhenry)
This may also be something we would want to get more QA testing for with a variety of hardware. 
Ryan is that possible?
Flags: needinfo?(ryanvm)
I'm not sure my team has the bandwidth for this right now. Maybe the Release QA team does.
Flags: needinfo?(ryanvm) → needinfo?(rares.bologa)
Florin is this something your team can put extra testing into, maybe on Nightly if not on 46 aurora? We could also try focusing a test day around it to try and get a broad range of different kinds of hardware from the community.
Flags: needinfo?(florin.mezei)
Reporter

Comment 167

3 years ago
I agree with the backout. It will take some time to stabilize the per-monitor DPI support.
Flags: needinfo?(VYV03354)
Assignee

Comment 168

3 years ago
Backed out from aurora, together with followups as noted in comment 162 (1239007, 1239683, 1239855, 1240180, 1242720):

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ae3b23b0dfef
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/95256c89d269
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/12ae79626cdf
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ca5baf4c8da3
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/7b96f22c3028
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/52c479d20d2b

Updated flags to wontfix for 46, and bumped target milestone to 47.
Target Milestone: mozilla46 → mozilla47
Discussed also with Rares and they don't have the bandwidth to take this. Adding Andrei so he can bring it up with the Release QA team.
Flags: needinfo?(rares.bologa)
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
Keywords: feature
Removing the site-compat keyword as per comment 160.
Keywords: site-compat
Depends on: 1248427
Marking fixed for 47.   We still have a lot of regressions here which we may need to track for 47. 
Ritu just a heads up that you may see issues here when 47 goes to aurora.
Flags: needinfo?(rkothari)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #169)
> Discussed also with Rares and they don't have the bandwidth to take this.
> Adding Andrei so he can bring it up with the Release QA team.

Bogdan is going to take ownership of this feature in terms of QA.
Flags: needinfo?(andrei.vaida)
QA Contact: bogdan.maris
Flags: needinfo?(rkothari)
Depends on: 1251217
No longer depends on: 1251217
Depends on: 1253446
Depends on: 1254019
Depends on: 1254020
Depends on: 1254026
Assignee

Updated

3 years ago
Depends on: 1255475
Assignee

Updated

3 years ago
Depends on: 1255515

Updated

3 years ago
Depends on: 1255645
Assignee

Updated

3 years ago
Depends on: 1256731
Depends on: 1258634
Assignee

Updated

3 years ago
Depends on: 1259492
Assignee

Updated

3 years ago
Duplicate of this bug: 1247495
Assignee

Updated

3 years ago
Depends on: 1259707
Reporter

Updated

3 years ago
Depends on: 1260214
Reporter

Updated

3 years ago
Depends on: 1262398
Assignee

Updated

3 years ago
Depends on: 1249279
Hey Johnathan, looks like this is rolling out in 47? I don't see a backing pref so I'm assuming the code is live on aurora now and will roll out with the 47 release.

Should we look into doing some qa coverage here by softvision?
Flags: firefox-backlog? → needinfo?(jfkthame)
Softvision has already been doing QA according to emails to release-drivers. I see emails from March 1 and March 29 testing this. The last email there said that a decision about disabling for 47 will be made the week before it goes to beta.
Assignee

Comment 176

3 years ago
Right, there's been softvision QA testing during the last several weeks. As of now, I am hoping it will stay enabled on 47 (provided bug 1249279 gets uplifted), but it'll be up to release drivers to make the final decision -- in a week or so, as I understand it.

(In reply to Jim Mathies [:jimm] from comment #174)
> I don't see a backing
> pref so I'm assuming the code is live on aurora now and will roll out with
> the 47 release.

Yes, it's live on nightly & aurora. It can't be controlled by a runtime pref, as the key to enabling/disabling the behavior is the change to the firefox.exe manifest (patch 7 here). So if we decide to hold back the feature, we'll need to revert that change when it hits beta.
Flags: needinfo?(jfkthame)
Assignee

Updated

3 years ago
No longer depends on: 1264222

Comment 177

3 years ago
I was affected by this bug. As I have a screenshot addon, I used ctypes to use native methods to capture screenshot of all monitors.

I then overlay a window on each monitor, giving the "convert all monitors into a single canvas" effect. To work around the scaling due to virtualization caused by "per monitor dpi unawareness" I would scale my canvas down. This led to blurriness.

So seeing this fixed is really awesome.

However I tested on nightly and my screenshot is still being scaled due to virtualization. I am using a single monitor, and I bumped up the DPI to test this feature. I restart the computer. I take a screenshot and it is scaled, which is unexpeted. I thought no more scaling should took place?

I also tried calling via ctypes - SetProcessDpiAwareness with value of 2 for PROCESS_PER_MONITOR_DPI_AWARE - https://msdn.microsoft.com/en-us/library/windows/desktop/dn302122%28v=vs.85%29.aspx - before any of my native calls, however the SetProcessDpiAwarness call returns E_ACCESSDENIED - 0x80070005.

Here is a video showing the virtualization causing the scaling in the screenshot:
https://www.youtube.com/watch?v=e2Cc8WVQ-ng


In summary - Using Firefox Nightly version 48.0a1 (2016-04-19) I am still seeing the virtualization due to DPI unawareness.

Comment 178

3 years ago
To correct my statement my screenshot is not being scaled, but the display of the image from within Firefox is being scaled. My image in that video is on a canvas, but if i loaded it as a png same situation.
Depends on: 1267374
Assignee

Updated

3 years ago
Depends on: 1270954
Depends on: 1276183
No longer depends on: 1276183

Updated

3 years ago
Depends on: 1278818

Updated

3 years ago
Depends on: 1279470

Updated

3 years ago
Depends on: 1279475

Updated

3 years ago
No longer depends on: 1279475
Reporter

Updated

3 years ago
Depends on: 1281778

Updated

3 years ago
Duplicate of this bug: 1285200
Depends on: 1259700
Blocks: 1307363

Updated

3 years ago
Depends on: 1285200

Updated

3 years ago
See Also: → 1327874
Depends on: 1334053

Updated

2 years ago
Depends on: 1345557
Reporter

Updated

2 years ago
Depends on: 1354020
You need to log in before you can comment on or make changes to this bug.