It sounds like we may not be forcing the compositing inputs to be clean before trying to get the bounding client rect which means it doesn't have the StickyPositionScrollingConstraints set up yet and so we don't know how to compute the sticky position until the next frame. We should be able to fix this by forcing compositing inputs to be clean (in addition to forcing clean layout) before returning bounding client rects.

Stephen, can you take a look at this? The sticky position offset is calculated in  LayoutBoxModelObject::stickyPositionOffset. I suspect that it is returning (0, 0) because we don't have a StickyPositionScrollingConstraints in the map on the ScrollingCoordinator yet. This is added by CompositingInputsUpdater::updateRecursive when it calls layer->layoutObject()->updateStickyPositionConstraints().

The analysis in #3 appears correct:

[1:1:1215/155332.880822:INFO:Element.cpp(1132)] getBoundingClientRect for "link"
[1:1:1215/155332.881082:INFO:Document.cpp(2133)] updateStyleAndLayoutIgnorePendingStylesheetsForNode for "link"
[50567:50567:1215/155332.881999:INFO:CONSOLE(46)] "postWriteTop", source: http://output.jsbin.com/lukekot (46)
[1:1:1215/155332.882359:INFO:LayoutBoxModelObject.cpp(978)] LayoutBoxModelObject::stickyPositionOffset, returning offset of 0, 232
[1:1:1215/155332.882830:INFO:LayoutBoxModelObject.cpp(971)] LayoutBoxModelObject::stickyPositionOffset, !scrollableArea->stickyConstraintsMap().contains(layer())
[1:1:1215/155332.883419:INFO:LayoutBoxModelObject.cpp(971)] LayoutBoxModelObject::stickyPositionOffset, !scrollableArea->stickyConstraintsMap().contains(layer())
[1:1:1215/155332.883657:INFO:LayoutBoxModelObject.cpp(971)] LayoutBoxModelObject::stickyPositionOffset, !scrollableArea->stickyConstraintsMap().contains(layer())
[1:1:1215/155332.883856:INFO:Element.cpp(1156)] getBoundingClientRect for "link", result = "72.8906,-212 58.2031x17"
[1:1:1215/155332.894161:INFO:CompositingInputsUpdater.cpp(124)] CompositingInputsUpdater, updateStickyPositionConstraints()

The first two calls to LayoutBoxModelObject::stickyPositionOffset are from the call to 'updateStyleAndLayoutIgnorePendingStylesheetsForNode'. The first one is correct because it happens as part of layout before the sticky constraints are invalidated. They are then invalidated and all future ones return false until the compositor inputs are updated. Only the latter two calls to stickyPositionOffset are from the actual quad calculations.

So I now understand better the ordering of operations here... but unfortunately this hasn't led me any closer to how to get getBoundingClientRect calls to wait for CompositingInputsUpdater :)

 Issue 677035  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c04a7850056a21b6c1dcb6d63451c4a0f11e707

commit 2c04a7850056a21b6c1dcb6d63451c4a0f11e707
Author: smcgruer <smcgruer@chromium.org>
Date: Tue Jan 31 01:49:41 2017

Force compositing inputs to be clean for location-related Element APIs

After layout, the location of position:sticky elements and their descendants
will be incorrect until compositing inputs have been cleaned. In most cases
this is not an issue since Blink will run a full lifecycle after page change,
including compositing. However if the user modifies layout during a script and
then calls a location-based API without yielding, compositing inputs would
still be dirty. This patch corrects that behavior by ensuring that compositing
inputs are clean for location-related APIs.

