Force compositing inputs to be clean for getBoundingClientRect (issue 2647533002 by smcgruer@chromium.org)

[smcg...@chromium.org]

Reviewers: chrishtr, flackr
CL: https://codereview.chromium.org/2647533002/

Message:
I suspect this may be overkill for the purpose of getting the compositing inputs
updated for getBoundingClientRect, but at the very least it works and we can
iterate from here :)

Description:
Force compositing inputs to be clean for getBoundingClientRect

This is necessary to have the StickyPositionScrollingConstraints setup
when getBoundingClientRect is called immediately after the page is
changed. See the bug for more details.

BUG=672457

Affected files (+3, -0 lines):
M third_party/WebKit/Source/core/dom/Element.cpp


Index: third_party/WebKit/Source/core/dom/Element.cpp
diff --git a/third_party/WebKit/Source/core/dom/Element.cpp b/third_party/WebKit/Source/core/dom/Element.cpp
index d5ea539ee3527b4a3f90bb8e3b32abb76b499bdf..9372ccbaad52318f96d6660172732539e012025e 100644
--- a/third_party/WebKit/Source/core/dom/Element.cpp
+++ b/third_party/WebKit/Source/core/dom/Element.cpp
@@ -1120,6 +1120,9 @@ IntRect Element::visibleBoundsInVisualViewport() const {

void Element::clientQuads(Vector<FloatQuad>& quads) {
document().updateStyleAndLayoutIgnorePendingStylesheetsForNode(this);
+ FrameView* view = document().view();
+ if (view)
+ view->updateLifecycleToCompositingCleanPlusScrolling();

LayoutObject* elementLayoutObject = layoutObject();
if (!elementLayoutObject)

[fla...@chromium.org]

Good CL for discussion. One thing we could do to limit the perf impact is have a
flag for whether we have an ancestor sticky position element.


https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp#newcode1123
third_party/WebKit/Source/core/dom/Element.cpp:1123: FrameView* view =
document().view();
nit: We tend to use the following style to scope conditionally used
optional variables
if (FrameView* view = document().view())
... code which uses view

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp#newcode1125
third_party/WebKit/Source/core/dom/Element.cpp:1125:
view->updateLifecycleToCompositingCleanPlusScrolling();
Was this call necessary? Does updateAllLifecyclePhasesExceptPaint not
get the lifecycle past compositing inputs?

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp#newcode1123
third_party/WebKit/Source/core/dom/Element.cpp:1123: FrameView* view =
document().view();

On 2017/01/18 21:11:10, flackr wrote:
> nit: We tend to use the following style to scope conditionally used
optional
> variables
> if (FrameView* view = document().view())
> ... code which uses view

Done.

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp#newcode1125
third_party/WebKit/Source/core/dom/Element.cpp:1125:
view->updateLifecycleToCompositingCleanPlusScrolling();

On 2017/01/18 21:11:10, flackr wrote:
> Was this call necessary? Does updateAllLifecyclePhasesExceptPaint not
get the
> lifecycle past compositing inputs?

I was aiming for minimal lifecycle update.
updateAllLifecyclePhasesExceptPaint() should work the same, except it
will also do the following after CompositingClean:

InPaintInvalidation,
PaintInvalidationClean,
InPrePaint,
PrePaintClean,

https://codereview.chromium.org/2647533002/

[fla...@chromium.org]

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp#newcode1125
third_party/WebKit/Source/core/dom/Element.cpp:1125:
view->updateLifecycleToCompositingCleanPlusScrolling();

On 2017/01/18 21:17:13, smcgruer wrote:
> On 2017/01/18 21:11:10, flackr wrote:
> > Was this call necessary? Does updateAllLifecyclePhasesExceptPaint
not get the
> > lifecycle past compositing inputs?
>
> I was aiming for minimal lifecycle update.
updateAllLifecyclePhasesExceptPaint()
> should work the same, except it will also do the following after
> CompositingClean:
>
> InPaintInvalidation,
> PaintInvalidationClean,
> InPrePaint,
> PrePaintClean,
>

Oops, you are right, this function seems to advance the lifecycle just
past the compositing update which should get the inputs clean - maybe
also DCHECK this?

https://codereview.chromium.org/2647533002/

[chri...@chromium.org]

On 2017/01/18 at 21:11:10, flackr wrote:
> Good CL for discussion. One thing we could do to limit the perf impact is have
a flag for whether we have an ancestor sticky position element.
>
>

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
>
>

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp#newcode1123
> third_party/WebKit/Source/core/dom/Element.cpp:1123: FrameView* view =
document().view();

> nit: We tend to use the following style to scope conditionally used optional
variables
> if (FrameView* view = document().view())
> ... code which uses view
>
>

https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp#newcode1125
> third_party/WebKit/Source/core/dom/Element.cpp:1125:
view->updateLifecycleToCompositingCleanPlusScrolling();

> Was this call necessary? Does updateAllLifecyclePhasesExceptPaint not get the
lifecycle past compositing inputs?

updateAllLifecyclePhasesExceptPaint does in fact do that.

https://codereview.chromium.org/2647533002/

[chri...@chromium.org]

On 2017/01/18 at 21:11:10, flackr wrote:
> Good CL for discussion. One thing we could do to limit the perf impact is have
a flag for whether we have an ancestor sticky position element.

I think this bit will be necessary. This is performance-critical code.

>
>
https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
>
>
https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp#newcode1123
> third_party/WebKit/Source/core/dom/Element.cpp:1123: FrameView* view =
document().view();
> nit: We tend to use the following style to scope conditionally used optional
variables
> if (FrameView* view = document().view())
> ... code which uses view
>
>
https://codereview.chromium.org/2647533002/diff/1/third_party/WebKit/Source/core/dom/Element.cpp#newcode1125
> third_party/WebKit/Source/core/dom/Element.cpp:1125:
view->updateLifecycleToCompositingCleanPlusScrolling();
> Was this call necessary? Does updateAllLifecyclePhasesExceptPaint not get the
lifecycle past compositing inputs?

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

One example of how we could detect being in a position:sticky subtree (code is
quite messy, apologies).

I'm open to other ideas. This approach aims to avoid adding anything to
LayoutObject, at the cost of doing a walk to the next LayoutBoxModelObject above
us. (Unsure if the cost of that walk is ever more than 1.)

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

Note: A similar approach is needed in at least FrameSelection::revealSelection
for issue 677035. There I have yet to work out how to get to the correct
LayoutBoxModelObject, but I actually think we might want to take a step back
first and think about how many and which places now need to clean compositing
input as well as do layout to make sure that the location of boxes is correct.

https://codereview.chromium.org/2647533002/

[chri...@chromium.org]

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1132
third_party/WebKit/Source/core/dom/Element.cpp:1132:
view->updateLifecycleToCompositingCleanPlusScrolling();
Hmm. I think this will be incorrect if layout/style state is dirty and
updating layout/style will
result in a change to isInStickySubtree(). Also, on reflection, the
logic here to avoid
updating to compositing clean plus scrolling only has a significant
effect if the developer
dirtied style/layout, then called clientQuads before yielding, because
otherwise Blink would update
the entire lifecycle before coming back to script.

Now I'm not sure if this is important to optimize. And also detecting
whether there is any stick
positioned ancestor is not so easy without extra code to store more
dirty bits (your CL incurs a
tree walk up to the root every time clientQuads is called).

Now I think we should just land this patch without the conditional here.

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1132
third_party/WebKit/Source/core/dom/Element.cpp:1132:
view->updateLifecycleToCompositingCleanPlusScrolling();

On 2017/01/19 21:29:19, chrishtr wrote:
> Hmm. I think this will be incorrect if layout/style state is dirty and
updating
> layout/style will
> result in a change to isInStickySubtree(). Also, on reflection, the
logic here
> to avoid
> updating to compositing clean plus scrolling only has a significant
effect if
> the developer
> dirtied style/layout, then called clientQuads before yielding, because
otherwise
> Blink would update
> the entire lifecycle before coming back to script.
>
> Now I'm not sure if this is important to optimize. And also detecting
whether
> there is any stick
> positioned ancestor is not so easy without extra code to store more
dirty bits
> (your CL incurs a
> tree walk up to the root every time clientQuads is called).
>
> Now I think we should just land this patch without the conditional
here.

I think those are all fair points. I'm happy to return this to the
unconditional approach.

Any thoughts on measuring any possible performance impact?

https://codereview.chromium.org/2647533002/

[fla...@chromium.org]

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1132
third_party/WebKit/Source/core/dom/Element.cpp:1132:
view->updateLifecycleToCompositingCleanPlusScrolling();

We could certainly measure how often we actually don't already have
fresh values, and/or how long it takes to update, but even with
microbenchmark data it's not clear to me if we would know the overall
impact. I think we can rely on telemetry pagesets catching if this
regresses performance overall in the wild, although I agree with Chris
that it's likely going to be clean most of the time and repeated calls
will only incur the cost once unless they dirty something which is a
known bad pattern.

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

Current version does fail fast/dom/forced-layout-only-in-document.html, havent
checked why yet (a task for tomorrow!)

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/40001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1132
third_party/WebKit/Source/core/dom/Element.cpp:1132:
view->updateLifecycleToCompositingCleanPlusScrolling();

Ack, uploaded new PS that takes the code back to PS2.

https://codereview.chromium.org/2647533002/

[fla...@chromium.org]

Please add some tests. Also, we should ensure the other common methods which
depend on the correct position have clean inputs as well (i.e. offsetTop,
offsetLeft, scrollIntoView).


https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1130
third_party/WebKit/Source/core/dom/Element.cpp:1130:
view->updateLifecycleToCompositingCleanPlusScrolling();
I suspect we'll need to do this for a lot of other methods, maybe we
should add a private helper that ensures clean compositing inputs. With
this here we don't need to updateStyleAndLayout above do we?

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/80001/third_party/WebKit/Source/core/dom/Element.cpp#newcode1130
third_party/WebKit/Source/core/dom/Element.cpp:1130:
view->updateLifecycleToCompositingCleanPlusScrolling();

On 2017/01/20 02:47:02, flackr wrote:
> maybe we should add a private helper that ensures clean compositing
inputs.

Done. I think it could also be just an anon function if that is
preferential.

> With this here we don't need to updateStyleAndLayout above do we?

I'm unsure. It appears to work, but I don't know the full impact of
doing this. Chris?

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

Note: I still need to add some tests that actually scroll and make sure that
everything remains good/etc. I'm currently trying to decide if these should be
LayoutTests or if there's a nicer (but still covering the same logic) unit-test
way to do it.

https://codereview.chromium.org/2647533002/

[chri...@chromium.org]

If you don't re-add the previous call to updateStyle...IgnorePendingStyleSheets,
you will regress the situation where there is a pending style sheet that
previously was not going to be updated, but will now. If you just add a call to
updateLifecycleToCompositingCleanPlusScrolling
after the existing call, it will do the right thing, because it starts with a
LayoutClean state.

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

Ack. We had better leave it then :)

Sounds like the only thing left to do then is to add some LayoutTests (or some
other sort of JS test?) to make sure that position:sticky element positions work
now.

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

LayoutTests added. PTAL.

https://codereview.chromium.org/2647533002/

[fla...@chromium.org]

Looking good, just a few test suggestions.


https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html
(right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html#newcode54
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html:54:
scroller.append(document.createElement('div'));
Can we have a test which inserts the sticky element and immediately
queries it?

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html#newcode65
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html:65:
}, "offsetLeft should be correct for sticky after script-caused
layout");
offsetTop and offsetLeft could probably be a single test.

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/Element.cpp#newcode4129
third_party/WebKit/Source/core/dom/Element.cpp:4129:
document().updateStyleAndLayoutIgnorePendingStylesheets();
It's probably worth adding a comment for why we need to do this despite
updating the lifecycle right after.

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp#newcode90
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();
It might be worth unit testing that the correct offset is returned here,
maybe after expecting CompositingClean, assert that the sticky
LayoutBoxModelObject no longer needs compositing inputs update and that
the original offset matches the new offset? i.e.

// Modify DOM, expect compositing inputs to be dirty.
EXPECT_EQ(true, LBMO->needsCompositingInputsUpdate());
offset = Element->offsetLeft();
EXPECT_EQ(false, LBMO->needsCompositingInputsUpdate());
EXPECT_EQ(DocumentLifeCycle::CompositingClean, ...)
EXPECT_EQ(Element->offsetLeft(), offset);

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html
(right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html#newcode54
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html:54:
scroller.append(document.createElement('div'));

On 2017/01/23 16:43:50, flackr wrote:
> Can we have a test which inserts the sticky element and immediately
queries it?

Fantastic idea. This actually helped expose that my fix for
offsetTop/offsetLeft was wrong; the actual JS bindings end up calling
HTMLElement::offsetTopForBinding, which has almost exactly the same
implementation but isn't an override... I intend to pursue making that
situation go away, but in the meantime I added the same refresh code to
HTMLElement::offsetTopForBinding.

Oh, and added an insert test.

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html#newcode65
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-valid-after-layout.html:65:
}, "offsetLeft should be correct for sticky after script-caused
layout");

On 2017/01/23 16:43:50, flackr wrote:
> offsetTop and offsetLeft could probably be a single test.

Done.

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/Element.cpp#newcode4129
third_party/WebKit/Source/core/dom/Element.cpp:4129:
document().updateStyleAndLayoutIgnorePendingStylesheets();

On 2017/01/23 16:43:50, flackr wrote:
> It's probably worth adding a comment for why we need to do this
despite updating
> the lifecycle right after.

Done.

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp#newcode90
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();

On 2017/01/23 16:43:50, flackr wrote:
> It might be worth unit testing that the correct offset is returned
here, maybe
> after expecting CompositingClean, assert that the sticky
LayoutBoxModelObject no
> longer needs compositing inputs update and that the original offset
matches the
> new offset? i.e.
>
> // Modify DOM, expect compositing inputs to be dirty.
> EXPECT_EQ(true, LBMO->needsCompositingInputsUpdate());
> offset = Element->offsetLeft();
> EXPECT_EQ(false, LBMO->needsCompositingInputsUpdate());
> EXPECT_EQ(DocumentLifeCycle::CompositingClean, ...)
> EXPECT_EQ(Element->offsetLeft(), offset);

Tried this, however for reasons I don't yet understand, the following
failed:

padding->setInnerHTML("<div id='test'></div>");
EXPECT_EQ(DocumentLifecycle::VisualUpdatePending,
document.lifecycle().state());


EXPECT_TRUE(sticky->layoutBoxModelObject()->layer()->needsCompositingInputsUpdate());
<-- this is false, not true.
int offsetTop = sticky->offsetTop();

Not sure why, but setInnerHTML (on anything, I tried it on the sticky
element as well as the padding) doesn't cause anything to flip the LMBO
to needsCompositingInputsUpdate().

https://codereview.chromium.org/2647533002/

[fla...@chromium.org]

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp#newcode90
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();

I think I know what's happening. This normally gets set when the layout
within the scrollable area changes by
PaintLayerScrollableArea::invalidateAllStickyConstraints:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp?q=setneedsCompositingInputsUpdate&sq=package:chromium&l=1515&dr=C

But layout is probably still dirty at this point.

https://codereview.chromium.org/2647533002/

[fla...@chromium.org]

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp#newcode90
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();

I guess remove the assertion that compositing inputs are dirty before
the change and just assert afterwards that inputs are clean + a position
dependent on the layout change.

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp#newcode90
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();

Added a partially-complete test to make sure that I understand what
you're looking for. Please ack the test and let me know if its not what
you were thinking of.

https://codereview.chromium.org/2647533002/

[fla...@chromium.org]

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/110001/third_party/WebKit/Source/core/dom/ElementTest.cpp#newcode90
third_party/WebKit/Source/core/dom/ElementTest.cpp:90:
document.body()->offsetLeft();

Acknowledged. That's exactly what I was suggesting.

https://codereview.chromium.org/2647533002/diff/150001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/150001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html#newcode54
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:54:
sticky.scrollIntoView();
Did we need to change layout before scrolling into view to test that it
gets the correct new constraints?

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

https://codereview.chromium.org/2647533002/diff/150001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/150001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html#newcode54
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:54:
sticky.scrollIntoView();

On 2017/01/24 20:22:28, flackr wrote:
> Did we need to change layout before scrolling into view to test that
it gets the
> correct new constraints?

Probably a good idea, done.

https://codereview.chromium.org/2647533002/

[fla...@chromium.org]

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html#newcode58
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:58:
scroller.append(newDiv);
nit: insert it before the sticky element here and below so that the old
constraints would produce the wrong answer.

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Source/core/dom/Element.cpp#newcode4129
third_party/WebKit/Source/core/dom/Element.cpp:4129: // NOTE(smcgruer):
|updateLifecycleToCompositingCleanPlusScrolling| below
I don't think NOTE(author) is common chromium style, just the comment is
good.

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Source/core/dom/ElementTest.cpp
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Source/core/dom/ElementTest.cpp#newcode103
third_party/WebKit/Source/core/dom/ElementTest.cpp:103: "#sticky {
height: 25px; position: sticky; top: 0; left: 25px }"
nit: semicolon after 25px

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Source/core/dom/ElementTest.cpp#newcode136
third_party/WebKit/Source/core/dom/ElementTest.cpp:136:
EXPECT_EQ(offsetTop, sticky->offsetTop());
Hm, I'm not sure what case I was worried about with ensuring the
repeated call returns the same value.

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html#newcode58
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:58:
scroller.append(newDiv);

