1149132 - SizeToContent generates incorrect result on some responsive design configurations

Status: UNCONFIRMED

(Core :: Layout, defect)

Description

[Steve Williams]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150326190726

Steps to reproduce:

Modified winembed so it performs SizeToContent following a page load.

Loaded : http://advance-software.com


Actual results:

Page sized to minimal responsive design configuration (with a horizontal scroll bar)


Expected results:

The page should have sized so that no horizontal scroll bar was present.

Comment 1

[Boris Zbarsky [:bzbarsky]]

SizeToContent does the following:

1) Ask the page for its preferred width (given whatever the current set of styles is, so
   this will depend on the width it is right this second in cases involving media queries).
2) Perform a layout with the viewport sized at that preferred width.
3) Return the resulting visible area size.

So what viewport size did you start at and what is the preferred width of the page at that viewport size?

Comment 2

[Steve Williams]

Hi Boris, yes, it's GetPrefWidth that's returning an incorrect result. We went through this yesterday, partial transcript :

roc	so, you can call GetMinISize and GetPrefISize on the root frame
roc	GetMinISize should be the minimum width that doesn't show a horizontal scrollbar
sw	oh really you rock, roc
roc	I'm not convinced that's the width you want
roc	e.g. if your document is just text, the minisize will just be the width of the longest word
roc	and the rendering will look a bit silly
sw	right.
roc	I guess you can get the min-ISize and the pref-ISize and pick a width between those two
roc	dunno if you care about writing-mode:vertical-rl documents but you might want to think about that
sw	as long as common sites/pages render optimally, can consider special cases later.  some thing like if size < some threshold, this page is fluid so select a sensible default.
sw	addresses the text example you gave above.
sw	sometimes a vertical banner ad for example might be desired so got to be careful with second guessing what the user wants.
sw	nope, GetMinISize get 460 pixels on that page. that's wrong.
sw	size the page by hand to see where the scroll bar appears.
sw	GetPrefISize returns the same.


think the issue is asking the page for its preferred width - maybe the page is at fault.

how does the page specify what it wants its preferred with to be ?

Comment 3

[Steve Williams]

that didn't answer your question. will ensure page is maxed out before the query as you imply I should.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The page specifies through the layout it uses; intrinsic sizes are determined based on various CSS properties.  Some work-in-progress on specifying how is at http://dev.w3.org/csswg/css-sizing/ , although the spec isn't particularly solid yet.

Comment 5

[Steve Williams]

so ... winembed by default starts quite small. changed that to 1200x1000 start size.

result still wrong, though no longer selects the minimal responsive configuration.

might be just a minor calc error so will try adding a fudge factor to  treeOwner->SizeShellTo at the end of nsGlobalWindow::SizeToContent

Comment 6

[Steve Williams]

have to add a fudge factor of around 40 device pixels to the width on that to get it to select the high end responsive design without a horizontal scroll bar correctly. that's too much to be a rounding error.

Comment 7

[Steve Williams]

need to update my tree to confirm this. might be something weird at my end.

Comment 8

[Steve Williams]

this is going to be a pain. winEmbed doesn't compile/run in current mozilla tree, so you're going to need a few of my patches so you can reproduce.


1. [mozilla]\gfx\layers\client\ClientLayerManager.cpp

void
ClientLayerManager::ForwardTransaction(bool aScheduleComposite)
{
   // ADV_SW_PATCH: mTransactionIdAllocator null guard required for winembed.
   if (!mTransactionIdAllocator)
      return;

2. [mozilla]\embedding\tests\winEmbed\winEmbed.cpp

#include <tchar.h>

// ADV_SW_PATCH: fix to compile with vs2013
#include "mozilla\Char16.h"

3. [mozilla]\embedding\tests\winEmbed\WebBrowserChrome.cpp

 * ***** END LICENSE BLOCK ***** */

// ADV_SW_PATCH: fix to compile with vs2013
#include "mozilla\Char16.h"


4. 2. [mozilla]\embedding\tests\winEmbed\winEmbed.cpp

rv = AppCallbacks::CreateBrowserWindow(nsIWebBrowserChrome::CHROME_ALL,
           nullptr, getter_AddRefs(chrome));

// ADV_SW_TEST - required for SizeToContent testing
chrome->SizeBrowserTo(1200, 1000);


... then load google.com . not weird disabled vertical scroll bar that shouldn't be there.


I also have it setup to run without xulrunner directory & have factored that configuration to make it easier to switch. My version of winEmbed.cpp attached - would be nice to get that factor in so its one less merge & you guys don't run this right now anyway.

Suppose I'll have to switch to firefox to nail this as you won't want to run winEmbed unless you have to, which you probably don't.

Comment 9

[Steve Williams]

Attachment: winembed gecko directory (xulrunner) configuration factor

Comment 10

[Steve Williams]

tried firefox -width 1200 -height 1000 http://google.com & no weird disabled vertical scroll bar so maybe winembed specific, though perhaps its just a browser size/resize difference. aggghhh.

suggest we get back to the point where others can compile/run winEmbed. there's not much broken. then go from there. I'm testing vanilla ff37b7 with minimal patching so winEmbed runs right now &can switch back to trunk if this progresses.

none of this blocks ff37.

Comment 11

[Steve Williams]

& vs 2013 winEmbed project file if anyone wants it but its hardwired to pick up build from my directory setup. could probably make that more generic but will be easier to fixup winembed mozbuild stuff.

Comment 12

[Steve Williams]

have reproduced the problem in firefox (37b7).

firefox.exe -width 1200  -height 1000 http://advance-software.com

void nsXULWindow::OnChromeLoaded() performs a SizeToContent if mIntrinsicallySized is true.

hardwired that on for testing here as don't know how to request that through the app yet.

... so others should be able to reproduce the problem without messing about with winembed. we can fix that up later.

please confirm whether you can reproduce this error.

Comment 13

[Steve Williams]

Suggest a new method on nsIDOMWindow

nsresult SizeToContentConstrained(uint32_t max_width, uint32_t max_height);

... which sizes to content, starting from the constraints specified. 

This is better than sizing the chrome at the API then calling sizetocontent as the win32 or whatever API can by bypassed so the operation is cleaner and more efficient.

Comment 14

[Steve Williams]

@David - thanks for the specification work in progress. For now, looking at a solution on current web content.

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

To be clear:  the spec doesn't define any new API surface, that is, it doesn't add any new features that would need to be used by Web content.  It's just trying to explain how existing poorly-specified features work.

Comment 16

[Steve Williams]

Thanks. 

Should we close this down & reopen a clean bug report ? 

Got winembed stuff mixed up with a SizeToContent bug report & the two things are separate issues. 

The SizeToContent bug can be reproduced in firefox as explained in comment #12 - via a minimal hardwire or whatever you have to do to fire mIntrinsicallySized.