1016682 - Page stuck in scrollable state after zooming back out

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

STR:
  1. Enable overscrolling in the developer prefs.
     (At the time I write this, the pref hasn't landed yet. Pull request here [1].) 

  2. Load http://people.mozilla.org/~kgupta/tmp/div.html

  3. Observe the page is not scrollable. 
     (If it is, use a bigger phone :). I used Nexus 4.)

  4. Overscroll the <div>. Observe the overscroll effect. 
     So far, so good.

  5. Zoom the page in. Observe that the <div> now cannot
     be overscrolled; instead, it is the page that overscrolls.
     So far, this is expected.

  6. Zoom the page back out. Overscroll the <div>.

Expected results:
  Now that the page is zoomed out, the <div> can overscroll again.

Actual results:
  It is still the page that overscrolls.

[1] https://github.com/mozilla-b2g/gaia/pull/19654

Comment 1

[Botond Ballo [:botond]]

This is a rounding error issue.

The root APZC has composition bounds 768 x 1032 screen pixels, and scrollable rect 980 x 1316 CSS pixels, which comes from TabChild calling SetCSSViewport with this size.

The initial zoom is set based on the ratio of the widths: 768 / 980 = 0.783673.

Zooming in increases this zoom; when zooming back out, the zoom is clamped to a minimum calculated as the _maximum_ of the width ratio (768 / 980 = 0.783673) and the height ratio (1032 / 1316 = 0.784194) [1]. This minimum is thus 0.784194.

At this zoom, the width of the "composited size in css pixels" is calculated as 979.35 pixels, and since this is smaller than the width of the scrollable rect, 980. Therefore, we determine [2] that the x axis has a scroll range and consider the page to be scrollable.

[1] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?from=AsyncPanZoomController.cpp#942
[2] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.cpp?from=Axis.cpp#107

Comment 2

[Botond Ballo [:botond]]

It's not immediately clear to me what the best fix for this is. A fix on the APZ side seems like a hack. Rather, it would be nice if the CSS viewport width would be calculated so as to match the screen's aspect ratio more exactly - SetCSSViewport takes a CSSSize, after all, not a CSSIntSize.

However, the viewport size comes from nsViewportInfo::mSize, which *is* a CSSIntSize, and the code which calculates it (nsDocument::GetViewportInfo) does explicit rounding (e.g. [1]), so I'm not sure if changing that to a CSSSize would be appropriate.

[1] http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp?from=nsDocument.cpp#7658

Comment 3

[Kartikaya Gupta (email:[e-mail])]

It seems wrong that you can't get back to the initial zoom of the page by zooming in and out. However I think the APZ code that clamps the zoom to the max of the width/height ratios is correct - it's the initial parameters that need changing. I think we should, in order of preference:

1) set the initial zoom the same way as the APZ calculates the min zoom (that is, take the max of the width/height ratios).

2) Ensure that the CSS viewport height is rounded up rather than rounded down (i.e. make it 1317 pixels tall in this case) so that in cases like this the width of the page is the "determining factor" of the min zoom.

Comment 4

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> It seems wrong that you can't get back to the initial zoom of the page by
> zooming in and out. However I think the APZ code that clamps the zoom to the
> max of the width/height ratios is correct - it's the initial parameters that
> need changing. I think we should, in order of preference:
> 
> 1) set the initial zoom the same way as the APZ calculates the min zoom
> (that is, take the max of the width/height ratios).

The problem with this is that APZ will now consider the page to be scrollable right off the bat. In other words, it makes the "right off the bat" situation consistent with the "zoom in and then zoom out" situation by making it, too, have the bad behaviour described in this bug.

> 2) Ensure that the CSS viewport height is rounded up rather than rounded
> down (i.e. make it 1317 pixels tall in this case) so that in cases like this
> the width of the page is the "determining factor" of the min zoom.

This would have the same problem - the page would now be ever so slightly scrollable vertically on account of that extra pixel (or fraction of pixel) of height.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #4)
> > 1) set the initial zoom the same way as the APZ calculates the min zoom
> > (that is, take the max of the width/height ratios).
> 
> The problem with this is that APZ will now consider the page to be
> scrollable right off the bat. In other words, it makes the "right off the
> bat" situation consistent with the "zoom in and then zoom out" situation by
> making it, too, have the bad behaviour described in this bug.

Yes, that's a problem, but I think it puts us one step closer to a proper solution. Fixing the scrollability issue will likely involve making the CSS viewport fractional.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> (In reply to Botond Ballo [:botond] from comment #4)
> > > 1) set the initial zoom the same way as the APZ calculates the min zoom
> > > (that is, take the max of the width/height ratios).
> > 
> > The problem with this is that APZ will now consider the page to be
> > scrollable right off the bat. In other words, it makes the "right off the
> > bat" situation consistent with the "zoom in and then zoom out" situation by
> > making it, too, have the bad behaviour described in this bug.
> 
> Yes, that's a problem, but I think it puts us one step closer to a proper
> solution. Fixing the scrollability issue will likely involve making the CSS
> viewport fractional.

If the CSS viewport is fractional, then the inconsistency between the width and height ratios does not arise, and how we calculate the initial zoom does not matter.