On 2017/01/24 22:47:15, flackr wrote:
> nit: insert it before the sticky element here and below so that the
old
> constraints would produce the wrong answer.

Done. As usual, your offhand comments cause me to discover big new bugs,
so this also prompted a fix in PaintLayerScrollableArea so that we
properly invalidate the constraints on a scroller itself as well as its
ancestor overlay.

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Source/core/dom/Element.cpp#newcode4129
third_party/WebKit/Source/core/dom/Element.cpp:4129: // NOTE(smcgruer):
|updateLifecycleToCompositingCleanPlusScrolling| below

On 2017/01/24 22:47:16, flackr wrote:
> I don't think NOTE(author) is common chromium style, just the comment
is good.

Done.

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Source/core/dom/ElementTest.cpp
File third_party/WebKit/Source/core/dom/ElementTest.cpp (right):

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Source/core/dom/ElementTest.cpp#newcode103
third_party/WebKit/Source/core/dom/ElementTest.cpp:103: "#sticky {
height: 25px; position: sticky; top: 0; left: 25px }"

On 2017/01/24 22:47:16, flackr wrote:
> nit: semicolon after 25px

Done.

https://codereview.chromium.org/2647533002/diff/170001/third_party/WebKit/Source/core/dom/ElementTest.cpp#newcode136
third_party/WebKit/Source/core/dom/ElementTest.cpp:136:
EXPECT_EQ(offsetTop, sticky->offsetTop());

