Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1961243002:
Round anchor offsets to DIPs in ScrollAnchor::restore. (Closed)
|
Created:
4 years, 7 months ago by skobes Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRound anchor offsets to DIPs in ScrollAnchor::restore.
A fractional adjustment may not match the actual pixel delta of the anchor node.
This eliminates visible judder when the anchor node's position is animating.
Also ignore FrameView's fractional scroll offset when computing the anchor's
viewport-relative position. This ensures we round in the right direction.
BUG=594450
Committed: https://crrev.com/3c17160bd5dd8a82b0a9bd2b9cb531e285e6386f
Cr-Commit-Position: refs/heads/master@{#392752}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : tweak and add comments #
Total comments: 2
Messages
Total messages: 20 (9 generated)
Description was changed from ========== Round to DIPs in ScrollAnchor::restore. BUG=594450 ========== to ========== Round intermediate offsets to DIPs in ScrollAnchor::restore. A fractional adjustment may not match the actual pixel delta of the anchor node. This is true regardless of whether the ScrollableArea supports fractional scroll offsets. BUG=594450 ==========
skobes@chromium.org changed reviewers: + ojan@chromium.org, ymalik@chromium.org
This seems to eliminate the animation judder in all the scenarios I tested: - high and low DPI - composited and non-composited scrolling - REF::FractionalScrollOffsets enabled and disabled - FrameView and PLSA In theory we ought to be able to round to physical pixels instead of DIPs by looking at Page::deviceScaleFactor(), but I couldn't get that to work for some reason. The changes in crbug.com/485650 will make them equivalent anyway.
lgtm, Can you add a quick comment about why rounding works here? I guess it's because rounding sort of tells us the anchor's pixel snapped position?
On 2016/05/10 17:03:14, ymalik1 wrote: > lgtm, > > Can you add a quick comment about why rounding works here? I guess it's because > rounding sort of tells us the anchor's pixel snapped position? Added some comments.
Description was changed from ========== Round intermediate offsets to DIPs in ScrollAnchor::restore. A fractional adjustment may not match the actual pixel delta of the anchor node. This is true regardless of whether the ScrollableArea supports fractional scroll offsets. BUG=594450 ========== to ========== Round intermediate offsets to DIPs in ScrollAnchor::restore. A fractional adjustment may not match the actual pixel delta of the anchor node. This eliminates judder when the anchor node's position is animating. BUG=594450 ==========
Description was changed from ========== Round intermediate offsets to DIPs in ScrollAnchor::restore. A fractional adjustment may not match the actual pixel delta of the anchor node. This eliminates judder when the anchor node's position is animating. BUG=594450 ========== to ========== Round anchor offsets to DIPs in ScrollAnchor::restore. A fractional adjustment may not match the actual pixel delta of the anchor node. This eliminates visible judder when the anchor node's position is animating. Also ignore FrameView's fractional scroll offset when computing the anchor's viewport-relative position. This ensures we round in the right direction. BUG=594450 ==========
The CQ bit was checked by skobes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ymalik@chromium.org Link to the patchset: https://codereview.chromium.org/1961243002/#ps40001 (title: "tweak and add comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1961243002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1961243002/40001
Message was sent while issue was closed.
Description was changed from ========== Round anchor offsets to DIPs in ScrollAnchor::restore. A fractional adjustment may not match the actual pixel delta of the anchor node. This eliminates visible judder when the anchor node's position is animating. Also ignore FrameView's fractional scroll offset when computing the anchor's viewport-relative position. This ensures we round in the right direction. BUG=594450 ========== to ========== Round anchor offsets to DIPs in ScrollAnchor::restore. A fractional adjustment may not match the actual pixel delta of the anchor node. This eliminates visible judder when the anchor node's position is animating. Also ignore FrameView's fractional scroll offset when computing the anchor's viewport-relative position. This ensures we round in the right direction. BUG=594450 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Round anchor offsets to DIPs in ScrollAnchor::restore. A fractional adjustment may not match the actual pixel delta of the anchor node. This eliminates visible judder when the anchor node's position is animating. Also ignore FrameView's fractional scroll offset when computing the anchor's viewport-relative position. This ensures we round in the right direction. BUG=594450 ========== to ========== Round anchor offsets to DIPs in ScrollAnchor::restore. A fractional adjustment may not match the actual pixel delta of the anchor node. This eliminates visible judder when the anchor node's position is animating. Also ignore FrameView's fractional scroll offset when computing the anchor's viewport-relative position. This ensures we round in the right direction. BUG=594450 Committed: https://crrev.com/3c17160bd5dd8a82b0a9bd2b9cb531e285e6386f Cr-Commit-Position: refs/heads/master@{#392752} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3c17160bd5dd8a82b0a9bd2b9cb531e285e6386f Cr-Commit-Position: refs/heads/master@{#392752}
Message was sent while issue was closed.
ojan@chromium.org changed reviewers: + eae@chromium.org, szager@chromium.org
Message was sent while issue was closed.
eae or szager are better reviewers than me for this change. eae/szager, mind sanity checking that this is the right place to do rounding?
Message was sent while issue was closed.
https://codereview.chromium.org/1961243002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/1961243002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:98: relativeBounds.moveBy(-flooredIntPoint(scroller->scrollPositionDouble())); We round scroll offsets in PaintLayerScrollableArea and floor them in RootFrameViewport. Logically rounding seems more correct but as long as we're consistent it doesn't really matter.
Message was sent while issue was closed.
https://codereview.chromium.org/1961243002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/1961243002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:98: relativeBounds.moveBy(-flooredIntPoint(scroller->scrollPositionDouble())); On 2016/05/10 23:09:18, eae wrote: > We round scroll offsets in PaintLayerScrollableArea and floor them in > RootFrameViewport. Logically rounding seems more correct but as long as we're > consistent it doesn't really matter. It's somewhat more subtle. For the return value of relativeBounds the important thing is that the fractional part matches the fractional position the paint code will see when it paints the anchor node into the GraphicsLayer that holds the scrolling content. The way the ScrollableArea treats the fractional part of the scroll offset is irrelevant. If scroller is a PaintLayerScrollableArea, localToAncestorQuad has already given us coordinates relative to the scroller's viewport based on a floored scroll offset (in LayoutBox::scrolledContentOffset). Flooring the FrameView's offset is consistent with that.
Message was sent while issue was closed.
On 2016/05/10 23:19:00, skobes wrote: > https://codereview.chromium.org/1961243002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): > > https://codereview.chromium.org/1961243002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:98: > relativeBounds.moveBy(-flooredIntPoint(scroller->scrollPositionDouble())); > On 2016/05/10 23:09:18, eae wrote: > > We round scroll offsets in PaintLayerScrollableArea and floor them in > > RootFrameViewport. Logically rounding seems more correct but as long as we're > > consistent it doesn't really matter. > > It's somewhat more subtle. > > For the return value of relativeBounds the important thing is that the > fractional part matches the fractional position the paint code will see when it > paints the anchor node into the GraphicsLayer that holds the scrolling content. > The way the ScrollableArea treats the fractional part of the scroll offset is > irrelevant. > > If scroller is a PaintLayerScrollableArea, localToAncestorQuad has already given > us coordinates relative to the scroller's viewport based on a floored scroll > offset (in LayoutBox::scrolledContentOffset). Flooring the FrameView's offset > is consistent with that. Ah, the joys of rounding. Thanks for clarifying!
Message was sent while issue was closed.
And just to close the loop on my original comments upthread - the reason rounding to physical pixels didn't work is that we don't actually layout at physical-pixel granularity (http://crbug.com/610805). :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
