602580 - After using setCSSViewport, window.innerWidth/innerHeight should return the viewport size

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Brubeck (:mbrubeck)]

Fennec uses nsIDOMWindowUtils.setCSSViewport to explicitly set the "virtual" window size for page layout.  But the window.innerWidth DOM property still returns the size of the browser widget.  Instead, it should return the size of the viewport in content-document CSS pixels.

For example http://www.quirksmode.org/m/tests/widthtest_vp380.html reports window.innerWidth=480 in Fennec if the screen width is 480 hardware pixels, while it should report window.innerWidth=380.

This breaks sites that resize images to fit the viewport, like Google Reader.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch?

Use the values in presContext to determine innerWidth/Height rather than the values in DocShell. Since I don't know much about this code, just asking for feedback for this first run. Happy to hear if there is a better way to tackle the problem.

Comment 2

[Timothy Nikkel (:tnikkel)]

Comment on attachment 489327 [details] [diff] [review]
Patch?

I think the changes to SetInner* cause the size stored on the docshell to no longer be updated even in cases where we don't use SetCSSViewport, which I don't think is what we want.

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 489327 [details] [diff] [review]
Patch?

In fact, I think the SetInner* change will mean that setting the innerHeight/Width will not change the window size at all (in cases where it is allowed).

Comment 4

[Matt Brubeck (:mbrubeck)]

Assigning this to Wes since he has started on it and I haven't.

Comment 5

[Timothy Nikkel (:tnikkel)]

Hmm, what behaviour do we want if a site uses the meta viewport tag and sets the innerWidth/Height anyway?

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 489327 [details] [diff] [review]
Patch?

Maybe we only use this behavior if the viewport metatag is specified?

Asking for feedback about how to proceed from roc and dbaron.

Comment 7

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