On 2017/01/24 22:47:16, flackr wrote:
> Hm, I'm not sure what case I was worried about with ensuring the
repeated call
> returns the same value.

Ok, removed.

https://codereview.chromium.org/2647533002/

[chri...@chromium.org]

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Source/core/dom/Element.cpp#newcode4129
third_party/WebKit/Source/core/dom/Element.cpp:4129:
document().updateStyleAndLayoutIgnorePendingStylesheets();
Previously it was
updateStyleAndLayoutIgnorePendingStylesheetsForNode(this) instead.
Why the change?

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Source/core/dom/Element.cpp#newcode4129
third_party/WebKit/Source/core/dom/Element.cpp:4129:
document().updateStyleAndLayoutIgnorePendingStylesheets();

On 2017/01/25 21:36:28, chrishtr wrote:
> Previously it was
updateStyleAndLayoutIgnorePendingStylesheetsForNode(this)
> instead.
> Why the change?

All updateStyleAndLayoutIgnorePendingStylesheetsForNode(node) does is
early-exit on !node->inActiveDocument(), and then call
updateStyleAndLayoutIgnorePendingStylesheets(). I already do the
early-exit check above, so it seemed superfluous, but I can change it
back if you'd prefer.

https://codereview.chromium.org/2647533002/

[fla...@chromium.org]

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html#newcode105
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:105:
assert_equals(scroller.scrollTop, 195);
Shouldn't this be 220 (150px paddingBefore + 70px writer)?

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html#newcode105
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:105:
assert_equals(scroller.scrollTop, 195);

