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

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 3f31f0a9877d3845d5260312a9899c18ed08faaf..97c267ae2eb0575038c397b8177b0268cb7cc36a 100644
--- a/third_party/WebKit/Source/core/dom/Element.cpp
+++ b/third_party/WebKit/Source/core/dom/Element.cpp
@@ -431,7 +431,7 @@ AtomicString Element::LowercaseIfNecessary(const AtomicString& name) const {
 }
 
 void Element::scrollIntoView(bool align_to_top) {
-  GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheetsForNode(this);
+  EnsureLifecycleValidForLocationAPIs();
 
   if (!GetLayoutObject())
     return;
@@ -456,7 +456,7 @@ void Element::scrollIntoView(bool align_to_top) {
 }
 
 void Element::scrollIntoViewIfNeeded(bool center_if_needed) {
-  GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheetsForNode(this);
+  EnsureLifecycleValidForLocationAPIs();
 
   if (!GetLayoutObject())
     return;
@@ -643,7 +643,7 @@ void Element::CallApplyScroll(ScrollState& scroll_state) {
 }
 
 int Element::OffsetLeft() {
-  GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheetsForNode(this);
+  EnsureLifecycleValidForLocationAPIs();
   if (LayoutBoxModelObject* layout_object = GetLayoutBoxModelObject())
     return AdjustLayoutUnitForAbsoluteZoom(
                LayoutUnit(
@@ -654,7 +654,7 @@ int Element::OffsetLeft() {
 }
 
 int Element::OffsetTop() {
-  GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheetsForNode(this);
+  EnsureLifecycleValidForLocationAPIs();

[fs, 2017/04/29 09:59:06]

I think OffsetWidth/Height needs this too (because of pixelsnapping.)

[smcgruer, 2017/05/01 20:15:15]

Done, though I'm having trouble coming up with a unittest that shows this
pixel-snapping.

[fs, 2017/05/01 21:19:34]

Yes, effects would very likely be minor since only the fractional part of the
location would come into play. Could be that fractional positioning is tricky
with 'sticky' (I guess you know that part better than I), but have a look at
SnapSizeToPixel to build an idea of what it'd take to get it to manifest.

[smcgruer, 2017/05/02 15:25:41]

After looking at this for some time, I believe that the sticky offset is only
relevant to OffsetHeight for non-LayoutBox elements (e.g. inline elements).

LayoutBox::PixelSnappedOffsetHeight relies on LayoutBox::OffsetHeight(),
LayoutBox::Location(), and LayoutBox::ClientTop(), all of which appear to me to
be unaffected by sticky/scroll. (This was surprising, but in my tests the
Location is (0,100) no matter how far you scroll and where the element is
sticking to).

On the other hand, LayoutBoxModelObject::PixelSnappedOffsetHeight relies on
LayoutBoxModelObject::OffsetTop, which can definitely change based on the sticky
offset. I was able to construct a (horrendous) test case where running
compositing inputs update changed the PixelSnappedOffsetHeight from 1 to 2 for
an element.

Someone stop me if the block elements part sounds insane, otherwise I will try
and tidy up the inline test case and add that.

   if (LayoutBoxModelObject* layout_object = GetLayoutBoxModelObject())
     return AdjustLayoutUnitForAbsoluteZoom(
                LayoutUnit(layout_object->PixelSnappedOffsetTop(OffsetParent())),
@@ -1132,7 +1132,7 @@ IntRect Element::VisibleBoundsInVisualViewport() const {
 }
 
 void Element::ClientQuads(Vector<FloatQuad>& quads) {
-  GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheetsForNode(this);
+  EnsureLifecycleValidForLocationAPIs();

[fs, 2017/04/29 09:59:06]

Potentially a lot of other methods needs this treatment, or? (BoundsInViewport
for example, which almost identical to this method...)

[smcgruer, 2017/05/01 20:15:15]

Added to BoundsInViewport, but I suspect that getting the complete list of all
API call-sites that need this will be very tricky. We don't want to fix
unnecessary call-sites due to the performance hit.

[fs, 2017/05/01 21:19:34]

Yes, I'm a bit worried that we're heading into a game of whack-a-mole with
this... I.e it'll be non-obvious to know that you should be using this
new-fangled method when you want to query a location of something (whether for
something directly web-exposed or not.) (I incidently came across such an
instance while looking at a bug of my own which involved
SVGGraphicsElement.getScreenCTM... and how about the shadier bits of MouseEvent?
[offsetX, offsetY])

[smcgruer, 2017/05/02 15:25:41]

I've no experience to rely on here, so if someone feels strongly that definite
correctness is more important than the potential performance hit, I can go
lookup as many uses of 'UpdateStyleAndLayoutIgnorePendingStylesheetsForNode' as
I can find and update them without mercy... but my preferred approach is to say
that this is 'good enough' for now ;)

[fs, 2017/05/02 15:58:07]

Well, you don't necessarily need to do any (additional) changes, but it'd
probably be good to do something like that (listing in a bug or so) and then
take a breif look to see if they could be relevant for updating (getScreenCTM
for instance doesn't use that exact method, so wouldn't be caught by that. I'll
change it to use that when I get the time though...) Every case where we're
inconsistent (for no good reason, or?) can turn into a pain point for authors.
(And if you're really unlucky, eventually even a compat issue... :-()

[smcgruer, 2017/05/02 17:25:30]

Ack, opened http://crbug.com/717601 to track this.

 
   LayoutObject* element_layout_object = GetLayoutObject();
   if (!element_layout_object)
@@ -4328,6 +4328,28 @@ void Element::LogUpdateAttributeIfIsolatedWorldAndInDocument(
   activity_logger->LogEvent("blinkSetAttribute", argv.size(), argv.data());
 }
 
+void Element::EnsureLifecycleValidForLocationAPIs() {

[fs, 2017/04/29 09:59:06]

Maybe this method would be better placed in Document (close to its similar
peers) and with a ...ForNode suffix (and taking a Node* of course)?

[smcgruer, 2017/05/01 20:15:15]

Done. I welcome bike-shedding on the name, it's pretty terrible :)

[fs, 2017/05/01 21:19:33]

Yes, naming... the lifecycle "duality" makes it even trickier. Part of me wants
to stay close to the UpdateStyleAndLayout...-lineage, but that could mean a mile
long name I suppose. I guess the most interesting part of the name is really the
"Location" bit, so might want to keep that.
UpdateStyle[And]LayoutAndAuxiliaryLocationDataForNode(...) =P.

[smcgruer, 2017/05/02 15:25:41]

Sgtm; if nobody has any objections I will update the name to that before commit.

[smcgruer, 2017/05/03 17:56:58]

So, I realized that this CL actually replaces the specific method
'UpdateStyleAndLayoutIgnorePendingStylesheetsForNode', and so the extended name
would be
'UpdateStyleAndLayoutAndAuxiliaryLocationDataIgnorePendingStylesheetsForNode',
which is **75** characters long, which seems a bit much...

For now I renamed to EnsureAuxiliaryLocationDataValidForNode with a moderate
sized comment in Document.h, but lmk if you prefer a different name.

+  if (!InActiveDocument())
+    return;
+
+  GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets();
+
+  // The location of elements that are position: sticky is not known until
+  // compositing inputs are cleaned. Therefore, for any elements that are either
+  // sticky or are in a sticky sub-tree (e.g. are affected by a sticky element),
+  // we need to also clean compositing inputs.
+  FrameView* view = GetDocument().View();
+  if (UNLIKELY(view && GetLayoutObject() &&
+               GetLayoutObject()->Style()->IsInStickySubtree())) {
+    if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
+      // In SPv2, compositing inputs are cleaned as part of PrePaint.
+      view->UpdateAllLifecyclePhasesExceptPaint();
+    } else {
+      view->UpdateLifecycleToCompositingInputsClean();
+    }
+  }
+}
+
 DEFINE_TRACE(Element) {
   if (HasRareData())
     visitor->Trace(GetElementRareData());