1249279 - Hello (and other XUL panels) sizing and scaling problems when moving window between Retina and non-Retina displays

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Sevaan Franks [:sevaan]]

Attachment: Hello display errors

The Hello panel and conversation window have display errors when moving window between retina and non-retina displays.

Comment 1

[Jonathan Kew [:jfkthame]]

With the introduction of per-monitor dpi support on windows (bug 890156), similar issues occur there.

It looks like there may be dimensions (in device pixels) getting cached by the Hello panel that are no longer valid after the resolution changes, leading to the bad display. Or at least I think that may be part of the problem, if not the whole story.

Comment 2

[Jonathan Kew [:jfkthame]]

Changing platform to All, as it looks like this will affect any platform where resolution is non-constant.

Comment 3

[Mark Banner (:standard8)]

Jonathan, afaik the panel is loaded & cached when we first open it. The panel has a browser as a child element, which loads a page and caches it for the lifetime of the app.

The code is here:

http://mxr.mozilla.org/mozilla-central/source/browser/modules/PanelFrame.jsm

Do you know what we should be doing to hook up some sort of resolution change handling here, or is this likely to be a backend issue?

Comment 4

[Jonathan Kew [:jfkthame]]

I'm not sure offhand, would need to do some digging.

One thing to note is that the Pocket panel (which seems like it's a similar UI element, at least on the surface) does not appear to suffer from this issue. So what's different about how these are implemented?

If that doesn't provide any clues, re-ping me and I'll try to look a bit deeper.

Comment 5

[Mark Banner (:standard8)]

I think the pocket code is written slightly differently, though I can see similar effects on the sharing panel. So there seems to be a common issue here.

On the Hello panel, we're setting the width/height explicitly on the element via js, which don't get updated during the transition - the sizes are based on the sizes of the content within the browser.

However, the share panel doesn't seem to have the width/height set and still gets affected.

Maybe the share panel is different?

I'm also starting to wonder if there's some recalculation needed, but if we're going to be able to do it after the content has resized, if I'm understanding this correctly.

Also pinging Shane who may have come across this previously.

Comment 6

[Shane Caraveo (:mixedpuppy)]

The only thing the share panel and hello would have in common would be that both have a browser in the panel.  I thought they might share some css at the xul level, but it doesn't look like they do.  Hello uses the PanelFrame module but share does not.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Mark, per-monitor DPI support is a new feature shipping in Fx47. This bug seems really bad (based on the screenshot). Do we have a better understanding of the root cause and a potential fix to uplift in Aurora? Thanks!

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

New regression in Fx47, tracked.

Comment 9

[Mark Banner (:standard8)]

Sorry, I don't have a better understanding here. Maybe Jonathon or Gijs can help us a bit more as I'm not sure where to start looking.

Comment 10

[Jonathan Kew [:jfkthame]]

Strictly speaking, I don't think this is new in Fx47, as it affects not only Windows with mixed-DPI monitors, but also OS X with mixed retina and non-retina screens, which we've supported for a long time.

But it's true that the introduction of per-monitor DPI support on Windows 8.1/10 will make it more widespread.

Comment 11

[Jonathan Kew [:jfkthame]]

It seems like something isn't being updated properly when the panel has already been created & displayed, and is then moved to a screen with a different resolution. Possibly we're failing to propagate the change notification (e.g. see nsPresShell::BackingScaleFactorChanged()) into the html document that's embedded within the xul panel, or something vaguely like that?

A possible workaround here, if we have trouble identifying a better fix, would be to tear down the panel altogether when the resolution has changed, so that it then gets re-created from scratch: AFAICS, it displays fine on either lo- or hi-dpi screens, provided it is being opened for the first time.

Comment 12

[:Gijs (he/him)]

Is the browser cached outside of the document when the panel is torn down? Does it ever live in the hidden window?

Comment 13

[Mark Banner (:standard8)]

(In reply to :Gijs Kruitbosch from comment #12)
> Is the browser cached outside of the document when the panel is torn down?

We never tear the panel down. The element is always there once created, its just hidden. Social API works the same way.

> Does it ever live in the hidden window?

I don't believe so.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Attempt to correct for possibly-stale AppUnitsToDevPixels value in nsDocumentViewer::SetBounds

Try run in progress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04beba6d9378

Comment 15

[Jonathan Kew [:jfkthame]]

I think I've made some progress trying to track this down... specifically, things go awry when nsDocumentViewer::SetBounds calls mViewManager->SetWindowDimensions here:

https://dxr.mozilla.org/mozilla-central/rev/17a0ded9bb99c05c25729c306b91771483109067/layout/base/nsDocumentViewer.cpp#1914-1915

What I think is happening when the window is moved between displays is something along these lines. We're using the size from nsSubDocumentFrame::GetSubdocumentSize, which computes it as a ScreenIntSize (i.e. device pixels) based on the resolution (AppUnitsToDevPixels value) of the presContext, which is connected to the new display; but here in nsDocumentViewer, we have a presContext that is still connected to the old display where the panel was previously displayed. So the AppUnitsToDevPixels value we get here is wrong, and our attempt to convert the ScreenIntSize dimensions back to appUnits for SetWindowDimensions results in a mis-scaled view.

I've got a patch that attempts to fix this by retrieving a proper value for p2a here, and this seems to give me stable, correct behavior with a combination of retina and non-retina displays: I can move the browser window back and forth, open the Hello panel on either display, and it is always sized and rendered properly.

An alternative approach, I think, might be to change nsSubDocumentFrame::GetSubdocumentSize such that it returns the size in (unscaled?) CSS pixels rather than (display-dependent) device pixels, and pass this through to mViewManager->SetWindowDimensions etc. In principle, using unscaled CSS pixels rather than device pixels to manage widget & view dimensions would probably make it easier to deal with mixed-resolution scenarios -- but I suspect this would lead to an awful lot of rabbit-holes that need to be followed up, making for a big, invasive and risky patch overall.

However, I don't really understand much about all this code ... :bz, can you offer any insight & advice? Is the localized fix (see current patch attached) within nsDocumentViewer::SetBounds a possible way forward, or is this liable to break all kinds of other things?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8738546 [details] [diff] [review]
Attempt to correct for possibly-stale AppUnitsToDevPixels value in nsDocumentViewer::SetBounds

> change nsSubDocumentFrame::GetSubdocumentSize such that it returns the size in (unscaled?) CSS pixels

Then you'd have to do conversion to device pixels in the "mWindow && !mAttachedToParent" branch, right?  I guess we could try to change what Resize() on an nsIWidget means, which I agree sounds like a large change.

I'd really like to understand how we get into a state where mPresContext->AppUnitsPerDevPixel() and rootPC->AppUnitsPerDevPixel() return different values, per IRC discussion.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: stack in nsDocumentViewer::SetBounds where we have mismatched AppUnitsPerDevPixel values while re-opening the Hello panel after moving the browser window from retina to non-retina screen

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: stack where mPresContext->mDeviceContext->mAppUnitsPerDevPixel subsequently gets updated

Comment 19

[Jonathan Kew [:jfkthame]]

Herewith, stacks as requested.

Also, just to confirm: at the point in nsDocumentViewer::SetBounds shown in the first stack, if I go to frame 4 (nsSubDocumentFrame::ReflowFinished) and check this->PresContext()->AppUnitsPerDevPixel(), I get 60 (as expected), confirming that the subdocument "knows" it has moved to the low-dpi screen. Seems like it's only the DocumentViewer's presContext that is lagging behind, still using the previous screen's dpi.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Thanks.  So the second stack shows us coming off a runnable that got posted in nsPresContext::UIResolutionChanged, right?  And then we set the new resolution on a prescontext in frame 6 ((this=0x0000000123bdf000) and then we walk its document's subdocuments and set the new resolution on the prescontext we actually care about in frame 3 (this=0x0000000120e37000).

One obvious question is whether the prescontext in frame 3 is the same as the relevant document viewer's mPresContext (I assume it is) and if so whether the prescontext in frame 6 is rootPC.

Or put another way, how did we manage to update the resolution on rootPC without then updating it on all descendants via the mDocument->EnumerateSubDocuments() call in nsPresContext::UIResolutionChangedInternal?  Is it possible that at the point when that happened aDocument->GetShell() ended up returning null or something?

Though if that _did_ happen I'd expect that the device context would get recreated when we went to create the presshell, so would pick up the current dpi setting....

Unless maybe that document was in bfcache when the notification came through?  Does the hello panel navigate away from things when hiding and then go back to them via bfcache when it gets shown?

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to Boris Zbarsky [:bz] from comment #20)
> One obvious question is whether the prescontext in frame 3 is the same as
> the relevant document viewer's mPresContext (I assume it is) and if so
> whether the prescontext in frame 6 is rootPC.

Yes, and yes; I re-ran this and checked that the prescontext pointers match.

Comment 22

[Boris Zbarsky [:bzbarsky]]

OK.  So we should probably figure out why when rootPC got updated it didn't update the subframe there.

Actually, one other thing worth checking is that bfcache hypothesis.  Do we land in nsDocumentViewer::Open with an mPresContext whose AppUnitsPerDevPixel() does not match that of our parent prescontext, if we have one?  Right around the SyncParentSubDocMap() call there...  If so, that would be the place to update our mPresContext to have the right AppUnitsPerDevPixel(), I would think.

Comment 23

[Jonathan Kew [:jfkthame]]

(In reply to Boris Zbarsky [:bz] from comment #22)
> OK.  So we should probably figure out why when rootPC got updated it didn't
> update the subframe there.

I think I'm starting to understand this...

At the point when the rootPC's resolution gets updated, we do visit the subdocument as expected, and "refresh" its prescontext's dpi as well... but the problem is that these two prescontexts are connected to two different top-level widgets, which have separate nsDeviceContexts, and at this point the panel's (non-visible) window has not yet been moved to the new screen. So its nsDeviceContext stays at the old resolution.

The subframe's prescontext will only get its resolution updated when its widget actually gets moved and displayed on the new monitor. And that's too late, because we've already used its (stale) values here, and the panel code has recorded the resulting incorrect dimensions in CSS properties.... :(

Comment 24

[Jonathan Kew [:jfkthame]]

(And so I don't think the bfcache issue is our problem here.)

Comment 25

[Boris Zbarsky [:bzbarsky]]

Aha!  Is the entire callstack to SetBounds before the panel's widget has been moved to the new screen?

Comment 26

[Jonathan Kew [:jfkthame]]

Yes, it seems so -- examining the widget's mBounds, I see coordinates that are clearly still on the old screen.

Comment 27

[Jonathan Kew [:jfkthame]]

Would it be valid to assume that subdocuments should always appear on the same screen as their parent document? (I'm not sure what situations we use them for, besides panels like this...) If so, perhaps we could pass the desired resolution down to the subdocuments when the root gets updated, instead of letting each of them retrieve its own.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Subdocuments are used for <frame> and <iframe> most obviously.  Those are _generally_ same-screen as the parent document.  This case of panels seems like the only situation in which that would not be the case, I'd think; the other separate widgets we create (tooltips, combobox dropdowns) don't contain child documents.

Comment 29

[Jonathan Kew [:jfkthame]]

Attachment: Let subdocuments' presContexts inherit the DPI setting of their parent, instead of retrieving it from their widget, to avoid using stale values from a currently-hidden widget on a screen with a different DPI

OK, here's an alternative fix based on inheriting the DPI value from the root presContext to the subdocuments, instead of letting them retrieve their own potentially-state values. Seems to work nicely in initial testing, but I'll want to try it on mixed-DPI Windows as well... if it looks like a feasible approach, I'll also add some comments to help future readers understand what's going on here.

Comment 30

[Jonathan Kew [:jfkthame]]

er... s/potentially-state/potentially-stale/, of course

Comment 31

[Jonathan Kew [:jfkthame]]

Attachment: Let subdocuments' presContexts inherit the DPI setting of their parent, instead of retrieving it from their widget, to avoid using stale values from a currently-hidden widget on a screen with a different DPI

Testing on Windows looks good, too. I think this is a much better patch than the first approach, which always felt like an ugly workaround for the real problem. Try run in progress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7702e2b00b4e.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8738713 [details] [diff] [review]
Let subdocuments' presContexts inherit the DPI setting of their parent, instead of retrieving it from their widget, to avoid using stale values from a currently-hidden widget on a screen with a different DPI

>+     * @param  aScale - if non-null, the CSS to device pixel scale factor

Should probable document whether this is unzoomed CSS pixels or zoomed ones or what...

>+      double scale = *reinterpret_cast<double*>(aData);

I believe this can be a static_cast.

r=me.  Thank you for sorting through this!

Comment 33

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5fe9e219441413c6fc8429050ca19c910b7c62a
Bug 1249279 - Let subdocuments' presContexts inherit the DPI setting of their parent, instead of retrieving it from their widget, to avoid using stale values from a currently-hidden widget on a screen with a different DPI. r=bz

Comment 34

[Jonathan Kew [:jfkthame]]

FTR, this affects the Social Share panel (airplane button) as well, not only the Hello panel.

Comment 35

[Jonathan Kew [:jfkthame]]

Comment on attachment 8738713 [details] [diff] [review]
Let subdocuments' presContexts inherit the DPI setting of their parent, instead of retrieving it from their widget, to avoid using stale values from a currently-hidden widget on a screen with a different DPI

Approval Request Comment
[Feature/regressing bug #]: 890156 (per-monitor DPI); not a regression as such -- already-existing issue on OS X, but also appears on Windows once per-monitor DPI is enabled

[User impact if declined]: the Hello panel (and possibly other panels) may be mis-sized/mangled if the browser window is moved between screens with different resolution after the panel has previously been opened

[Describe test coverage new/current, TreeHerder]: tested manually on both OS X and Windows (requires mixed-DPI multiscreen setup)

[Risks and why]: pretty low -- this only touches code that handles dynamic resolution-change events, so should not affect systems without mixed-DPI monitors

[String/UUID change made/needed]: none



I think (fingers crossed!) this may be the last serious glitch we need to fix for per-monitor dpi support on 47. :)

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f5fe9e219441

Comment 37

[Mark Banner (:standard8)]

Thank you Jonathan, I've only tried it briefly, but looks good on Mac :-)

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Sevaan, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 39

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8738713 [details] [diff] [review]
Let subdocuments' presContexts inherit the DPI setting of their parent, instead of retrieving it from their widget, to avoid using stale values from a currently-hidden widget on a screen with a different DPI

per-monitor DPI feature is planned for Fx47, this fix was manually verified, let's uplift to Aurora47

Comment 40

[Sevaan Franks [:sevaan]]

(In reply to Ritu Kothari (:ritu) from comment #38)
> Sevaan, could you please verify this issue is fixed as expected on a latest
> Nightly build? Thanks!

Verified!

Comment 41

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1cff2bbb09ff