On 2017/01/25 21:48:18, flackr wrote:
> Shouldn't this be 220 (150px paddingBefore + 70px writer)?

No; scrollIntoViewIfNeeded attempts to center the target element. So
it's (150 + 70 - 25).

https://codereview.chromium.org/2647533002/

[fla...@chromium.org]

LGTM

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html
(right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html#newcode105
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-position-works-with-scroll-apis.html:105:
assert_equals(scroller.scrollTop, 195);

On 2017/01/25 21:50:19, smcgruer wrote:
> On 2017/01/25 21:48:18, flackr wrote:
> > Shouldn't this be 220 (150px paddingBefore + 70px writer)?
>
> No; scrollIntoViewIfNeeded attempts to center the target element. So
it's (150 +
> 70 - 25).

Ah, thanks for explanation!

https://codereview.chromium.org/2647533002/

[chri...@chromium.org]

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Source/core/dom/Element.cpp
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2647533002/diff/230001/third_party/WebKit/Source/core/dom/Element.cpp#newcode4129
third_party/WebKit/Source/core/dom/Element.cpp:4129:
document().updateStyleAndLayoutIgnorePendingStylesheets();

On 2017/01/25 at 21:39:05, smcgruer wrote:
> On 2017/01/25 21:36:28, chrishtr wrote:
> > Previously it was
updateStyleAndLayoutIgnorePendingStylesheetsForNode(this)
> > instead.
> > Why the change?
>
> All updateStyleAndLayoutIgnorePendingStylesheetsForNode(node) does is
early-exit on !node->inActiveDocument(), and then call
updateStyleAndLayoutIgnorePendingStylesheets(). I already do the
early-exit check above, so it seemed superfluous, but I can change it
back if you'd prefer.

