[DL] Don't immediately update the locked element frame rect.

This patch ensures that we do layout in a pending frame rect, which is
only put in place for children of the locked element during layout.

After the promise is resolved, we put the pending frame rect as the
active one and ensure that we relayout the previously locked box itself.

This means that if the promise resolution, for example, removed the
element as is the case in
https://docs.google.com/document/d/1VrcVA5JyBmYn0Yi4wjPt1l9ce5Mhf-zu6o63p_mt98w/edit

Then we never commit what was a pending layout and never see a flash
of laid out content.

R=chrishtr@chromium.org, mstensho@chromium.org

Bug: 907613, 882663
Change-Id: I7d03ae73ea4cb96c17ce8bb4162718155c2072be
Reviewed-on: https://chromium-review.googlesource.com/c/1358969Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: vmpstr <vmpstr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614373}

third_party/blink/renderer/core/display_lock/display_lock_context.cc

@@ -11,6 +11,7 @@
 #include "third_party/blink/renderer/core/dom/node_computed_style.h"
 #include "third_party/blink/renderer/core/frame/local_frame_view.h"
 #include "third_party/blink/renderer/core/inspector/inspector_trace_events.h"
+#include "third_party/blink/renderer/core/layout/layout_box.h"
 #include "third_party/blink/renderer/core/layout/layout_object.h"
 #include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/platform/bindings/microtask.h"
@@ -195,6 +196,20 @@ void DisplayLockContext::ProcessQueue() {
   // continue the work.
   process_queue_task_scheduled_ = false;
 
+  // This is the first operation we do on a locked subtree, and a guaranteed
+  // operation before we try to use it, so save off the locked_frame_rect_ here
+  // if we don't have one yet.
+  // TODO(vmpstr): In mode 1, we should add an explicit states for lock pending
+  // and lock acquired.
+  if (!locked_frame_rect_) {
+    auto* layout_object = GetElement()->GetLayoutObject();
+    if (layout_object && layout_object->IsBox()) {
+      locked_frame_rect_ = ToLayoutBox(layout_object)->FrameRect();
+    } else {
+      locked_frame_rect_ = LayoutRect();
+    }
+  }
+
   // If we're not in callbacks pending, then we shouldn't run anything at the
   // moment.
   if (state_ != kCallbacksPending)
@@ -245,6 +260,7 @@ void DisplayLockContext::RejectAndCleanUp() {
     resolver_ = nullptr;
   }
   callbacks_.clear();
+  locked_frame_rect_.reset();
 
   // We may have a dirty subtree and have not propagated the dirty bit up the
   // ancestor tree. Since we're now rejecting the promise and unlocking the
@@ -417,10 +433,58 @@ void DisplayLockContext::DidAttachLayoutTree() {
     RejectAndCleanUp();
 }
 
+DisplayLockContext::ScopedPendingFrameRect
+DisplayLockContext::GetScopedPendingFrameRect() {
+  SCOPED_LOGGER(__PRETTY_FUNCTION__);
+  if (IsResolved())
+    return ScopedPendingFrameRect(nullptr);
+  DCHECK(GetElement()->GetLayoutObject() && GetElement()->GetLayoutBox());
+  GetElement()->GetLayoutBox()->SetFrameRectForDisplayLock(pending_frame_rect_);
+  return ScopedPendingFrameRect(this);
+}
+
+void DisplayLockContext::NotifyPendingFrameRectScopeEnded() {
+  SCOPED_LOGGER(__PRETTY_FUNCTION__);
+  DCHECK(GetElement()->GetLayoutObject() && GetElement()->GetLayoutBox());
+  DCHECK(locked_frame_rect_);
+  pending_frame_rect_ = GetElement()->GetLayoutBox()->FrameRect();
+  GetElement()->GetLayoutBox()->SetFrameRectForDisplayLock(*locked_frame_rect_);
+}
+
 void DisplayLockContext::FinishResolution() {
   SCOPED_LOGGER(__PRETTY_FUNCTION__);
   if (state_ == kResolving)
     state_ = kResolved;
+  locked_frame_rect_.reset();
+
+  // Update the pending frame rect if we still have a layout object.
+
+  // First, if the element is gone, then there's nothing to do.
+  if (!weak_element_handle_)
+    return;
+  auto* layout_object = GetElement()->GetLayoutObject();
+  // The resolution could have disconnected us, which would destroy the layout
+  // object.
+  if (!layout_object || !layout_object->IsBox())
+    return;
+  bool frame_rect_changed =
+      ToLayoutBox(layout_object)->FrameRect() != pending_frame_rect_;
+  // If the frame rect hasn't actually changed then we don't need to do
+  // anything.
+  // TODO(vmpstr): Is this true? What about things like paint? This also needs
+  // to be updated if there are more "stashed" things.
+  if (!frame_rect_changed)
+    return;
+
+  // Set the pending frame rect as the new one, and ensure to schedule a layout
+  // for just the box itself. Note that we use the non-display locked version to
+  // ensure all the hooks are property invoked.
+  ToLayoutBox(layout_object)->SetFrameRect(pending_frame_rect_);
+  layout_object->SetNeedsLayout(
+      layout_invalidation_reason::kDisplayLockCommitting);
+  // Schedule an animation to perform the lifecycle phases.
+  GetElement()->GetDocument().GetPage()->Animator().ScheduleVisualUpdate(
+      GetElement()->GetDocument().GetFrame());
 }
 
 void DisplayLockContext::StartCommit() {
@@ -468,4 +532,19 @@ void DisplayLockContext::MarkAsDisconnected() {
   element_ = nullptr;
 }
 
+DisplayLockContext::ScopedPendingFrameRect::ScopedPendingFrameRect(
+    DisplayLockContext* context)
+    : context_(context) {}
+
+DisplayLockContext::ScopedPendingFrameRect::ScopedPendingFrameRect(
+    ScopedPendingFrameRect&& other)
+    : context_(other.context_) {
+  other.context_ = nullptr;
+}
+
+DisplayLockContext::ScopedPendingFrameRect::~ScopedPendingFrameRect() {
+  if (context_)
+    context_->NotifyPendingFrameRectScopeEnded();
+}
+
 }  // namespace blink

third_party/blink/renderer/core/display_lock/display_lock_context.h

@@ -46,6 +46,19 @@ class CORE_EXPORT DisplayLockContext final
     kDone
   };
 
