852775 - social panels sized incorrectly on hidpi displays

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: screenshot showing the clipped panel

Since bug 844604 landed, social panels are sized incorrectly on Windows with hidpi settings.

Attaching a screen-shot showing this.  If you set the preference layout.css.devPixelsPerPx to 1.0 (the default before bug 844604) things work as expected.

Comment 1

[Jonathan Kew [:jfkthame]]

Hmm, I'm not seeing this problem with the current (2013-03-19) Nightly. What build are you on? Any addons present?

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to Jonathan Kew (:jfkthame) from comment #1)
> Hmm, I'm not seeing this problem with the current (2013-03-19) Nightly. What
> build are you on? Any addons present?

I'm on a locally built nightly - a pull and build from a few hours ago [22.0a1 (2013-03-21)] has the same problem.  DOM Inspector is the only addon and it still happens when that is disabled.

And here it gets weird!  I instrumented our panel resize code, and in both cases the exact same height and width is being requested.  Note however that we adjust the height and width by setting iframe.style.width and iframe.style.height, rather than adjusting the panel itself (and obviously, 'iframe' is the iframe inside the panel)

Poking around with the DOM Inspector, I noticed the panel itself got 'width' and 'height' attributes - but they are never set by the social code (neither explicitly, nor implicitly via sizeTo()) - and the height is too small, reflecting what I am seeing.  Removing that attribute via the inspector solved the problem.

So then I tried again with layout.css.devPixelsPerPx set to 1.0 - in that case, the panel never gets height and width attributes!  IOW, these mystery attributes only appear when layout.css.devPixelsPerPx is set to the default, not when explicitly set to 1.0.

To be even more explicit, I instrumented browser-social.js with:

diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js
--- a/browser/base/content/browser-social.js
+++ b/browser/base/content/browser-social.js
@@ -397,18 +397,20 @@ function sizeSocialPanelToContent(panel,
   let wDiff = width - iframe.getBoundingClientRect().width;
   // A panel resize will move the right margin - if that is where the anchor
   // arrow is, the arrow will be mis-aligned from the anchor.  So we move the
   // popup to compensate for that.  See bug 799014.
   if (wDiff !== 0 && panel.getAttribute("side") == "right") {
     let box = panel.boxObject;
     panel.moveTo(box.screenX - wDiff, box.screenY);
   }
+  dump("before iframe resize has " + panel.getAttribute("height") + "\n");
   iframe.style.height = height + "px";
   iframe.style.width = width + "px";
+  dump("Panel resized to " + width + "x" + height + "\n");
 }

And running with layout.css.devPixelsPerPx set to the default of "-1.0" yields:

before iframe resize has 114
Panel resized to 330x284

But setting layout.css.devPixelsPerPx to "1.0" and restarting (but no other changes) yields:

before iframe resize has
Panel resized to 330x285

(ie, there is no 'height' attribute on the panel in the second example.)

I can't see anything in the panel code that could account for this and I'm a little stumped, but I'll try and dig a little further later today or tomorrow...

Comment 3

[Mark Hammond [:markh] [:mhammond]]