Oh ok.

https://codereview.chromium.org/2647533002/

[chri...@chromium.org]

(defer to flackr on rest of review)

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

flackr has LGTM'd, but I still need an LGTM from someone with Blink OWNERS.

https://codereview.chromium.org/2647533002/

[chri...@chromium.org]

lgtm



https://codereview.chromium.org/2647533002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2647533002/230001


https://codereview.chromium.org/2647533002/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/371303)

https://codereview.chromium.org/2647533002/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,

http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/371469)

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

Ok, seems this patch changes something relating to animations, in such a way
that only a single test caught it -
inspector-protocol/animation/animation-empty-transition-cancel.html

I'm still digging, but before this CL the animation would be created and then
cancelled immediately due to this series of calls:

node.offsetTop;
node.style.transition = "1s";
node.offsetTop;
node.style.width = "200px";
node.offsetTop;
node.style.transition = "";
node.offsetTop;

The animation would *not* be started.

After this CL, the animation is created *and* started, and never cancelled.

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

Found the root cause. InspectorAnimationAgent::animationPlayStateChanged will
not trigger an 'animationCancelled' event if it has ever seen the animation
started. (Because it early exits on 'm_idToAnimation.get(animationId)' being
true. An animation started event causes
'InspectorAnimationAgent::buildObjectForAnimation' to be called, which adds the
elementId to that hashmap.)

