ContentVisibility: Ensure to keep auto f-i-p target unlocked for 2 frames.

This patch ensures that when we have a find-in-page target, we keep
it unlocked for two frames, so that the visibility detection can take
place and if needed keep the context unlocked.

The root problem here is that sometimes our contain-intrinsic-size is
too small, so that when we find a match and scroll it into view,
we actually scroll past the locked size by enough distance that
the container remains locked, so we never see the match.

With this patch, the container is unlocked for two frames which is
the least amount needed for visibility to kick in and start being
unlocked.

This patch also cleans up the previous focus-clearing bug to use
the same mechanism of unlocking-until-lifecycle.

R=chrishtr@chromium.org

Change-Id: I75a2d8b2f41335d5d6fb7a4ec0f42d93438807e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2264756Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: vmpstr <vmpstr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782115}

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

@@ -30,6 +30,7 @@
 #include "third_party/blink/renderer/platform/bindings/microtask.h"
 #include "third_party/blink/renderer/platform/heap/heap.h"
 #include "third_party/blink/renderer/platform/instrumentation/use_counter.h"
+#include "third_party/blink/renderer/platform/wtf/text/string_builder.h"
 
 namespace blink {
 
@@ -239,7 +240,9 @@ void DisplayLockContext::UpdateActivationObservationIfNeeded() {
 }
 
 bool DisplayLockContext::NeedsLifecycleNotifications() const {
-  return needs_deferred_not_intersecting_signal_;
+  return needs_deferred_not_intersecting_signal_ ||
+         render_affecting_state_[static_cast<int>(
+             RenderAffectingState::kAutoStateUnlockedUntilLifecycle)];
 }
 
 void DisplayLockContext::UpdateLifecycleNotificationRegistration() {
@@ -409,9 +412,31 @@ void DisplayLockContext::CommitForActivationWithSignal(
   DCHECK(IsLocked());
   DCHECK(ShouldCommitForActivation(DisplayLockActivationReason::kAny));
 
+  // Find in page scrolls content into view. However, if the position of the
+  // target is outside of the bounds that would cause the auto-context to
+  // unlock, then we can scroll into wrong content while the context remains
+  // lock. To avoid this, unlock it until the next lifecycle. If the scroll is
+  // successful, then we will gain visibility anyway so the context will be
+  // unlocked for other reasons.
+  // TODO(vmpstr): See if scrollIntoView() needs this as well.
+  if (reason == DisplayLockActivationReason::kFindInPage) {
+    // Note that because the visibility is only determined at the _end_ of the
+    // next frame, we need to ensure that we stay unlocked for two frames.
+    SetKeepUnlockedUntilLifecycleCount(2);
+  }
+
   RecordActivationReason(document_, reason);
 }
 
+void DisplayLockContext::SetKeepUnlockedUntilLifecycleCount(int count) {
+  DCHECK_GT(count, 0);
+  keep_unlocked_count_ = std::max(keep_unlocked_count_, count);
+  SetRenderAffectingState(
+      RenderAffectingState::kAutoStateUnlockedUntilLifecycle, true);
+  UpdateLifecycleNotificationRegistration();
+  ScheduleAnimation();
+}
+
 void DisplayLockContext::NotifyIsIntersectingViewport() {
   // If we are now intersecting, then we are definitely not nested in a locked
   // subtree and we don't need to lock as a result.
@@ -805,9 +830,18 @@ void DisplayLockContext::WillStartLifecycleUpdate(const LocalFrameView& view) {
   if (needs_deferred_not_intersecting_signal_)
     NotifyIsNotIntersectingViewport();
 
-  if (has_deferred_selection_clear_) {
-    NotifySubtreeLostSelection();
-    DCHECK(!has_deferred_selection_clear_);
+  // If we're keeping this context unlocked, update the values.
+  if (keep_unlocked_count_) {
+    if (--keep_unlocked_count_) {
+      ScheduleAnimation();
+    } else {
+      SetRenderAffectingState(
+          RenderAffectingState::kAutoStateUnlockedUntilLifecycle, false);
+      UpdateLifecycleNotificationRegistration();
+    }
+  } else {
+    DCHECK(!render_affecting_state_[static_cast<int>(
+        RenderAffectingState::kAutoStateUnlockedUntilLifecycle)]);
   }
 }
 
@@ -983,16 +1017,11 @@ void DisplayLockContext::SetRenderAffectingState(RenderAffectingState state,
   // find-in-page. We cannot lock an object while doing this, since it may
   // invalidate layout and in turn prevent find-in-page from properly finding
   // text (and DCHECK). Since layout is clean for this lock (we're unlocked),
-  // defer selection clearing to the next lifecycle.
-  if (state == RenderAffectingState::kSubtreeHasSelection) {
-    if (!new_flag && document_->GetDisplayLockDocumentState()
-                         .ActivatableDisplayLocksForced()) {
-      has_deferred_selection_clear_ = true;
-      ScheduleAnimation();
-      return;
-    } else {
-      has_deferred_selection_clear_ = false;
-    }
+  // keep the context unlocked until the next lifecycle starts.
+  if (state == RenderAffectingState::kSubtreeHasSelection && !new_flag &&
+      document_->GetDisplayLockDocumentState()
+          .ActivatableDisplayLocksForced()) {
+    SetKeepUnlockedUntilLifecycleCount(1);
   }
 
   render_affecting_state_[static_cast<int>(state)] = new_flag;
@@ -1022,7 +1051,8 @@ void DisplayLockContext::NotifyRenderAffectingStateChanged() {
       (state_ != EContentVisibility::kAuto ||
        (!state(RenderAffectingState::kIntersectsViewport) &&
         !state(RenderAffectingState::kSubtreeHasFocus) &&
-        !state(RenderAffectingState::kSubtreeHasSelection)));
+        !state(RenderAffectingState::kSubtreeHasSelection) &&
+        !state(RenderAffectingState::kAutoStateUnlockedUntilLifecycle)));
 
   if (should_be_locked && !IsLocked())
     Lock();
@@ -1036,4 +1066,35 @@ void DisplayLockContext::Trace(Visitor* visitor) const {
   visitor->Trace(whitespace_reattach_set_);
 }
 
+const char* DisplayLockContext::RenderAffectingStateName(int state) const {
+  switch (static_cast<RenderAffectingState>(state)) {
+    case RenderAffectingState::kLockRequested:
+      return "LockRequested";
+    case RenderAffectingState::kIntersectsViewport:
+      return "IntersectsViewport";
+    case RenderAffectingState::kSubtreeHasFocus:
+      return "SubtreeHasFocus";
+    case RenderAffectingState::kSubtreeHasSelection:
+      return "SubtreeHasSelection";
+    case RenderAffectingState::kAutoStateUnlockedUntilLifecycle:
+      return "AutoStateUnlockedUntilLifecycle";
+    case RenderAffectingState::kNumRenderAffectingStates:
+      break;
+  }
+  return "<Invalid State>";
+}
+
+String DisplayLockContext::RenderAffectingStateToString() const {
+  StringBuilder builder;
+  for (int i = 0;
+       i < static_cast<int>(RenderAffectingState::kNumRenderAffectingStates);
+       ++i) {
+    builder.Append(RenderAffectingStateName(i));
+    builder.Append(": ");
+    builder.Append(render_affecting_state_[i] ? "true" : "false");
+    builder.Append("\n");
+  }
+  return builder.ToString();
+}
+
 }  // namespace blink

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

@@ -9,6 +9,7 @@
 #include "third_party/blink/renderer/core/frame/local_frame_view.h"
 #include "third_party/blink/renderer/platform/scheduler/public/post_cancellable_task.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
+#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
 
 namespace blink {
 
@@ -197,6 +198,9 @@ class CORE_EXPORT DisplayLockContext final
   // GC functions.
   void Trace(Visitor*) const override;
 
+  // Debugging functions.
+  String RenderAffectingStateToString() const;
+
  private:
   // Give access to |NotifyForcedUpdateScopeStarted()| and
   // |NotifyForcedUpdateScopeEnded()|.
@@ -295,6 +299,12 @@ class CORE_EXPORT DisplayLockContext final
   // selected notes up to its root looking for `element_`.
   void DetermineIfSubtreeHasSelection();
 
+  // Keep this context unlocked until the beginning of lifecycle. Effectively
+  // keeps this context unlocked for the next `count` frames. It also schedules
+  // a frame to ensure the lifecycle happens. Only affects locks with 'auto'
+  // setting.
+  void SetKeepUnlockedUntilLifecycleCount(int count);
+
   WeakMember<Element> element_;
   WeakMember<Document> document_;
   EContentVisibility state_ = EContentVisibility::kVisible;
@@ -348,21 +358,26 @@ class CORE_EXPORT DisplayLockContext final
   // Lock has been requested.
   bool is_locked_ = false;
 
-  // If true, clear selection render affecting state bit on the next lifecycle.
-  bool has_deferred_selection_clear_ = false;
+  // If true, this lock is kept unlocked at least until the beginning of the
+  // lifecycle. If nothing else is keeping it unlocked, then it will be locked
+  // again at the start of the lifecycle.
+  bool keep_unlocked_until_lifecycle_ = false;
 
   enum class RenderAffectingState : int {
     kLockRequested,
     kIntersectsViewport,
     kSubtreeHasFocus,
     kSubtreeHasSelection,
+    kAutoStateUnlockedUntilLifecycle,
     kNumRenderAffectingStates
   };
   void SetRenderAffectingState(RenderAffectingState state, bool flag);
   void NotifyRenderAffectingStateChanged();
+  const char* RenderAffectingStateName(int state) const;
 
   bool render_affecting_state_[static_cast<int>(
       RenderAffectingState::kNumRenderAffectingStates)] = {false};
+  int keep_unlocked_count_ = 0;
 };
 
 }  // namespace blink

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

@@ -360,7 +360,6 @@ TEST_F(DisplayLockContextTest, FindInPageContinuesAfterRelock) {
   EXPECT_EQ(1, client.Count());
 
   auto* container = GetDocument().getElementById("container");
-  // container->classList().Add("auto");
   GetDocument().scrollingElement()->setScrollTop(0);
 
   UpdateAllLifecyclePhasesForTest();
@@ -380,6 +379,41 @@ TEST_F(DisplayLockContextTest, FindInPageContinuesAfterRelock) {
   EXPECT_EQ(1, client.Count());
 }
 
+TEST_F(DisplayLockContextTest, FindInPageTargetBelowLockedSize) {
+  ResizeAndFocus();
+  SetHtmlInnerHTML(R"HTML(
+    <style>
+    .spacer { height: 1000px; }
+    #container { contain-intrinsic-size: 1px; }
+    .auto { content-visibility: auto }
+    </style>
+    <body>
+      <div class=spacer></div>
+      <div id=container class=auto>
+        <div class=spacer></div>
+        <div id=target>testing</div>
+      </div>
+      <div class=spacer></div>
+      <div class=spacer></div>
+    </body>
+  )HTML");
+
+  const String search_text = "testing";
+  DisplayLockTestFindInPageClient client;
+  client.SetFrame(LocalMainFrame());
+
+  Find(search_text, client);
+  EXPECT_EQ(1, client.Count());
+
+  auto* container = GetDocument().getElementById("container");
+  // The container should be unlocked.
+  EXPECT_FALSE(container->GetDisplayLockContext()->IsLocked());
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_FALSE(container->GetDisplayLockContext()->IsLocked());
+
+  EXPECT_FLOAT_EQ(GetDocument().scrollingElement()->scrollTop(), 1768.5);
+}
+
 TEST_F(DisplayLockContextTest,
        ActivatableLockedElementTickmarksAreAtLockedRoots) {
   ResizeAndFocus();