In the comment above, note the 1px difference in the resize height - it turns out this is significant (and possibly why Jonathan can't see this - the specific size of the panel for him might be such that no "off-by-one" condition exists.)  

What happens is that we are hitting:

http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsXULPopupManager.cpp#402

We hit this because there is a 1px difference after all the conversions to css pixels, so the code adds height and width attributes.  Once the panel has these attributes, it appears to stop "flexing" - the size becomes fixed.  This is also evident in bug 822150, which is reporting very similar symptoms (these attributes appearing then the panel stops resizing)

So the question here seems to be: why does this code set these attributes when they don't already exist?  I've made a quick patch that checks popup->HasAttr() before attempting to set them and this solves the problem for me.  I bet it would also solve bug 822150.

Requesting some insights from enn on this.  Enn, is there any reason those code unconditionally sets the attributes, or would it be reasonable to only set them if the attribute already exists?

Comment 4

[Neil Deakin]

It sets the attributes in case the popup is resizable and someone uses the borders to resize the popup. In this case the attributes don't yet exist. There's another bug already on this issue, which I can't find. (maybe 814565 is related)  The fix is to be able to distinguish between popups that are resized by the system and those resized by us.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Neil Deakin from comment #4)
> It sets the attributes in case the popup is resizable...

So I think I'm conflating 2 different issues here.  For now, let's ignore the issue with 'width' and 'height' attributes - the real question is why nsXULPopupManager::PopupResized thinks the size has changed when, in this case, it hasn't.

I've determined that the system isn't doing any kind of sizing.  We ask Windows to size the widget (nsWindow::Resize) and later nsXULPopupManager::PopupResized() is called with exactly the same size.  IOW, Windows isn't resizing anything, the window is exactly the size we earlier requested it to be.  In both cases the app pixels are also identical - so the problem seems to be that PopupResized() is calculating a slightly different number of device pixels than nsView::CalcWidgetBounds() (which, IIUC, is what calculates the size that nsWindow::Resize() uses).  I *think* this discrepancy is due to CalcWidgetBounds() calling nsRect::ScaleToNearestPixels(), which first rounds the start position of the window then uses that as the basis for the width, whereas PopupResized() only considers the width.

To demonstrate this, I added the following code to nsXULPopupManager::PopupResized:

  nsView* view = menuPopupFrame->GetView();
  nsIntRect wtf = view->CalcWidgetBounds(eWindowType_popup);

and I find that 'wtf' has exactly the same number of pixels as passed to the function.  IOW, if we could use view->CalcWidgetBounds() instead of the explicit conversion to css pixels, the problem wouldn't exist as the size is identical.

Neil, is that an option?

Comment 6

[Neil Deakin]

> I *think* this discrepancy is due to CalcWidgetBounds() calling
> nsRect::ScaleToNearestPixels(), which first rounds the start position
> of the window then uses that as the basis for the width, whereas PopupResized()
> only considers the width.

Without looking at the code, it sounds like a reasonable guess, but Timothy is probably a better person to answer questions about the widget pixel conversions.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Neil Deakin from comment #4)
> The fix is to be able to distinguish between popups that are
> resized by the system and those resized by us.

I think this is the main problem. The width/height checks in nsXULPopupManager::PopupResized are a poor attempt at doing this, but as you've discovered they are faulty.

(In reply to Mark Hammond (:markh) from comment #5)
> and I find that 'wtf' has exactly the same number of pixels as passed to the
> function.  IOW, if we could use view->CalcWidgetBounds() instead of the
> explicit conversion to css pixels, the problem wouldn't exist as the size is
> identical.

I think comparing the bounds with what we expect them to be from CalcWidgetBounds before doing this width/height stuff is a good idea. Do you want to patch or shall I?

Comment 8

[Jonathan Kew [:jfkthame]]

FWIW, this reminds me of one of the issues that showed up during the initial Mac HiDPI support - the "discrepancy" in the popup size causing persistent width/height attributes to be set on it, when we really don't want them. I presume the origin of the problem is the rounding error that may accumulate when converting back and forth between device and css pixels.

In bug 674373 part 6, we patched nsXULPopupManager::PopupResized to do the comparison in integer CSS pixels, so as to effectively ignore a potential 0.5px discrepancy (1 dev px on a retina display), and that seemed to be sufficient to avoid the issue. But with the variety of different scale factors on Windows, not just the 2:1 ratio we have on OS X, this clearly isn't enough - the proposed approach here sounds like it'll be better.

Comment 9

[Timothy Nikkel (:tnikkel)]

Oh, okay. Thanks for the context. I had assumed that code in PopupResized was "old code", but it's good to know that it was for the initial landing of HiDPI support.

Comment 10

[Jonathan Kew [:jfkthame]]

Well - the fact that PopupResized may set the attributes was indeed "old code", but the comparison was moved from device-px to css-px coordinates to work around a possible single-dev-pix (0.5 css-px) discrepancy.

I suspect we should really switch from integer to floating-point variables for a lot of css-px values, as widget sizes are constrained to integer -device- pixels, which may not necessarily be integers when converted to css pixels. So we accumulate errors when they are converted back and forth using integers on both sides.

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> Well - the fact that PopupResized may set the attributes was indeed "old
> code", but the comparison was moved from device-px to css-px coordinates to
> work around a possible single-dev-pix (0.5 css-px) discrepancy.

Yeah.

> I suspect we should really switch from integer to floating-point variables
> for a lot of css-px values, as widget sizes are constrained to integer
> -device- pixels, which may not necessarily be integers when converted to css
> pixels. So we accumulate errors when they are converted back and forth using
> integers on both sides.

That makes sense. device pixels are potentially the only thing that should be int.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Timothy Nikkel (:tn) from comment #7)
> I think comparing the bounds with what we expect them to be from
> CalcWidgetBounds before doing this width/height stuff is a good idea. Do you
> want to patch or shall I?

Thanks for the updates - I'll make a patch early next week and flag you for review.

(In reply to Jonathan Kew (:jfkthame) from comment #10)

> I suspect we should really switch from integer to floating-point variables
> for a lot of css-px values, as widget sizes are constrained to integer
> -device- pixels, which may not necessarily be integers when converted to css
> pixels. So we accumulate errors when they are converted back and forth using
> integers on both sides.

I haven't verified this, but I think the specific problem here is that one approach uses nsRect::ScaleToNearestPixels() while the other does not.  ScaleToNearestPixels first seems to round the start positions, then applies the size and rounds that.  If the rounding of the start position calculates a device pixel which is a few CSS pixels different than actually specified, it makes sense that the resulting end device pixel will be different too.  IOW, considering only the width seems to make the assumption that the start position falls exactly on a specific device pixel, but this isn't the case.

Regardless, I'm sure you are correct that in many cases floating point should be used where ints are currently, but in this case it makes sense that the same routine which is responsible for calculating the device pixels to place a window is also used when comparing device pixels.  So I'll make a patch for this and leave the general float-vs-int issue in the capable hands of you guys :)

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Attachment: Use result of CalcWidgetBounds to determine if popup size changed

This patch uses CalcWidgetBounds() to check if the size changed, and if so, still converts to CSS pixels and stores the new values as 'width' and 'height' attributes.

The only thing I'm not sure about is whether the check for menuPopupFrame->GetView() returning null is necessary (eg, the existing code assumes menuPopupFrame->GetContent() will never return null, so maybe this should too?)

try at https://tbpl.mozilla.org/?tree=Try&rev=c7f991657ab5

Comment 15

[Timothy Nikkel (:tnikkel)]

Comment on attachment 728849 [details] [diff] [review]
Use result of CalcWidgetBounds to determine if popup size changed

Sorry for the delay.

Comment 16

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3253b8b2b75e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3253b8b2b75e