I'm unclear if the behavior is deliberate or not. It looks to have been
introduced in https://codereview.chromium.org/1422713012 if I'm reading the code
correctly, and it certainly doesn't look deliberate to me in that CL.

+samli since he wrote much of the InspectorAnimationAgent code.

Sam, was this intended behavior? For context, this CL changes the lifecycle
behavior of offsetTop/etc to also run compositing. That then had the knock-on
effect of causing the animation in
inspector-protocol/animation/animation-empty-transition-cancel.html to start
running before it is cancelled (due to a call to
Animation::notifyCompositorStartTime) which then triggered this above described
behavior in InspectorAnimationAgent::animationPlayStateChanged.

https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

FYI with the following diff the test passes, though of course I don't know the
intended behavior in InspectorAnimationAgent.cpp :)

diff --git
a/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
b/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
index 081bdb56fff1..c71f3ba9f8a8 100644
--- a/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
+++ b/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
@@ -500,11 +500,11 @@ void InspectorAnimationAgent::animationPlayStateChanged(
blink::Animation::AnimationPlayState oldPlayState,
blink::Animation::AnimationPlayState newPlayState) {
const String& animationId = String::number(animation->sequenceNumber());
- if (m_idToAnimation.get(animationId) ||
- m_clearedAnimations.contains(animationId))
+ if (m_clearedAnimations.contains(animationId))
return;
- if (newPlayState == blink::Animation::Running ||
- newPlayState == blink::Animation::Finished)
+ if ((newPlayState == blink::Animation::Running ||
+ newPlayState == blink::Animation::Finished) &&
+ !m_idToAnimation.contains(animationId))
frontend()->animationStarted(buildObjectForAnimation(*animation));
else if (newPlayState == blink::Animation::Idle ||
newPlayState == blink::Animation::Paused)


https://codereview.chromium.org/2647533002/

[smcg...@chromium.org]

It appears that samli@ is no longer active in Chromium. Adding a few other
people who have touched InspectorAnimationAgent semi-recently, at the very least
hoping that they can point me in the right direction of whoever owns it now :)

