If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

SizeToContent generates incorrect result on some responsive design configurations

UNCONFIRMED
Unassigned

Status

()

Core
Layout
UNCONFIRMED
3 years ago
3 years ago

People

(Reporter: Steve Williams, Unassigned)

Tracking

Trunk
x86_64
Windows 8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Component: Untriaged → Layout
Product: Firefox → Core
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?
(Reporter)

Comment 2

3 years ago
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 ?
(Reporter)

Comment 3

3 years ago
that didn't answer your question. will ensure page is maxed out before the query as you imply I should.
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.
(Reporter)

Comment 5

3 years ago
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
(Reporter)

Comment 6

3 years ago
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.
(Reporter)

Comment 7

3 years ago
need to update my tree to confirm this. might be something weird at my end.
(Reporter)

Comment 8

3 years ago
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.
(Reporter)

Comment 9

3 years ago
Created attachment 8585673 [details]
winembed gecko directory (xulrunner) configuration factor
(Reporter)

Comment 10

3 years ago
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.
(Reporter)

Comment 11

3 years ago
& 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.
(Reporter)

Comment 12

3 years ago
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.
(Reporter)

Updated

3 years ago
Version: 37 Branch → Trunk
(Reporter)

Comment 13

3 years ago
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.
(Reporter)

Comment 14

3 years ago
@David - thanks for the specification work in progress. For now, looking at a solution on current web content.
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.
(Reporter)

Comment 16

3 years ago
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.
You need to log in before you can comment on or make changes to this bug.