Closed Bug 915831 Opened 11 years ago Closed 11 years ago

APZC zoom does not consistently stop at the same place when bounded by overzoom

Categories

(Core :: Graphics: Layers, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files, 2 obsolete files)

The zoom behaviour in APZC is a little weird, in that it computes the new zoom value that would take effect if the pinch succeeded, and then checks to see if that would cause the content to be smaller than the composition bounds (see Axis::ScaleWillOverscroll call sites and Axis::ScaleWillOverscrollBothSides). If the content would be smaller, it aborts that pinch increment.

So, if the current zoom is say 1.51, with a scrollableRect width of 200 and a composition bounds of 300 (so we are zoomed in just a tiny bit more than the minimum allowed zoom) and the user tries to zoom out, this zoom out action might fail, depending on the span ratio in APZC::OnScale. If the span ratio is 1.00666666666667 or less, the zoom will succeed but otherwise it won't because the new computed zoom will be < 1.50 which is not allowed.
Maybe rather than aborting the pinch in such a case, we should zoom out to the minimum allowed zoom?

In a sense this is analogous to overscroll, where we scroll as far as possible - I guess this would be "underzoom"? :)
I'm defining "overzoom" to be when you zoom in so far that the content is smaller than the composition bounds, so that you necessarily end up with some dead space.
Summary: APZC zoom does not consistently stop at the same place when bounded by overscroll → APZC zoom does not consistently stop at the same place when bounded by overzoom
Sorry, didn't see this comment before I uploaded the patches.

(In reply to Botond Ballo [:botond] from comment #1)
> Maybe rather than aborting the pinch in such a case, we should zoom out to
> the minimum allowed zoom?
> 

Yup, that's what part 1 does.

> In a sense this is analogous to overscroll, where we scroll as far as
> possible - I guess this would be "underzoom"? :)

You're right, underzoom would be more appropriate than overzoom :)
(I don't think this bug has much noticeable effect so I'm just going to leave the patches here for now and get back to a more thorough testing of them later).
Comment on attachment 803917 [details] [diff] [review]
Part 1 - Take into account the overzoom boundary

Might as well get these patches landed, no point in just sitting on them
Attachment #803917 - Flags: review?(botond)
Attachment #803919 - Flags: review?(botond)
While reading some of the code relevant to these patches, I came across something suspicious:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/Axis.cpp#257 adds GetOrigin(), which is a coordinate of FrameMetrics::mScrollOffset, which is a CSSPoint, and aFocus, which http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#583 initializes with a coordinate of a ScreenPoint.

Do we have a units mismatch here?
Attachment #803919 - Flags: review?(botond) → review+
Comment on attachment 803917 [details] [diff] [review]
Part 1 - Take into account the overzoom boundary

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +563,5 @@
> +    CSSToScreenScale realMinZoom = mMinZoom;
> +    CSSToScreenScale realMaxZoom = mMaxZoom;
> +    if (mFrameMetrics.mCompositionBounds.width > mFrameMetrics.mScrollableRect.width * realMinZoom.scale) {
> +      realMinZoom.scale = mFrameMetrics.mCompositionBounds.width / mFrameMetrics.mScrollableRect.width;
> +    }

I think

realMinZoom.scale = max(realMinZoom.scale, mFrameMetrics.mCompositionBounds.width / mFrameMetrics.mScrollableRect.width);

would be clearer (and similarly for height).

@@ +582,2 @@
>  
>        switch (mX.ScaleWillOverscroll(spanRatio, focusPoint.x))

Can this call to ScaleWillOverscroll still return OVERSCROLL_BOTH now that we're clamping in advance? If not, should we assert to that effect?
(In reply to Botond Ballo [:botond] from comment #8)
> While reading some of the code relevant to these patches, I came across
> something suspicious:
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/Axis.cpp#257
> adds GetOrigin(), which is a coordinate of FrameMetrics::mScrollOffset,
> which is a CSSPoint, and aFocus, which
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/
> AsyncPanZoomController.cpp#583 initializes with a coordinate of a
> ScreenPoint.
> 
> Do we have a units mismatch here?

Yeah, good catch! I filed bug 923431 for this and other misc cleanup. I will come back to this bug once that is done.
This addresses your first note in comment 9. It also adds a test for the overzoom behaviour.
Attachment #803917 - Attachment is obsolete: true
Attachment #803917 - Flags: review?(botond)
Attachment #814597 - Flags: review?(botond)
Update commit message to fix a typo in the bug number. Carrying r+
Attachment #803919 - Attachment is obsolete: true
Attachment #814598 - Flags: review+
This addresses the second part of comment 9 but also goes beyond that a little bit to merge ScaleWillOverscroll and ScaleWillOverscrollAmount. It seemed kind of silly to me that APZC called ScaleWillOverscroll and then based on the result would call ScaleWillOverscrollAmount which then just called ScaleWillOverscroll again.

Note that these patches apply on top of those from bug 923431.
Attachment #814599 - Flags: review?(botond)
Comment on attachment 814597 [details] [diff] [review]
Part 1 - Take into account the overzoom boundary (v2)

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

Looks good. r=me with the small correction to the test.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +198,5 @@
> +  fm.mScrollableRect = CSSRect(0, 0, 125, 150);
> +  fm.mScrollOffset = CSSPoint(10, 0);
> +  fm.mZoom = CSSToScreenScale(1.0);
> +  apzc->SetFrameMetrics(fm);
> +  // the visible area of the document in CSS pixels is x=2 y=0 w=100 h=100

Looks like this should be x=10.
Attachment #814597 - Flags: review?(botond) → review+
Comment on attachment 814599 [details] [diff] [review]
Part 3 - Squash together some code to simplify it

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

Looks good.

I note that we have the same redundancy in DisplacementWillOverscroll() and DisplacementWillOverscrollAmount(). I am OK with fixing that another time though.

::: gfx/layers/ipc/Axis.h
@@ +135,2 @@
>     * If a scale will overscroll the axis, this returns the amount and in what
>     * direction. Similar to getExcess() but takes a displacement to apply.

nit: getExcess() should have a capital G.
Attachment #814599 - Flags: review?(botond) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: