829963 - incorrect display of popup window on external display of retina mac

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jonathan Kew [:jfkthame]]

Filing this as a separate bug as it seems to be distinct from other multi-screen issues we've dealt with.

Originally from bug 828514 comment 9 (see also comment 0 there):

"However, there is still a problem with pop up windows.  When a pop up window is triggered on the external display, the pop up opens to the full width and height of the external display but the web page only appears in the top left 1/4 of the window.  The rest of the window is painted black.  When you drag the window to the retina display the black is then filled in with the rest of the website but the screen still remains in full screen.  It does not activate the OSX full screen mode, but simply enlarges to fill the screen."

Comment 1

[matthewcatalano]

After doing some testing this bug does not occur on all websites.  This may mean that it could be the way that the website creates pop up screens.  The fact that the issue didn't occur until Firefox 18 may also indicate that Firefox 18 changed the way it handles this type of pop up.

A specific example can be found at the following site after pressing the "ENTER" button:

https://businessaccess.citibank.citigroup.com/cbusol/signon.do

Comment 2

[matthewcatalano]

Additional testing has revealed that the full screen pop up is supposed to occur as it also occurs on the retina display.  The remaining anomaly is the black area on the external display which still can be reproduced 100% of the time.

Comment 3

[chrille_b]

I can confirm this bug. 

It looks like bug 821490. It seems that firefox does not get that the new window is on a LoDPI screen and not on a HiDPI screen.

Comment 4

[Jonathan Kew [:jfkthame]]

Yes, this may well be the same issue. I won't mark it "duplicate" just yet, until I'm more certain the cause (and fix) is the same, but that does seem likely.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: simplified testcase

The problem arises when the site passes width and height features in the window.open() call; it does not occur if the window is resized -after- opening.

This example demonstrates the issue on a Retina Mac with (non-HiDPI) external display. If the window is placed on the main monitor (the one with the menu bar, whichever that is), and the Click Me! button is clicked, it'll open a new window correctly; but if the original window is placed on the -secondary- monitor, the new window will have incorrectly-scaled/clipped content until it is again resized.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: screenshot, result of the above testcase on an external non-hidpi screen

Attaching a screenshot to illustrate the bug (as demonstrated by the testcase in attachment 706425 [details]), for those not in a position to reproduce it easily.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: experimental (not fully working!) patch

A part, at least, of the problem here seems to be that the resolution of the device context (as reflected in AppUnitsPerDevPixel()) is "stale" (still implies a HiDPI screen) at the time when nsView::WindowResized() is called to adjust the size of the window's content.

This patch tries to address that by calling CheckDPIChange() on the devContext within WindowResized(), to ensure it is up to date. (For good measure, it throws an assertion if it finds the value was not current; this is just to confirm when it actually comes into play.) As expected, this assertion is hit when opening a new window using the testcase.

This patch, however, doesn't fully work: it results in correctly-sized window -content-, but the window's -chrome- has double-size fonts, resulting in ugly, badly-clipped text.

So this isn't really the right place to fix the issue - or at least it's not the only place that needs fixing.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: screenshot of the testcase with experimental patch in nsView::WindowResized

This shows the result of the above patch: window content looks fine in the new window, but the chrome is a mess.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: fix hi-/lo-dpi scaling in window opened from script with explicit size on secondary display

The problem with system font sizes in the chrome, as in the preceding screenshot, is fixed by calling AppUnitsPerDevPixelChanged unconditionally from nsPresContext::UIResolutionChangedInternal, so that we don't skip it in the case where CheckDPIChange had already been called (from nsView::WindowResized) on the same device context. We then get correct scaling in the testcase here, as well as in bug 821490, which is essentially the same problem reproduced in a slightly different way.

Comment 10

[Jonathan Kew [:jfkthame]]

Tryserver build (in progress): https://tbpl.mozilla.org/?tree=Try&rev=d8ddf14b0e2b

Comment 11

[chrille_b]

I can confirm that this issue is fixed with your latest tryserver build. 
Also bug 821490 is fixed which is a duplicate of this bug.

Bug 822266 is not fixed with this build. But I think the solution to bug 822266 is very similar to this bug. The problem with bug 822266 seems to be font-sizes are calculated on the wrong window scaling. But this time it is in the page-render-window-box and not outside (or not in chrome) like on this bug.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 707715 [details] [diff] [review]
fix hi-/lo-dpi scaling in window opened from script with explicit size on secondary display

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

::: view/src/nsView.cpp
@@ +921,5 @@
>      mViewManager->GetDeviceContext(*getter_AddRefs(devContext));
>      int32_t p2a = devContext->AppUnitsPerDevPixel();
> +    if (devContext->CheckDPIChange()) {
> +      NS_WARNING("nsView::WindowResized - DPI changed but dev context was not yet updated");
> +      p2a = devContext->AppUnitsPerDevPixel();

Why not just call CheckDPIChange before initializing p2a?

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> >      mViewManager->GetDeviceContext(*getter_AddRefs(devContext));
> >      int32_t p2a = devContext->AppUnitsPerDevPixel();
> > +    if (devContext->CheckDPIChange()) {
> > +      NS_WARNING("nsView::WindowResized - DPI changed but dev context was not yet updated");
> > +      p2a = devContext->AppUnitsPerDevPixel();
> 
> Why not just call CheckDPIChange before initializing p2a?

Sure, we can do it that way. I did it like this while investigating so as to find out when this case occurred, but we can drop that now.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: fix hi-/lo-dpi scaling in window opened from script with explicit size on secondary display

Comment 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/538d00d025c4

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to chrille_b from comment #11)
> I can confirm that this issue is fixed with your latest tryserver build. 
> Also bug 821490 is fixed which is a duplicate of this bug.
> 
> Bug 822266 is not fixed with this build. But I think the solution to bug
> 822266 is very similar to this bug. The problem with bug 822266 seems to be
> font-sizes are calculated on the wrong window scaling. But this time it is
> in the page-render-window-box and not outside (or not in chrome) like on
> this bug.

Thanks for testing. Yes, I'm aware bug 822266 is not yet fixed; although it's clearly a similar issue, dragging a tab to a new/different window ends up hitting somewhat different code paths.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/538d00d025c4

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 708038 [details] [diff] [review]
fix hi-/lo-dpi scaling in window opened from script with explicit size on secondary display

Requesting approval to uplift this for FF20, along with the rest of multi-screen HiDPI support. (Not required for FF19 because bug 814434 has preffed-off HiDPI support for systems with mixed hi- and lo-dpi displays)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): HiDPI (Retina) support when external non-hidpi screen is also present

User impact if declined: bad display if a window is opened on a secondary display with an explicitly-specified size, such as some popup windows

Testing completed (on m-c, etc.): on Nightly for the past week

Risk to taking this patch (and alternatives if risky): minimal risk - simple patch with no effect on behavior unless mixed lo- and hi-dpi displays are present

String or UUID changes made by this patch: none

Comment 20

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f87b14a13299