suzyh, kozyatinskiy, please see comments above regarding what I think is wrong
with InspectorAnimationAgent and how it is blocking this CL. Thanks.

https://codereview.chromium.org/2647533002/

[sa...@chromium.org]

Diff looks fine to me.

https://codereview.chromium.org/2647533002/

[su...@chromium.org]

On 2017/01/27 at 13:05:36, smcgruer wrote:
> It appears that samli@ is no longer active in Chromium. Adding a few other
people who have touched InspectorAnimationAgent semi-recently, at the very least
hoping that they can point me in the right direction of whoever owns it now :)
>
> suzyh, kozyatinskiy, please see comments above regarding what I think is wrong
with InspectorAnimationAgent and how it is blocking this CL. Thanks.

Apologies for the delay in responding! Thanks for sending that
InspectorAnimationAgent CL through. I'm not very familiar with
InspectorAnimationAgent but am happy to chase up if you encounter any further
issues.

https://codereview.chromium.org/2647533002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2647533002/250001


https://codereview.chromium.org/2647533002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #14 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/2c04a7850056a21b6c1dcb6d63451c4a0f11e707

https://codereview.chromium.org/2647533002/

[joh...@chromium.org]

A revert of this CL (patchset #14 id:250001) has been created in
https://codereview.chromium.org/2667003003/ by joh...@chromium.org.

The reason for reverting is: 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.

https://codereview.chromium.org/2647533002/