(In reply to comment #5)
> Hmm, what behaviour do we want if a site uses the meta viewport tag and sets
> the innerWidth/Height anyway?

Isn't the idea of the viewport API that we're rendering the page as though it's in a viewport of the given size?  If so, shouldn't that be the size we expose to the page through existing APIs?

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Fix v2

Check for viewports. I'm using the mViewportOverridden property of nsPresShell to determine if we've set the viewport.

innerWidth and innerHeight always return the viewport though. I think (tell me if I'm wrong) that the docshell size and css-viewport should be the same unless that has been set.

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 491914 [details] [diff] [review]
Fix v2

> nsGlobalWindow::SetInnerWidth(PRInt32 aInnerWidth)
>+    height = shellArea.height;

This height is in app units.

>+    docShellAsWin->GetSize(&notused, &height);

This path sets height in dev pixels, but the call to SetInnerWidthAndHeight expects CSS pixels. Same comment for width later on.

In fact I think you should either pass float CSS pixels or app units to SetInnerWidthAndHeight because converting from int dev pixels to int CSS pixels and then back could give you two different numbers.

> nsGlobalWindow::SetInnerHeight(PRInt32 aInnerHeight)
>-  NS_ENSURE_STATE(mDocShell);

I don't think you want to lose this line.

>+nsGlobalWindow::SetInnerWidthAndHeight(PRInt32 aInnerWidth, PRInt32 aInnerHeight)
>+  if (false) //presShell->GetIsViewportOverridden())

You'll need to fix this.

The two calls to SetInnerWidthAndHeight pass one parameter in CSS pixels (the value being changed)

>diff --git a/layout/base/nsIPresShell.h b/layout/base/nsIPresShell.h
>+  virtual PRBool GetIsViewportOverridden() = 0;

This would require and IID change on nsIPresShell, but as this point in the release cycle I don't think we can do that. So I think we need to do this without changing the vtable of nsIPresShell.

Comment 10

[Doug Turner (:dougt)]

dbaron, tn, is there a better way to get access to mViewportOverridden.  I'd hate to provide a static method which does the casting from a nsIPresShell to a nsPresShell and returns mViewportOverridden.  I suppose that is better than adding a new interface.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Just unifying the units here. Everything being passed to SetInnerWidthAndHeight should be in IntCSSPixels now. Maybe it is better to refactor that into two different functions... Will keep playing, but wanted to get these nits addressed.

Still need to find a better way to check if a CSSViewport has been set (if we can't change the interface here). Maybe we can try something like checking if the two (CSSViewport and DocShell size) are the same, and use that as an indicator? Happy to hear alternative ideas.

Comment 12

[Timothy Nikkel (:tnikkel)]

The interaction between setting innerHeight/Width in js and using the meta viewport tag should be defined. Which one takes precedence? We don't need to do that here, but there should be a bug filed.

Comment 13

[Timothy Nikkel (:tnikkel)]

As for the IID change, I don't know if we can change the IID or not at this point. But if we can't then we can do what we did for 1.9.2: split off a nsIPresShell_MOZILLA_1_9_2 interface. See http://mxr.mozilla.org/mozilla1.9.2/source/layout/base/nsIPresShell.h#137 . Of course since we haven't branched yet we'll have to remember to revert this change when we do branch.

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 492839 [details] [diff] [review]
Patch v3

I think SetInnerWidthAndHeight should take appunits so we can eliminate the rounding bug of going int dev pixels -> int css pixels -> int dev pixels.

Other than that and comments 12 and 13 this looks good. I'd like to see the updated patch though.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch v4

Revised patch. I haven't heard anything from the release drivers about API changes, so this implements a new interface (nsIPresShell_MOZILLA_2_0_1 ?). I also refactored a bit so that there are SetDocShellWidthAndHeight and SetCSSViewportWidthAndHeight functions instead of one SetInnerWidthAndHeight functions. DocShell function takes in DevPixels and CSSViewport takes in CSSPixels. 

I've tested this somewhat on desktop and fennec with a local test page, have browsed around with it in Fennec for a few days. I am going to push it to try today, but if you have any ideas for some good tests to run I would be happy to hear them.

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 494727 [details] [diff] [review]
Patch v4

+class nsIPresShell_MOZILLA_2_0_1 : public nsISupports {

The prevailing style seems to be to add _MOZILLA_2_0_BRANCH to the name.

Is there a bug somewhere collecting all the instances of things like this that we need to remove once 2.0 actually branches? We should track it somewhere so this ugly code does indeed get removed.

I think you should leave the "!CanMoveResizeWindows() || IsFrame()" and CheckSecurityWidthAndHeight checks in SetInnerWidth/Height because in SetDocShellWidthAndHeight and SetCSSViewportWidthAndHeight the width/height that is passed to CheckSecurityWidthAndHeight is dev pixels or app units, it should be in CSS pixels. And we also want to pass null for the parameter we are not changing because CheckSecurityWidthAndHeight can change the value of what it is passed. Also, if one dimension of the window is already illegal (say if the user made the window really small) and someone is trying to change the other (legal) dimension we shouldn't change the illegal dimension, or at least that would match what we did before.

+  nsresult SetCSSViewportWidthAndHeight(PRInt32 width, PRInt32 height);
+  nsresult SetDocShellWidthAndHeight(PRInt32 width, PRInt32 height);

App units should be nscoord (this applies to the other places that are in app units). Add a comment above each one saying what units their arguments are in.

 nsGlobalWindow::SetInnerWidth(PRInt32 aInnerWidth)
+    nsRefPtr<nsPresContext> presContext;
+    mDocShell->GetPresContext(getter_AddRefs(presContext));

You have the presShell already, just get the prescontext from it, the docshell getter is complicated.

We should probably have a test that when the viewport is set we return the right values for innerHeight/Width, and setting them also works correctly when a viewport is set.

And please file a followup for comment 12.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch v5

I think this addresses most comments. Renamed interface to nsIPresShell_MOZILLA_2_0_BRANCH. Changed units for viewport function to nscoord. Changed accessor for presContext to use presShell.

Working on tests, and filed Bug 617034 to address the "which should take precedence" issue.

Comment 18

[Wesley Johnston (:wesj)]

Oh. I also just changed the Getter function for innerWidth and Height to only use the CSSViewport if one has been set. I am a bit nervous that not doing so would change behavior (for instance, not take into account scrollbars). If there isn't anything to be concerned about, I can revert that change.

Comment 19

[Timothy Nikkel (:tnikkel)]

(In reply to comment #16)
> Is there a bug somewhere collecting all the instances of things like this that
> we need to remove once 2.0 actually branches? We should track it somewhere so
> this ugly code does indeed get removed.

I didn't find one after a quick look so I filed bug 617539 for this.

Comment 20

[Timothy Nikkel (:tnikkel)]

(In reply to comment #18)
> Oh. I also just changed the Getter function for innerWidth and Height to only
> use the CSSViewport if one has been set. I am a bit nervous that not doing so
> would change behavior (for instance, not take into account scrollbars). If
> there isn't anything to be concerned about, I can revert that change.

I think we should be fine either way.

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 495530 [details] [diff] [review]
Patch v5

+  nsresult SetCSSViewportWidthAndHeight(nscoord width, nscoord height);
+  nsresult SetDocShellWidthAndHeight(PRInt32 width, PRInt32 height);

Comment above these saying the units of the arguments please.

 nsGlobalWindow::SetInnerWidth(PRInt32 aInnerWidth)
+  PRInt32 height = 0;
+  PRInt32 width  = 0;

These hold a mix of pixels or appunits depending on which branch of the if we take. Better to declare them inside each branch of the if as their correct type (ie nscoord for appunits). Less chance for confusion that way I think. Same for SetInnerHeight.

Can you write a basic test for this?

Other than that this is pretty much good. And I think this patch should get sr, perhaps from dbaron or roc.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch v6

Fixed up patch. I have a quick test file written, but it currently has problems when run with other mochitests. Working on it...

Comment 23

[Timothy Nikkel (:tnikkel)]

Comment on attachment 496311 [details] [diff] [review]
Patch v6

+  // Arugments to this function should have values scaled to App units
+  nsresult SetCSSViewportWidthAndHeight(nscoord width, nscoord height);
+  // Arugments to this function should have values scaled to device pixels
+  nsresult SetDocShellWidthAndHeight(PRInt32 width, PRInt32 height);

Just say "in" instead of "scaled to". And s/Arugments/Arguments/.

(In reply to comment #21)
>  nsGlobalWindow::SetInnerWidth(PRInt32 aInnerWidth)
> +  PRInt32 height = 0;
> +  PRInt32 width  = 0;
> 
> These hold a mix of pixels or appunits depending on which branch of the if we
> take. Better to declare them inside each branch of the if as their correct type
> (ie nscoord for appunits). Less chance for confusion that way I think. Same for
> SetInnerHeight.

It doesn't look like you addressed this.

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch v7

Whoops. I remember making that change.... My mq must have eaten it somewhere else.

Comment 25

[Timothy Nikkel (:tnikkel)]

Comment on attachment 496339 [details] [diff] [review]
Patch v7

r+ conditional on this getting a test.

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Tests

Hope this is ok. Firefox doesn't honor viewport metatags and Fennec doesn't honor window.open settings, so I set two tests off to not run under Fennec, and two to not run under Firefox (using a really ugly useragent hack).

These pass with both browsers for me and can be updated when/if the behavior of either changes.

Comment 27

[Timothy Nikkel (:tnikkel)]

Comment on attachment 496361 [details] [diff] [review]
Tests

>diff --git a/dom/tests/mochitest/dom-level0/Makefile.in b/dom/tests/mochitest/dom-level0/Makefile.in
>+    test_innerWidthHeight_metaviewport.html \
>+    innerWidthHeight_metaviewport.html \

You should use tabs in the makefile like the other lines do.

>+    if (!navigator.userAgent.match(/Fennec/)) {

This feels wrong. How do we test other mobile browser specific things? Conditional inclusion of tests in the Makefile?

Comment 28

[Wesley Johnston (:wesj)]

Attachment: Tests v2

Moved this into two test, and sets one to only run for mobile.

Comment 29

[Timothy Nikkel (:tnikkel)]

Comment on attachment 496643 [details] [diff] [review]
Tests v2

Looks good. Thanks for going through all the iterations to get this done.

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

dbaron - sr ping

Comment 31

[Wesley Johnston (:wesj)]

Tryserver is showing repeatable reftest failure on Win Debug builds with these patches:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292463367.1292466156.5773.gz

Looking into the errors, but don't see much (anything) that seems related to this stuff... Not sure what's going wrong.

Comment 32

[Timothy Nikkel (:tnikkel)]

Maybe the revision you pushed your patches on top of is causing the failure?

Comment 33

[Timothy Nikkel (:tnikkel)]

Ie, it got backed out or fixed later.

Comment 34

[Wesley Johnston (:wesj)]

I pushed twice today and got the same test failure both times. Here's the earlier push:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292443565.1292447102.7905.gz

None of the other Try server pushes near the same times show the same error either. There seem to be some warnings from DocShell.

Comment 35

[Timothy Nikkel (:tnikkel)]

Can you reproduce the problem on your own machine?

Comment 36

[Wesley Johnston (:wesj)]

I don't have a windows on this machine. I'll bring my windows laptop in tomorrow and see if I can reproduce the error.

Comment 37

[Timothy Nikkel (:tnikkel)]

Does it happen on whatever OS you run?

Comment 38

[Timothy Nikkel (:tnikkel)]

Alternatively you could try wrapping all of your changes in a "displayport is set" check to see if anything changes.

Comment 39

[Wesley Johnston (:wesj)]

Built a debug windows build today, but this test passes on it, as do my Linux builds. Also sent another build to try with rewritten getters. Still waiting on that guy, but it should be done soon.

Comment 40

[Wesley Johnston (:wesj)]

Attachment: Patch V8

Changed to check what it returns during GetInnerWidth/Height. This passes everything on try.

Comment 41

[Timothy Nikkel (:tnikkel)]

Comment on attachment 498730 [details] [diff] [review]
Patch V8

So why does getting the innerWidth/Height differ in some cases when getting it from the prescontext vs docshell? I find it strange that this would cause test failures.

Comment 42

[Wesley Johnston (:wesj)]

Attachment: Patch v7

I think I must have accidentally pushed against the same tree twice the other day or something. I'm not able to reproduce the try failure anymore using the original patch, and couldn't reproduce it locally last week. I'll put the original back up. Thanks :)

carrying over the r+ from tn. Still waiting for sr....

Comment 43

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

Comment on attachment 498928 [details] [diff] [review]
Patch v7

sr=dbaron

Comment 44

[Wesley Johnston (:wesj)]

Attachment: Patch for checkin

Comment 45

[Wesley Johnston (:wesj)]

Attachment: Tests for checkin

Comment 46

[Alon Zakai (:azakai)]

Pushed:

http://hg.mozilla.org/mozilla-central/rev/db27d37d7879
http://hg.mozilla.org/mozilla-central/rev/5ff3997a30c2

Comment 47

[Eric Shepherd [:sheppy]]

Updated documentation:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMWindowUtils#setCSSViewport%28%29
https://developer.mozilla.org/en/DOM/window.innerHeight
https://developer.mozilla.org/en/DOM/window.innerWidth

Comment 48

[Timothy Nikkel (:tnikkel)]

Sites don't use setCSSViewport directly, but Fennec uses it for pages that have a meta viewport. So the documentation isn't really clear about that.

Comment 49

[Boris Zbarsky [:bzbarsky]]

For what it's worth this broke things for all inner* consumers, because nothing in this patch makes sure the prescontext actually has the right size.  The EnsureSizeUpToDate call makes sure the _docshell_ has the right size, but that's not always synchronously propagated to the prescontext.

Comment 50

[Eric Shepherd [:sheppy]]

(In reply to comment #48)
> Sites don't use setCSSViewport directly, but Fennec uses it for pages that have
> a meta viewport. So the documentation isn't really clear about that.

I think the fact that the doc says you have to have UniversalXPConnect privs makes that fairly clear. Does it not?