+  class ScopedPendingFrameRect {
+   public:
+    ScopedPendingFrameRect(ScopedPendingFrameRect&&);
+    ~ScopedPendingFrameRect();
+
+   private:
+    friend class DisplayLockContext;
+
+    ScopedPendingFrameRect(DisplayLockContext*);
+
+    UntracedMember<DisplayLockContext> context_ = nullptr;
+  };
+
   DisplayLockContext(Element*, ExecutionContext*);
   ~DisplayLockContext() override;
 
@@ -101,6 +114,13 @@ class CORE_EXPORT DisplayLockContext final
 
   void DidAttachLayoutTree();
 
+  // Returns a ScopedPendingFrameRect object which exposes the pending layout
+  // frame rect to LayoutBox. This is used to ensure that children of the locked
+  // element use the pending layout frame to update the size of the element.
+  // After the scoped object is destroyed, the previous frame rect is restored
+  // and the pending one is stored in the context until it is needed.
+  ScopedPendingFrameRect GetScopedPendingFrameRect();
+
  private:
   friend class DisplayLockContextTest;
   friend class DisplayLockSuspendedHandle;
@@ -153,6 +173,10 @@ class CORE_EXPORT DisplayLockContext final
     return weak_element_handle_;
   }
 
+  // When ScopedPendingFrameRect is destroyed, it calls this function. See
+  // GetScopedPendingFrameRect() for more information.
+  void NotifyPendingFrameRectScopeEnded();
+
   HeapVector<Member<V8DisplayLockCallback>> callbacks_;
   Member<ScriptPromiseResolver> resolver_;
 
@@ -185,6 +209,8 @@ class CORE_EXPORT DisplayLockContext final
   unsigned suspended_count_ = 0;
   State state_ = kUninitialized;
   LifecycleUpdateState lifecycle_update_state_ = kNeedsStyle;
+  LayoutRect pending_frame_rect_;
+  base::Optional<LayoutRect> locked_frame_rect_;
 };
 
 }  // namespace blink

third_party/blink/renderer/core/layout/layout_block.cc

