Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2404393003:
Tie scroll anchoring adjustments to frame lifecycle instead of layout. (Closed)
|
Created:
4 years, 2 months ago by skobes Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dglazkov+blink, dshwang, eae+blinkwatch, jchaffraix+rendering, kinuko+watch, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rwlbuis, sof, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTie scroll anchoring adjustments to frame lifecycle instead of layout.
This applies suppressions to the entire animation frame, which lets us spec the
behavior without referring to layout passes.
ScrollAnchor::notifyBeforeLayout, formerly known as save(), adds the scroller to
a queue owned by the FrameView, which iterates the queue to do the scroll
adjustments after layout, but before the compositing update.
Web APIs that force synchronous layout also force these adjustments to happen
synchronously. This means it is impossible for script to observe an un-adjusted
scroll position after an anchor node movement.
BUG=648780
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/e0e3af8004e7f656ae8e1cb45c76a4a013c43961
Cr-Commit-Position: refs/heads/master@{#428155}
Patch Set 1 #
Total comments: 11
Patch Set 2 : address review comments #
Total comments: 9
Patch Set 3 : rebase #Patch Set 4 : force adjustments in Document::updateStyleAndLayout #
Total comments: 5
Patch Set 5 : add DCHECK #Messages
Total messages: 61 (33 generated)
Description was changed from ========== [WIP] Anchor at animation frame boundaries. BUG=648780 ========== to ========== [WIP] Anchor at animation frame boundaries. BUG=648780 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [WIP] Anchor at animation frame boundaries. BUG=648780 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Tie scroll anchoring adjustments to frame lifecycle instead of layout. This applies suppressions to the entire animation frame, which lets us spec the behavior without referring to layout passes. ScrollAnchor::notifyBeforeLayout, formerly save(), adds the scroller to a queue owned by the FrameView, which iterates over the queue to do scroll adjustments after layout, but before the compositing update. Web APIs like scrollTop that read the current scroll position force these adjustments to happen synchronously. BUG=648780 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Tie scroll anchoring adjustments to frame lifecycle instead of layout. This applies suppressions to the entire animation frame, which lets us spec the behavior without referring to layout passes. ScrollAnchor::notifyBeforeLayout, formerly save(), adds the scroller to a queue owned by the FrameView, which iterates over the queue to do scroll adjustments after layout, but before the compositing update. Web APIs like scrollTop that read the current scroll position force these adjustments to happen synchronously. BUG=648780 ========== to ========== Tie scroll anchoring adjustments to frame lifecycle instead of layout. This applies suppressions to the entire animation frame, which lets us spec the behavior without referring to layout passes. ScrollAnchor::notifyBeforeLayout, formerly known as save(), adds the scroller to a queue owned by the FrameView, which iterates the queue to do the scroll adjustments after layout, but before the compositing update. Web APIs like scrollTop that read the current scroll position force these adjustments to happen synchronously. BUG=648780 ==========
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
skobes@chromium.org changed reviewers: + bokan@chromium.org, ojan@chromium.org, ymalik@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, some questions: 1) Are we just landing this in favor of anchoring on every layout? Should we add a flag that can allow us to switch between the two to make testing bugs easier? 2) Does this fix the WSJ bug? crbug.com/652377 3) Is writing LayoutTests the preferred way of testing post this change? https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/suppressions-are-per-frame.html (right): https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/suppressions-are-per-frame.html:22: document.body.offsetHeight; Can we add a test that forces a call to Document::updateScrollPosition and the adjustment is actually applied? https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:250: frameView->enqueueScrollAnchoringAdjustment(owningScroller); I wonder if we should just enqueue m_scroller here and in FrameView::performScrollAnchoringAdjustments, get RootFrameViewport to return layoutViewport()->scrollAnchor(). https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right): https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ScrollAnchor.h:91: // by the main frame's FrameView. s/owned by the main frame's FrameView/owned by RootFrameViewport's m_layoutViewport, which in the general case is the main frame's FrameView? https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:2000: webViewImpl()->updateAllLifecyclePhases(); Why did we need to add this?
https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right): https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ScrollAnchor.h:91: // by the main frame's FrameView. On 2016/10/13 13:05:40, ymalik wrote: > s/owned by the main frame's FrameView/owned by RootFrameViewport's > m_layoutViewport, which in the general case is the main frame's FrameView? Right, if we swapped the layout viewport using document.rootScroller, we might be owned by a different FrameView or PLSA but m_scroller will still be RootFrameViewport.
Description was changed from ========== Tie scroll anchoring adjustments to frame lifecycle instead of layout. This applies suppressions to the entire animation frame, which lets us spec the behavior without referring to layout passes. ScrollAnchor::notifyBeforeLayout, formerly known as save(), adds the scroller to a queue owned by the FrameView, which iterates the queue to do the scroll adjustments after layout, but before the compositing update. Web APIs like scrollTop that read the current scroll position force these adjustments to happen synchronously. BUG=648780 ========== to ========== Tie scroll anchoring adjustments to frame lifecycle instead of layout. This applies suppressions to the entire animation frame, which lets us spec the behavior without referring to layout passes. ScrollAnchor::notifyBeforeLayout, formerly known as save(), adds the scroller to a queue owned by the FrameView, which iterates the queue to do the scroll adjustments after layout, but before the compositing update. Web APIs like scrollTop that read the current scroll position force these adjustments to happen synchronously. BUG=648780 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
On 2016/10/13 13:05:40, ymalik wrote: > 1) Are we just landing this in favor of anchoring on every layout? Should we add > a flag that can allow us to switch between the two to make testing bugs easier? Adding a flag will introduce significant complexity, and it should be straightforward to diagnose regressions with bisect-builds.py, so I'm not in favor of adding a flag at this time. > 2) Does this fix the WSJ bug? crbug.com/652377 Sadly no. The symptoms are the same after this change. :( > 3) Is writing LayoutTests the preferred way of testing post this change? I think we should use layout tests to test behavior related to the frame lifecycle, since the lifecycle is artificially managed in a RenderingTest. But tests that verify isolated behaviors of ScrollAnchor should still be unit tests. https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/suppressions-are-per-frame.html (right): https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/suppressions-are-per-frame.html:22: document.body.offsetHeight; On 2016/10/13 13:05:39, ymalik wrote: > Can we add a test that forces a call to Document::updateScrollPosition and the > adjustment is actually applied? Done. https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:250: frameView->enqueueScrollAnchoringAdjustment(owningScroller); On 2016/10/13 13:05:39, ymalik wrote: > I wonder if we should just enqueue m_scroller here and in > FrameView::performScrollAnchoringAdjustments, get RootFrameViewport to return > layoutViewport()->scrollAnchor(). I thought about this but it seemed a bit cleaner for ScrollableArea::scrollAnchor to reflect ownership. https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ScrollAnchor.h (right): https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ScrollAnchor.h:91: // by the main frame's FrameView. On 2016/10/13 13:05:40, ymalik wrote: > s/owned by the main frame's FrameView/owned by RootFrameViewport's > m_layoutViewport, which in the general case is the main frame's FrameView? Done. https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ScrollAnchor.h:91: // by the main frame's FrameView. On 2016/10/13 14:23:57, bokan wrote: > On 2016/10/13 13:05:40, ymalik wrote: > > s/owned by the main frame's FrameView/owned by RootFrameViewport's > > m_layoutViewport, which in the general case is the main frame's FrameView? > > Right, if we swapped the layout viewport using document.rootScroller, we might > be owned by a different FrameView or PLSA but m_scroller will still be > RootFrameViewport. Acknowledged. https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:2000: webViewImpl()->updateAllLifecyclePhases(); On 2016/10/13 13:05:40, ymalik wrote: > Why did we need to add this? The programmatic scroll clears the anchor node. If we don't finish the frame here, the layout that happens during WebViewImpl::resize won't compute a new one. I think that's the correct behavior - if the page sets scrollTop in the same frame as the anchor node moves, the programmatic scroll should "win".
lgtm https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2404393003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:2000: webViewImpl()->updateAllLifecyclePhases(); On 2016/10/13 17:21:37, skobes wrote: > On 2016/10/13 13:05:40, ymalik wrote: > > Why did we need to add this? > > The programmatic scroll clears the anchor node. If we don't finish the frame > here, the layout that happens during WebViewImpl::resize won't compute a new > one. > > I think that's the correct behavior - if the page sets scrollTop in the same > frame as the anchor node moves, the programmatic scroll should "win". Acknowledged.
szager@chromium.org changed reviewers: + szager@chromium.org
https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2652: forAllNonThrottledFrameViews([](FrameView& frameView) { Shouldn't this happen before updateViewportIntersectionsForSubtree? https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:187: case ClampingScroll: I would have thought this would be treated like a ProgrammaticScroll, no?
https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2652: forAllNonThrottledFrameViews([](FrameView& frameView) { On 2016/10/13 17:54:49, szager1 wrote: > Shouldn't this happen before updateViewportIntersectionsForSubtree? It does. For targetState > LayoutClean, the call to updateViewportIntersectionsForSubtree occurs at the end of the function. For targetState <= LayoutClean we do not perform the scroll anchoring adjustments. https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:187: case ClampingScroll: On 2016/10/13 17:54:49, szager1 wrote: > I would have thought this would be treated like a ProgrammaticScroll, no? Generally it is. The important difference is that we don't clear the scroll anchor. We also can't use programmaticScrollHelper here because ProgrammaticScrollAnimator will pass ProgrammaticScroll to scrollOffsetChanged.
https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2652: forAllNonThrottledFrameViews([](FrameView& frameView) { On 2016/10/13 18:00:31, skobes wrote: > On 2016/10/13 17:54:49, szager1 wrote: > > Shouldn't this happen before updateViewportIntersectionsForSubtree? > > It does. For targetState > LayoutClean, the call to > updateViewportIntersectionsForSubtree occurs at the end of the function. > > For targetState <= LayoutClean we do not perform the scroll anchoring > adjustments. > Ah, I see, thanks. https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:187: case ClampingScroll: On 2016/10/13 18:00:31, skobes wrote: > On 2016/10/13 17:54:49, szager1 wrote: > > I would have thought this would be treated like a ProgrammaticScroll, no? > > Generally it is. The important difference is that we don't clear the scroll > anchor. > > We also can't use programmaticScrollHelper here because > ProgrammaticScrollAnimator will pass ProgrammaticScroll to scrollOffsetChanged. This code path doesn't call cancelScrollAnimation(), is that intended?
https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:187: case ClampingScroll: On 2016/10/13 18:50:56, szager1 wrote: > On 2016/10/13 18:00:31, skobes wrote: > > On 2016/10/13 17:54:49, szager1 wrote: > > > I would have thought this would be treated like a ProgrammaticScroll, no? > > > > Generally it is. The important difference is that we don't clear the scroll > > anchor. > > > > We also can't use programmaticScrollHelper here because > > ProgrammaticScrollAnimator will pass ProgrammaticScroll to > scrollOffsetChanged. > > This code path doesn't call cancelScrollAnimation(), is that intended? Perhaps programmaticScrollHelper should take a scrollType? When I added it there was just user and programmatic scrolls.
https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:187: case ClampingScroll: On 2016/10/13 18:50:56, szager1 wrote: > This code path doesn't call cancelScrollAnimation(), is that intended? Basically, yes. I assumed that clamping cancelling animation was an unintended side effect of reusing the ProgrammaticScroll value. I can't think of a scenario where it is important for clamping to cancel animation.
lgtm https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2404393003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:187: case ClampingScroll: On 2016/10/13 18:55:19, skobes wrote: > On 2016/10/13 18:50:56, szager1 wrote: > > This code path doesn't call cancelScrollAnimation(), is that intended? > > Basically, yes. I assumed that clamping cancelling animation was an unintended > side effect of reusing the ProgrammaticScroll value. I can't think of a > scenario where it is important for clamping to cancel animation. I haven't thought too deeply about this, but my instinct is that whenever a discontinuous scroll happens (including programmatic and clamping scrolls), we should cancel animation. Scroll anchoring could be a legitimate exception to that, though.
Hmmm...now that I see the patch, I'm kind of concerned about how we decide which things will flush the scroll anchoring. For example, pageX/pageY do, but clientX/clientY and getBoundingClientRect don't? We need to find something more principled here I think.
On 2016/10/13 19:17:40, ojan wrote:
> Hmmm...now that I see the patch, I'm kind of concerned about how we decide
which
> things will flush the scroll anchoring. For example, pageX/pageY do, but
> clientX/clientY and getBoundingClientRect don't?
>
> We need to find something more principled here I think.
The principle I followed is "flush the scroll anchoring if you're specifically
asking for a scroll position, but not if you're just asking for layout
information that might depend on it". I'm flushing in DOMVisualViewport because
window.scroll{X/Y} actually delegates to visualViewport.page{X/Y}.
I suppose we could have a principle of "flush for anything that depends on
layout", but then we might as well not even have this patch?
Stefan and I talked about it over lunch today and it's hard for me to see any halfway solutions that authors could reasonably understand. So, I think that leaves us with 3 choices: 1. The current tip of tree behavior. 2. The behavior in this patch, except we only apply anchoring between layout and paint in update lifecycle phases. 3. The behavior in this patch, except we apply anchoring whenever updating layout (i.e. in updateStyleAndLayout. #1 and #3 are roughly equivalent, although the former might be exposed to custom layout code. #2 has the problem of some APIs giving the wrong answer (e.g. getBoundingClientRect). From my perspective at the moment, I think we should do #3. From a code health perspective, I think it has better separation of concerns than #1 and doesn't result in the guaranteed flicker that #2 does. Conceptually, you save all anchors before layout starts. Do layout. Then anything that would read position/size information would apply the anchoring, including the paint step of the rendering pipeline. In practice, this patch saves the scroll anchors in the middle of layout instead of before, but the behavior is the same, right? It's just a performance optimization? WDYT?
On 2016/10/13 22:55:28, ojan wrote: > From my perspective at the moment, I think we should do #3. From a code health > perspective, I think it has better separation of concerns than #1 and doesn't > result in the guaranteed flicker that #2 does. I don't quite follow the "guaranteed flicker" bit? > In practice, this patch saves the scroll anchors in the middle of layout instead > of before, but the behavior is the same, right? It's just a performance > optimization? Right, the behavior should be as if we saved at the end of the previous frame. > WDYT? I am fine with any of the options. #3 would mean this patch is just a code health refactoring with no behavior change. Will that satisfy the concerns Microsoft raised at TPAC?
Description was changed from ========== Tie scroll anchoring adjustments to frame lifecycle instead of layout. This applies suppressions to the entire animation frame, which lets us spec the behavior without referring to layout passes. ScrollAnchor::notifyBeforeLayout, formerly known as save(), adds the scroller to a queue owned by the FrameView, which iterates the queue to do the scroll adjustments after layout, but before the compositing update. Web APIs like scrollTop that read the current scroll position force these adjustments to happen synchronously. BUG=648780 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Tie scroll anchoring adjustments to frame lifecycle instead of layout. This applies suppressions to the entire animation frame, which lets us spec the behavior without referring to layout passes. ScrollAnchor::notifyBeforeLayout, formerly known as save(), adds the scroller to a queue owned by the FrameView, which iterates the queue to do the scroll adjustments after layout, but before the compositing update. Web APIs that force synchronous layout also force these adjustments to happen synchronously. This means it is impossible for script to observe an un-adjusted scroll position after an anchor node movement. BUG=648780 ==========
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Updated for option #3, PTAL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Code is lgtm if y'all are happy with the behavior. https://codereview.chromium.org/2404393003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2404393003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:247: m_scroller->isRootFrameViewport() Override ScrollableArea::scrollAnchor in RFV to return the layoutViewport().scrollAnchor(). Then this can just be m_scroller->scrollAnchor().
https://codereview.chromium.org/2404393003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2404393003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:247: m_scroller->isRootFrameViewport() On 2016/10/27 15:41:48, bokan wrote: > Override ScrollableArea::scrollAnchor in RFV to return the > layoutViewport().scrollAnchor(). Then this can just be > m_scroller->scrollAnchor(). That raises the question of what ScrollableArea::scrollAnchor actually means - is it "the ScrollAnchor that scrolls me", or "the ScrollAnchor I own"? Either way, FrameView and RootFrameViewport can't both return the same ScrollAnchor if we want consistent semantics.
https://codereview.chromium.org/2404393003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right): https://codereview.chromium.org/2404393003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:247: m_scroller->isRootFrameViewport() On 2016/10/27 16:57:20, skobes wrote: > On 2016/10/27 15:41:48, bokan wrote: > > Override ScrollableArea::scrollAnchor in RFV to return the > > layoutViewport().scrollAnchor(). Then this can just be > > m_scroller->scrollAnchor(). > > That raises the question of what ScrollableArea::scrollAnchor actually means - > is it "the ScrollAnchor that scrolls me", or "the ScrollAnchor I own"? Either > way, FrameView and RootFrameViewport can't both return the same ScrollAnchor if > we want consistent semantics. Good point I hadn't thought of that. It might clearer if ScrollAnchor was on the heap and we could move it to RFV in this case. That way, ownership and "scrolls me" are equivalent. We could also then just enqueue ScrollAnchors rather than areas. In any case, just a thought. lgtm as is.
One crash concern, lgtm if I'm reading the code wrong. https://codereview.chromium.org/2404393003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2404393003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2795: scroller->scrollAnchor()->adjust(); It looks like it's possible for the scroll anchor to be cleared, but not removed from the queue. Am I reading that wrong? I think we could null-check scrollAnchor or remove from the queue when the anchor is cleared. The former might be cheaper.
Description was changed from ========== Tie scroll anchoring adjustments to frame lifecycle instead of layout. This applies suppressions to the entire animation frame, which lets us spec the behavior without referring to layout passes. ScrollAnchor::notifyBeforeLayout, formerly known as save(), adds the scroller to a queue owned by the FrameView, which iterates the queue to do the scroll adjustments after layout, but before the compositing update. Web APIs that force synchronous layout also force these adjustments to happen synchronously. This means it is impossible for script to observe an un-adjusted scroll position after an anchor node movement. BUG=648780 ========== to ========== Tie scroll anchoring adjustments to frame lifecycle instead of layout. This applies suppressions to the entire animation frame, which lets us spec the behavior without referring to layout passes. ScrollAnchor::notifyBeforeLayout, formerly known as save(), adds the scroller to a queue owned by the FrameView, which iterates the queue to do the scroll adjustments after layout, but before the compositing update. Web APIs that force synchronous layout also force these adjustments to happen synchronously. This means it is impossible for script to observe an un-adjusted scroll position after an anchor node movement. BUG=648780 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
https://codereview.chromium.org/2404393003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2404393003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2795: scroller->scrollAnchor()->adjust(); On 2016/10/27 19:02:24, ojan wrote: > It looks like it's possible for the scroll anchor to be cleared, but not removed > from the queue. Am I reading that wrong? > > I think we could null-check scrollAnchor or remove from the queue when the > anchor is cleared. The former might be cheaper. A cleared ScrollAnchor still exists as a ScrollAnchor object. All overrides of ScrollableArea::scrollAnchor guarantee non-nullness. Added a DCHECK to be safe.
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, szager@chromium.org, bokan@chromium.org, ojan@chromium.org Link to the patchset: https://codereview.chromium.org/2404393003/#ps140001 (title: "add DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
skobes@chromium.org changed reviewers: + eae@chromium.org
+eae for platform/OWNERS
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
RS LGTM
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Tie scroll anchoring adjustments to frame lifecycle instead of layout. This applies suppressions to the entire animation frame, which lets us spec the behavior without referring to layout passes. ScrollAnchor::notifyBeforeLayout, formerly known as save(), adds the scroller to a queue owned by the FrameView, which iterates the queue to do the scroll adjustments after layout, but before the compositing update. Web APIs that force synchronous layout also force these adjustments to happen synchronously. This means it is impossible for script to observe an un-adjusted scroll position after an anchor node movement. BUG=648780 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Tie scroll anchoring adjustments to frame lifecycle instead of layout. This applies suppressions to the entire animation frame, which lets us spec the behavior without referring to layout passes. ScrollAnchor::notifyBeforeLayout, formerly known as save(), adds the scroller to a queue owned by the FrameView, which iterates the queue to do the scroll adjustments after layout, but before the compositing update. Web APIs that force synchronous layout also force these adjustments to happen synchronously. This means it is impossible for script to observe an un-adjusted scroll position after an anchor node movement. BUG=648780 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/e0e3af8004e7f656ae8e1cb45c76a4a013c43961 Cr-Commit-Position: refs/heads/master@{#428155} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e0e3af8004e7f656ae8e1cb45c76a4a013c43961 Cr-Commit-Position: refs/heads/master@{#428155} |