On the other hand, changing how we calculate the initial zoom, before making the CSS viewport fractional, will regress the overscrolling behaviour, so I'm not sure how it gets us closer to a proper solution. It seems the proper solution is just to make the CSS viewport fractional.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #6)
> If the CSS viewport is fractional, then the inconsistency between the width
> and height ratios does not arise, and how we calculate the initial zoom does
> not matter.
> 

... in this example, yes. There might be other scenarios where the inconsistency matters. Content could force a CSS viewport using the meta-viewport tag, for example. It could force it to be exactly 980x1316 in this example, and so changing our default computation of the CSS viewport would have no effect.

> On the other hand, changing how we calculate the initial zoom, before making
> the CSS viewport fractional, will regress the overscrolling behaviour, so
> I'm not sure how it gets us closer to a proper solution. It seems the proper
> solution is just to make the CSS viewport fractional.

We could do both and land them together. Overscrolling is preffed off right now anyway, so the regression wouldn't be visible.

Comment 8

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> (In reply to Botond Ballo [:botond] from comment #6)
> > If the CSS viewport is fractional, then the inconsistency between the width
> > and height ratios does not arise, and how we calculate the initial zoom does
> > not matter.
> 
> ... in this example, yes. There might be other scenarios where the
> inconsistency matters. Content could force a CSS viewport using the
> meta-viewport tag, for example. It could force it to be exactly 980x1316 in
> this example, and so changing our default computation of the CSS viewport
> would have no effect.

Good point.

> > On the other hand, changing how we calculate the initial zoom, before making
> > the CSS viewport fractional, will regress the overscrolling behaviour, so
> > I'm not sure how it gets us closer to a proper solution. It seems the proper
> > solution is just to make the CSS viewport fractional.
> 
> We could do both and land them together.

Sounds good!

> Overscrolling is preffed off right now anyway, so the regression wouldn't be visible.

(It would be visible to people testing overscrolling like Gordon. But that's a moot point if we're landing them together.)

Comment 9

[Botond Ballo [:botond]]

Attachment: Part 1 - Store CSS viewport size as floating-point to avoid rounding errors

This patch changes nsViewportInfo::mSize to be a CSSSize and updates surrounding code appropriately.

I didn't change nsIDOMWindowUtils.getViewportInfo() to return floats instead of ints for the viewport size. As far as I can tell, this function is only used in test code [1], and the test code only tests for integer values. If someone feels strongly about also changing nsIDOMWindowUtils.getViewportInfo(), please let me know.

[1] http://dxr.mozilla.org/mozilla-central/search?q=getviewportinfo&=mozilla-central&redirect=true

Comment 10

[Botond Ballo [:botond]]

Attachment: Part 2 - Tolerate floating-point error in Axis::CanScroll()

Since we're now in floating-point land, the comparison in Axis::CanScroll() now needs to tolerate a COORDINATE_EPSILON's worth of floating-point error.

Note that COORDINATE_EPSILON is much smaller than the error caused by round-tripping from float to int and back (which Part 1 fixes), so Part 1 is very much necessary still.

With these two patches, the STR in this bug works as expected. 

For completeness, I'll also fix the initial zoom calculation as Kats suggested, in a Part 3 patch.

Comment 11

[Botond Ballo [:botond]]

Attachment: Part 1 - Store CSS viewport size as floating-point to avoid rounding errors

Updated to remove some debugging statements that snuck in there.

Comment 12

[Botond Ballo [:botond]]

Attachment: Part 2 - Tolerate floating-point error in Axis::CanScroll()

No reason to re-upload this one besides an OCDish desire to keep the patches in order.

Comment 13

[Botond Ballo [:botond]]

Attachment: Part 3 - Make TabChild's initial zoom calculation consistent with APZ

This patch makes TabChild's initial-zoom calculation consistent with APZ's min-zoom calculation.

I'm slightly worried about the possibility of this patch causing regressions; since it's not actually needed to fix the STR in this bug, what do you think about landing it separately?

Comment 14

[Botond Ballo [:botond]]

Try push (with all three patches): https://tbpl.mozilla.org/?tree=Try&rev=7dfdc1abcb17

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8430927 [details] [diff] [review]
Part 3 - Make TabChild's initial zoom calculation consistent with APZ

Review of attachment 8430927 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I agree it would be good to land this separately since it might introduce regressions.

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8430919 [details] [diff] [review]
Part 1 - Store CSS viewport size as floating-point to avoid rounding errors

Looks fine to me.

Matt, I think you wrote most of this code? Any reason we can't make the size a float?

Comment 17

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8430919 [details] [diff] [review]
Part 1 - Store CSS viewport size as floating-point to avoid rounding errors

Review of attachment 8430919 [details] [diff] [review]:
-----------------------------------------------------------------

Actually Scott Johnson wrote this code, but I reviewed it. :)

I don't see any reason this shouldn't be a float.

Comment 18

[Botond Ballo [:botond]]

Landed parts 1 and 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f2c8fd727a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d483f2df0424

Comment 19

[Botond Ballo [:botond]]

Landed part 3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9245a2d5e693

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f2c8fd727a4
https://hg.mozilla.org/mozilla-central/rev/d483f2df0424
https://hg.mozilla.org/mozilla-central/rev/9245a2d5e693