@@ -434,8 +434,21 @@ void LayoutBlock::UpdateLayout() {
 
   LayoutAnalyzer::Scope analyzer(*this);
 
-  if (LayoutBlockedByDisplayLock())
-    return;
+  base::Optional<DisplayLockContext::ScopedPendingFrameRect>
+      scoped_pending_frame_rect;
+  if (auto* context = GetDisplayLockContext()) {
+    // In a display locked element, we might be prevented from doing layout in
+    // which case we should abort.
+    if (LayoutBlockedByDisplayLock())
+      return;
+    // If we're display locked, then our layout should go into a pending frame
+    // rect without updating the frame rect visible to the ancestors. The
+    // following scoped object provides this functionality: it puts in place the
+    // (previously updated) pending frame rect. When the object is destroyed, it
+    // saves the pending frame rect in the DisplayLockContext and restores the
+    // frame rect that was in place at the time the lock was acquired.
+    scoped_pending_frame_rect.emplace(context->GetScopedPendingFrameRect());
+  }
 
   bool needs_scroll_anchoring =
       HasOverflowClip() && GetScrollableArea()->ShouldPerformScrollAnchoring();

third_party/blink/renderer/core/layout/layout_box.h

@@ -389,6 +389,14 @@ class CORE_EXPORT LayoutBox : public LayoutBoxModelObject {
     SetLocation(rect.Location());
     SetSize(rect.Size());
   }
+  // Similar to SetFrameRect(), except it avoids notifying other code about size
+  // and location changes. This should only be used from a DisplayLockContext to
+  // temporarily put in place a pending frame rect which is restored at the end
+  // of layout. Code outside of layout should not observe location or size
+  // changes.
+  void SetFrameRectForDisplayLock(const LayoutRect& rect) {
+    frame_rect_ = rect;
+  }
 
   // Note that those functions have their origin at this box's CSS border box.
   // As such their location doesn't account for 'top'/'left'. About its

third_party/blink/renderer/core/layout/layout_object.h

@@ -2386,6 +2386,14 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
       context->DidLayout();
   }
 
+  DisplayLockContext* GetDisplayLockContext() const {
+    if (!RuntimeEnabledFeatures::DisplayLockingEnabled())
+      return nullptr;
+    if (!GetNode() || !GetNode()->IsElementNode())
+      return nullptr;
+    return ToElement(GetNode())->GetDisplayLockContext();
+  }
+
  private:
   // Used only by applyFirstLineChanges to get a first line style based off of a
   // given new style, without accessing the cache.
@@ -2469,14 +2477,6 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
   }
   LayoutRect AdjustVisualRectForInlineBox(const LayoutRect&) const;
 
-  DisplayLockContext* GetDisplayLockContext() const {
-    if (!RuntimeEnabledFeatures::DisplayLockingEnabled())
-      return nullptr;
-    if (!GetNode() || !GetNode()->IsElementNode())
-      return nullptr;
-    return ToElement(GetNode())->GetDisplayLockContext();
-  }
-
   // This is set by Set[Subtree]ShouldDoFullPaintInvalidation, and cleared
   // during PrePaint in this object's InvalidatePaint(). It's different from
   // DisplayItemClient::GetPaintInvalidationReason() which is set during

third_party/blink/web_tests/display-lock/lock-before-append/measure-and-remove-expected.html

+<style>
+#spacer {
+  width: 150px;
+  height: 150px;
+  background: green;
+}
+</style>
+
+<div id="spacer"></div>
+<div id="log">0 20 40 8 8</div>
+

third_party/blink/web_tests/display-lock/lock-before-append/measure-and-remove.html

+<!doctype HTML>
+
+<!--
+Runs an acquireDisplayLock which constructs a subtree, and measures it in
+the promise resolution.
+-->
+
+<style>
+#container {
+  contain: content;
+  width: 100px;
+  height: 100px;
+  background: lightgreen;
+}
+.child {
+  width: 20px;
+  height: 20%;
+  background: cyan;
+}
+#empty {
+  background: red;
+  width: max-content;
+}
+#spacer {
+  width: 150px;
+  height: 150px;
+  background: green;
+}
+</style>
+
+<div id="empty"></div>
+<div id="spacer"></div>
+<div id="log"></div>
+
+<script>
+if (window.testRunner)
+  window.testRunner.waitUntilDone();
+
+function finish() {
+  if (window.testRunner)
+    window.testRunner.notifyDone();
+}
+
+function measureAndRemove() {
+  let log = document.getElementById("log");
+  log.innerHTML += "" + document.getElementById("0").offsetTop;
+  log.innerHTML += " " + document.getElementById("1").offsetTop;
+  log.innerHTML += " " + document.getElementById("2").offsetTop;
+  log.innerHTML += " " + document.getElementById("empty").offsetTop;
+  log.innerHTML += " " + document.getElementById("spacer").offsetTop;
+  document.getElementById("container").remove();
+}
+
+function createChild(id) {
+  let child = document.createElement("div");
+  child.classList = "child";
+  child.id = id;
+  return child;
+}
+
+function construct(context) {
+  context.lockedElement.appendChild(createChild("0"));
+  context.lockedElement.appendChild(createChild("1"));
+  context.lockedElement.appendChild(createChild("2"));
+}
+
+function acquire() {
+  let container = document.createElement("div");
+  container.id = "container";
+  container.acquireDisplayLock(construct).then(measureAndRemove).then(finish);
+  document.getElementById("empty").appendChild(container);
+}
+
+window.onload = acquire;
+</script>
+