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

third_party/WebKit/Source/core/dom/Element.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Peter Kelly (pmk@post.com)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2007 David Smith (catfish.man@gmail.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2012, 2013 Apple Inc.
  * All rights reserved.
  *           (C) 2007 Eric Seidel (eric@webkit.org)
  *
@@ skipping 413 matching lines @@
     return attribute->Value();
   return g_null_atom;
 }
 
 AtomicString Element::LowercaseIfNecessary(const AtomicString& name) const {
   return IsHTMLElement() && GetDocument().IsHTMLDocument() ? name.LowerASCII()
                                                            : name;
 }
 
 void Element::scrollIntoView(bool align_to_top) {
-  GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheetsForNode(this);
+  EnsureLifecycleValidForLocationAPIs();
 
   if (!GetLayoutObject())
     return;
 
   bool make_visible_in_visual_viewport =
       !GetDocument().GetPage()->GetSettings().GetInertVisualViewport();
 
   LayoutRect bounds = BoundingBox();
   // Align to the top / bottom and to the closest edge.
   if (align_to_top)
     GetLayoutObject()->ScrollRectToVisible(
         bounds, ScrollAlignment::kAlignToEdgeIfNeeded,
         ScrollAlignment::kAlignTopAlways, kProgrammaticScroll,
         make_visible_in_visual_viewport);
   else
     GetLayoutObject()->ScrollRectToVisible(
         bounds, ScrollAlignment::kAlignToEdgeIfNeeded,
         ScrollAlignment::kAlignBottomAlways, kProgrammaticScroll,
         make_visible_in_visual_viewport);
 
   GetDocument().SetSequentialFocusNavigationStartingPoint(this);
 }
 
 void Element::scrollIntoViewIfNeeded(bool center_if_needed) {
-  GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheetsForNode(this);
+  EnsureLifecycleValidForLocationAPIs();
 
   if (!GetLayoutObject())
     return;
 
   bool make_visible_in_visual_viewport =
       !GetDocument().GetPage()->GetSettings().GetInertVisualViewport();
 
   LayoutRect bounds = BoundingBox();
   if (center_if_needed)
     GetLayoutObject()->ScrollRectToVisible(
@@ skipping 166 matching lines @@
     callback->handleEvent(&scroll_state);
   if (callback->NativeScrollBehavior() !=
       WebNativeScrollBehavior::kDisableNativeScroll)
     NativeApplyScroll(scroll_state);
   if (callback->NativeScrollBehavior() ==
       WebNativeScrollBehavior::kPerformAfterNativeScroll)
     callback->handleEvent(&scroll_state);
 }
 
 int Element::OffsetLeft() {
-  GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheetsForNode(this);
+  EnsureLifecycleValidForLocationAPIs();
   if (LayoutBoxModelObject* layout_object = GetLayoutBoxModelObject())
     return AdjustLayoutUnitForAbsoluteZoom(
                LayoutUnit(
                    layout_object->PixelSnappedOffsetLeft(OffsetParent())),
                layout_object->StyleRef())
         .Round();
   return 0;
 }
 
 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())),
                layout_object->StyleRef())
         .Round();
   return 0;
 }
 
 int Element::OffsetWidth() {
   GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheetsForNode(this);
@@ skipping 457 matching lines @@
   IntSize viewport_size = GetDocument().GetPage()->GetVisualViewport().Size();
   IntRect rect(0, 0, viewport_size.Width(), viewport_size.Height());
   // We don't use absoluteBoundingBoxRect() because it can return an IntRect
   // larger the actual size by 1px. crbug.com/470503
   rect.Intersect(GetDocument().View()->ContentsToViewport(
       RoundedIntRect(GetLayoutObject()->AbsoluteBoundingBoxFloatRect())));
   return rect;
 }
 
 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)
     return;
 
   if (IsSVGElement() && !element_layout_object->IsSVGRoot()) {
     // Get the bounding rectangle from the SVG model.
     if (ToSVGElement(this)->IsSVGGraphicsElement())
       quads.push_back(element_layout_object->LocalToAbsoluteQuad(
           element_layout_object->ObjectBoundingBox()));
@@ skipping 3175 matching lines @@
   if (!activity_logger)
     return;
   Vector<String, 4> argv;
   argv.push_back(element);
   argv.push_back(params.name.ToString());
   argv.push_back(params.old_value);
   argv.push_back(params.new_value);
   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());
   visitor->Trace(element_data_);
   ContainerNode::Trace(visitor);
 }
 
 DEFINE_TRACE_WRAPPERS(Element) {
   if (HasRareData()) {
     visitor->TraceWrappers(GetElementRareData());
   }
   ContainerNode::TraceWrappers(visitor);
 }
 
 }  // namespace blink