Closed Bug 829963 Opened 7 years ago Closed 7 years ago

incorrect display of popup window on external display of retina mac

Categories

(Core :: Widget: Cocoa, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- disabled
firefox19 --- disabled
firefox20 + fixed
firefox21 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

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."
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
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.
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.
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.
Attached file 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.
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.
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.
This shows the result of the above patch: window content looks fine in the new window, but the chrome is a mess.
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.
Attachment #707715 - Flags: review?(roc)
Attachment #707138 - Attachment is obsolete: true
Tryserver build (in progress): https://tbpl.mozilla.org/?tree=Try&rev=d8ddf14b0e2b
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 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?
(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.
Attachment #707715 - Attachment is obsolete: true
Attachment #707715 - Flags: review?(roc)
https://hg.mozilla.org/integration/mozilla-inbound/rev/538d00d025c4
Assignee: nobody → jfkthame
Target Milestone: --- → mozilla21
Duplicate of this bug: 821490
(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.
https://hg.mozilla.org/mozilla-central/rev/538d00d025c4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Attachment #708038 - Flags: approval-mozilla-aurora?
Attachment #708038 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.