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)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(3 files, 2 obsolete files)
4.80 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
5.97 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•11 years ago
|
||
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"? :)
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
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
Assignee | ||
Comment 5•11 years ago
|
||
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 :)
Assignee | ||
Comment 6•11 years ago
|
||
(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).
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #803919 -
Flags: review?(botond)
Comment 8•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #803919 -
Flags: review?(botond) → review+
Comment 9•11 years ago
|
||
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?
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Comment 11•11 years ago
|
||
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)
Assignee | ||
Comment 12•11 years ago
|
||
Update commit message to fix a typo in the bug number. Carrying r+
Attachment #803919 -
Attachment is obsolete: true
Attachment #814598 -
Flags: review+
Assignee | ||
Comment 13•11 years ago
|
||
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 14•11 years ago
|
||
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 15•11 years ago
|
||
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+
Assignee | ||
Comment 16•11 years ago
|
||
Landed with those review comments addressed: remote: https://hg.mozilla.org/integration/b2g-inbound/rev/1efba3ff110a remote: https://hg.mozilla.org/integration/b2g-inbound/rev/fb635df6e89b remote: https://hg.mozilla.org/integration/b2g-inbound/rev/823fe5c2959e
https://hg.mozilla.org/mozilla-central/rev/1efba3ff110a https://hg.mozilla.org/mozilla-central/rev/fb635df6e89b https://hg.mozilla.org/mozilla-central/rev/823fe5c2959e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•