BUG= 672457 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2647533002
Cr-Commit-Position: refs/heads/master@{#447158}

[add] https://crrev.com/2c04a7850056a21b6c1dcb6d63451c4a0f11e707/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-insertion.html
[add] https://crrev.com/2c04a7850056a21b6c1dcb6d63451c4a0f11e707/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html
[add] https://crrev.com/2c04a7850056a21b6c1dcb6d63451c4a0f11e707/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
[modify] https://crrev.com/2c04a7850056a21b6c1dcb6d63451c4a0f11e707/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/2c04a7850056a21b6c1dcb6d63451c4a0f11e707/third_party/WebKit/Source/core/dom/Element.h
[modify] https://crrev.com/2c04a7850056a21b6c1dcb6d63451c4a0f11e707/third_party/WebKit/Source/core/dom/ElementTest.cpp
[modify] https://crrev.com/2c04a7850056a21b6c1dcb6d63451c4a0f11e707/third_party/WebKit/Source/core/html/HTMLElement.cpp
[modify] https://crrev.com/2c04a7850056a21b6c1dcb6d63451c4a0f11e707/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
[modify] https://crrev.com/2c04a7850056a21b6c1dcb6d63451c4a0f11e707/third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp

[Auto-generated comment by a script] We noticed that this issue is targeted for M-57; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-57 label, otherwise remove Merge-TBD label. Thanks.

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This CL is being reverted in https://codereview.chromium.org/2667003003/ by the build sheriff. Reason for revert is that it broke transitions/unprefixed-transform-origin.html on debug builds (only, release is fine), which I can reproduce locally. Investigating.

transitions/unprefixed-transform-origin.html appears to be a racy test that has previously avoided the race-condition. The test:

1. Sets a transition for 6 elements, calling offsetTop in-between each (to clean layout, I guess).

2. Listens for 'transitionend', and fires a rAF to signal test completion.

3. Expects that before the first rAF returns, all 6 'transitionend' results will be returned.

What is happening now is that the call to offsetTop is triggering compositing clean as well as layout, which takes more time. This means that in debug builds, only 5 'transitionend' events are triggered before the first rAF returns. The last one happens in the next rAF.

Fixing this test is straight-forward; it should count 'transitionend' events and ensure that all targets get one before notifying that the test is done. However, I'm concerned that this may hint at a possible perf regression from my CL, due to the compositing clean. It seems an open question whether the perf hit from doing compositing clean in the cases the CL addresses is more or less expensive than making sticky position calculation part of layout rather than compositing inputs. 


I'll aim to fix the test and re-land the CL, but am looking to see if I can run the perf tests proactively first.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66c36f258e7ef15d44b8fd7d2766943da38cc40c

commit 66c36f258e7ef15d44b8fd7d2766943da38cc40c
Author: johnme <johnme@chromium.org>
Date: Wed Feb 01 17:04:23 2017

Revert of Force compositing inputs to be clean for getBoundingClientRect (patchset #14 id:250001 of https://codereview.chromium.org/2647533002/ )

Reason for revert:
transitions/unprefixed-transform-origin.html has failed on almost every build of WebKit Mac10.11 (dbg) and WebKit Linux Trusty (dbg) since this patch landed.

Each time it's because the transitionend event fails to fire for property -webkit-transform-origin-z.

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=transitions%2Funprefixed-transform-origin.html&testType=webkit_tests

Original issue's description:
> Force compositing inputs to be clean for location-related Element APIs
>
> After layout, the location of position:sticky elements and their descendants
> will be incorrect until compositing inputs have been cleaned. In most cases
> this is not an issue since Blink will run a full lifecycle after page change,
> including compositing. However if the user modifies layout during a script and
> then calls a location-based API without yielding, compositing inputs would
> still be dirty. This patch corrects that behavior by ensuring that compositing
> inputs are clean for location-related APIs.
>
> BUG= 672457 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
>
> Review-Url: https://codereview.chromium.org/2647533002
> Cr-Commit-Position: refs/heads/master@{#447158}
> Committed: https://chromium.googlesource.com/chromium/src/+/2c04a7850056a21b6c1dcb6d63451c4a0f11e707

TBR=chrishtr@chromium.org,flackr@chromium.org,suzyh@chromium.org,kozyatinskiy@chromium.org,smcgruer@chromium.org
NOTRY=true
NOTREECHECKS=true
BUG= 672457 

Review-Url: https://codereview.chromium.org/2667003003
Cr-Commit-Position: refs/heads/master@{#447542}

[delete] https://crrev.com/b742db2fa3101ae54b7de497497b0643cf723ad4/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-insertion.html
[delete] https://crrev.com/b742db2fa3101ae54b7de497497b0643cf723ad4/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html
[delete] https://crrev.com/b742db2fa3101ae54b7de497497b0643cf723ad4/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
[modify] https://crrev.com/66c36f258e7ef15d44b8fd7d2766943da38cc40c/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/66c36f258e7ef15d44b8fd7d2766943da38cc40c/third_party/WebKit/Source/core/dom/Element.h
[modify] https://crrev.com/66c36f258e7ef15d44b8fd7d2766943da38cc40c/third_party/WebKit/Source/core/dom/ElementTest.cpp
[modify] https://crrev.com/66c36f258e7ef15d44b8fd7d2766943da38cc40c/third_party/WebKit/Source/core/html/HTMLElement.cpp
[modify] https://crrev.com/66c36f258e7ef15d44b8fd7d2766943da38cc40c/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
[modify] https://crrev.com/66c36f258e7ef15d44b8fd7d2766943da38cc40c/third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp

 Issue 687589  has been merged into this issue.

And indeed, shortly before the revert landed the perf-bots noticed a regression,  issue 687589 :

https://chromeperf.appspot.com/group_report?bug_id=687589

So 4ms on mean_input_event_latency/https___plus.google.com_110031535020051778989_posts on smoothness.top_25_smooth, a 19% regression.

flackr@, chrishtr@, thoughts on whether this is a price we're willing to pay, or do we need to revisit the CL design? (Or position:sticky design.)

Re comment 21 question: I think we need to understand better why the regression happened. Is that site constantly dirtying style and then causing forced layouts?

I don't understand the analysis of transitions/unprefixed-transform-origin.html in #18. 

The purpose of accessing `offsetTop` was to update style and ensure that the properties had a computed value to allow a transition to occur. Animations generated by style should not be permitted to start until the frame is being committed.

> What is happening now is that the call to offsetTop is triggering compositing clean as well as layout

Could this ever happen before? This sounds like a major change that needs to be considered in the animation system.

> I don't understand the analysis of transitions/unprefixed-transform-origin.html in #18. 

Apologies if I misrepresented it, wasn't intentional :). My main belief was that the goal of the test was to make sure that the animations were created when transition was set to those values (e.g. that both 'transform-origin' and '-webkit-transform-origin*' are understood and propagated to the animation backend). Was it an additional goal of the test to make sure that all 6 end in the same rAF?

> The purpose of accessing `offsetTop` was to update style and ensure that the properties had a computed value to allow a transition to occur. Animations generated by style should not be permitted to start until the frame is being committed.

Main frame, or cc frame?

> Could this ever happen before? This sounds like a major change that needs to be considered in the animation system.

From the script side, I don't think there's any other way to cause compositing clean until this CL. (flackr?)

>  Was it an additional goal of the test to make sure that all 6 end in the same rAF?

Not this test. But if this is the only failure, then animation's tests are lacking.

> Main frame, or cc frame?

In terms of what is observable from script (the document lifecycle).

DocumentAnimations::updateAnimations is likely the function that's being called at a different time now.

>> Not this test. But if this is the only failure, then animation's tests are lacking.

I think it was, yeah. The CL passed the CQ fine, and then the post-commit check only failed that test afaik (see the revert CL).

>> DocumentAnimations::updateAnimations is likely the function that's being called at a different time now.

Yep, you're correct. With this CL, FrameView::updateLifecyclePhasesInternal makes it past the 'if (targetState == DocumentLifecycle::LayoutClean) {' check and performs work including calling into 'view.compositor()->updateIfNeededRecursive();' which then calls DocumentAnimations::updateAnimations. (Or for SPv2, updateLifecyclePhasesInternal calls the animation update directly).

So it appears like animations are kicked off by compositing clean, so this CL would have an effect on that.

Please merge your change to M57 branch 2987 ASAP (latest before 5:00 PM PT today, Wednesday) so we can pick it up for M57 Beta promotion release this week. Thank you.

Removing merge request since CL was reverted. We will likely want to drop ReleaseBlock-Stable and punt this to M58 as well at some point, if there is discussion to happen on animation.

Re #25 and 26: What additional behaviours would you recommend that we test? It sounds like we may have some lifecycle assumptions we're relying on that we aren't enforcing through tests, but it's not clear to me what they are.

Which phase of the lifecycle is currently tied to updating animations? Do we need to add a phase before compositing clean which only updates compositing inputs or perhaps move updating animation to a phase immediately after compositing clean?

I think a new lifecycle phase probably makes sense. I'd like animation update to
be after compositing update. This makes it work better with SPv2 as well.

Currently, PaintLayerCompositor::updateIfNeededRecursiveInternal calls
DocumentAnimations::updateAnimations after updating compositing, but before
updating compositing for frames which are parents of the current frame.

I don't know if this is important or not.

> I think a new lifecycle phase probably makes sense.

Preference on where the lifecycle phase should go? Naively I would put it between CompositingClean and InPaintInvalidation.

> This makes it work better with SPv2 as well.

Currently for SPv2 DocumentAnimations::updateAnimations looks like it happens after PrePaintClean. See FrameView::updateLifecyclePhasesInternal:

if (targetState >= DocumentLifecycle::PrePaintClean) {
...
  if (targetState >= DocumentLifecycle::PrePaintClean)
        prePaint();
}

if (RuntimeEnabledFeatures::slimmingPaintV2Enabled())
      DocumentAnimations::updateAnimations(layoutView()->document());
...

Do we know if this was deliberate?

> Preference on where the lifecycle phase should go? Naively I would put it between CompositingClean and InPaintInvalidation.

Naive attempt to insert a lifecycle here failed; All/PaintLayerPainterTest.DoPaintWithEffectAnimationZeroOpacity/1 crashes in blink::CompositorAnimations::canStartAnimationOnCompositor because element.layoutObject()->paintProperties is null. This could explain why animation update was after PrePaintClean for SPv2.

Next attempt will be to always do AnimationUpdate after PrePaintClean, SPv2 or otherwise.

Re: animations after pre-paint: yes it was deliberate.

I think you should add a lifecycle state that is after Compositing and before
PaintInvalidation for SPv1, and after PrePaint and before Paint for SPv2 /
SPInvalidation.

 Issue 686745  has been merged into this issue.

smcgruer@,
Friendly ping to get an update on this issue marked as Blocker.
Thank you!!

I'm dropping ReleaseBlock-Stable from this. We just keep bumping it from version to version.

I believe our next steps are to:

1. Introduce a new lifecycle phase, either the animation phase discussed in #35 or a compositing inputs update phase that runs before compositing. (Advantage of the latter is that I think it would be the same for SPv1 and SPv2).

2. Determine a way to make running compositing/compositing-inputs performant during location-related APIs. We may have to fall back to a solution where we only run it for elements in a sticky tree, which would be ugly but would solve most performance cases.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb

commit 79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb
Author: smcgruer <smcgruer@chromium.org>
Date: Tue Apr 18 15:47:05 2017

Add a new document lifecycle; CompositingInputsClean

The goal here is to be able to clean the compositing inputs without
running full compositing or starting animations. This is necessary for
properly computing compositing inputs-related properties such as sticky
(see the bug for details on that).

BUG= 672457 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2795263002
Cr-Commit-Position: refs/heads/master@{#465248}

[modify] https://crrev.com/79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb/third_party/WebKit/Source/core/dom/DocumentLifecycle.cpp
[modify] https://crrev.com/79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb/third_party/WebKit/Source/core/dom/DocumentLifecycle.h
[modify] https://crrev.com/79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb/third_party/WebKit/Source/core/frame/FrameView.cpp
[modify] https://crrev.com/79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb/third_party/WebKit/Source/core/frame/FrameView.h
[modify] https://crrev.com/79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb/third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
[modify] https://crrev.com/79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb/third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.h
[add] https://crrev.com/79d50ca3b3326c45c7d041bc9cd8f5abd64b24cb/third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositorTest.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/383d1bb3f30b49fcdf3918cefc588637aa678ec0

commit 383d1bb3f30b49fcdf3918cefc588637aa678ec0
Author: smcgruer <smcgruer@chromium.org>
Date: Fri May 12 04:32:39 2017

Clean compositing inputs for location APIs for sticky-affected elements.

After layout, the location of position:sticky elements and their descendants
will be incorrect until compositing inputs have been cleaned. In most cases
this is not an issue since Blink will run a full lifecycle after page change,
including compositing. However if the user modifies layout during a script and
then calls a location-based API without yielding, compositing inputs would
still be dirty. This patch corrects that behavior by ensuring that compositing
inputs are clean for location-related APIs.

BUG= 672457 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2825343003
Cr-Commit-Position: refs/heads/master@{#471221}

[add] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-insertion.html
[add] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html
[modify] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp
[modify] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/Source/core/dom/DocumentTest.cpp
[modify] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/Source/core/dom/ElementTest.cpp
[modify] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/Source/core/html/HTMLElement.cpp
[modify] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/Source/core/style/ComputedStyle.h
[modify] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/Source/core/style/StyleRareInheritedData.cpp
[modify] https://crrev.com/383d1bb3f30b49fcdf3918cefc588637aa678ec0/third_party/WebKit/Source/core/style/StyleRareInheritedData.h