Closed Bug 602580 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b4+ ---

People

(Reporter: mbrubeck, Assigned: wesj)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 11 obsolete files)

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.
tracking-fennec: --- → ?
Assignee: nobody → mbrubeck
tracking-fennec: ? → 2.0b3+
Blocks: 605538
Attached patch Patch? (obsolete) — Splinter Review
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.
Attachment #489327 - Flags: feedback?(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 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).
Assigning this to Wes since he has started on it and I haven't.
Assignee: mbrubeck → wjohnston
Status: NEW → ASSIGNED
Hmm, what behaviour do we want if a site uses the meta viewport tag and sets the innerWidth/Height anyway?
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.
Attachment #489327 - Flags: feedback?(tnikkel)
Attachment #489327 - Flags: feedback?(roc)
Attachment #489327 - Flags: feedback?(dbaron)
(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?
Attached patch Fix v2 (obsolete) — Splinter Review
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.
Attachment #489327 - Attachment is obsolete: true
Attachment #491914 - Flags: review?(tnikkel)
Attachment #489327 - Flags: feedback?(roc)
Attachment #489327 - Flags: feedback?(dbaron)
Attachment #491914 - Flags: superreview?(dbaron)
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.
tracking-fennec: 2.0b3+ → 2.0b4+
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.
Attached patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #491914 - Attachment is obsolete: true
Attachment #492839 - Flags: superreview?(dbaron)
Attachment #492839 - Flags: review?(tnikkel)
Attachment #491914 - Flags: superreview?(dbaron)
Attachment #491914 - Flags: review?(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.
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 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.
Attached patch Patch v4 (obsolete) — Splinter Review
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.
Attachment #492839 - Attachment is obsolete: true
Attachment #494727 - Flags: review?(tnikkel)
Attachment #492839 - Flags: superreview?(dbaron)
Attachment #492839 - Flags: review?(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.
Attached patch Patch v5 (obsolete) — Splinter Review
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.
Attachment #494727 - Attachment is obsolete: true
Attachment #495530 - Flags: review?(tnikkel)
Attachment #494727 - Flags: review?(tnikkel)
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.
(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.
(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 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.
Attached patch Patch v6 (obsolete) — Splinter Review
Fixed up patch. I have a quick test file written, but it currently has problems when run with other mochitests. Working on it...
Attachment #495530 - Attachment is obsolete: true
Attachment #496311 - Flags: superreview?(dbaron)
Attachment #496311 - Flags: review?(tnikkel)
Attachment #495530 - Flags: review?(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.
Attached patch Patch v7 (obsolete) — Splinter Review
Whoops. I remember making that change.... My mq must have eaten it somewhere else.
Attachment #496311 - Attachment is obsolete: true
Attachment #496339 - Flags: superreview?(dbaron)
Attachment #496339 - Flags: review?(tnikkel)
Attachment #496311 - Flags: superreview?(dbaron)
Attachment #496311 - Flags: review?(tnikkel)
Comment on attachment 496339 [details] [diff] [review]
Patch v7

r+ conditional on this getting a test.
Attachment #496339 - Flags: review?(tnikkel) → review+
Attached patch Tests (obsolete) — Splinter Review
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.
Attachment #496361 - Flags: review?(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?
Attached patch Tests v2 (obsolete) — Splinter Review
Moved this into two test, and sets one to only run for mobile.
Attachment #496361 - Attachment is obsolete: true
Attachment #496643 - Flags: review?(tnikkel)
Attachment #496361 - Flags: review?(tnikkel)
Comment on attachment 496643 [details] [diff] [review]
Tests v2

Looks good. Thanks for going through all the iterations to get this done.
Attachment #496643 - Flags: review?(tnikkel) → review+
dbaron - sr ping
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.
Maybe the revision you pushed your patches on top of is causing the failure?
Ie, it got backed out or fixed later.
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.
Can you reproduce the problem on your own machine?
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.
Does it happen on whatever OS you run?
Alternatively you could try wrapping all of your changes in a "displayport is set" check to see if anything changes.
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.
Attached patch Patch V8 (obsolete) — Splinter Review
Changed to check what it returns during GetInnerWidth/Height. This passes everything on try.
Attachment #496339 - Attachment is obsolete: true
Attachment #498730 - Flags: superreview?(dbaron)
Attachment #498730 - Flags: review?(tnikkel)
Attachment #496339 - Flags: superreview?(dbaron)
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.
Attached patch Patch v7 (obsolete) — Splinter Review
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....
Attachment #498730 - Attachment is obsolete: true
Attachment #498928 - Flags: superreview?(dbaron)
Attachment #498928 - Flags: review+
Attachment #498730 - Flags: superreview?(dbaron)
Attachment #498730 - Flags: review?(tnikkel)
Comment on attachment 498928 [details] [diff] [review]
Patch v7

sr=dbaron
Attachment #498928 - Flags: superreview?(dbaron) → superreview+
Attachment #498928 - Attachment is obsolete: true
Attachment #499148 - Flags: superreview+
Attachment #499148 - Flags: review+
Attachment #496643 - Attachment is obsolete: true
Attachment #499150 - Flags: review+
Pushed:

http://hg.mozilla.org/mozilla-central/rev/db27d37d7879
http://hg.mozilla.org/mozilla-central/rev/5ff3997a30c2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
Depends on: 641188
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.
(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?
Depends on: 801341
See Also: → 1514429
You need to log in before you can comment on or make changes to this bug.