Fix the bug with Incremental Mount

Summary:
## Context
This was another attempt to fix the issue with IM based on the discussion of D62756083. Now that we've identified another bug with `initIncrementalMount` where would unmount both Hosts and Components that exclude from IM, which is different with the logic of `performIncrementalMount`.

So the current solution would work like this:

1. Kick off IM with an empty visible rect when LithoView gets detached, which would go into `initIncrementalMount` and unmount all components not excluding from IM.
2. We'll end up having a couple of hosts(the parent of component that excludes from IM) and concreate components unmounted, during the `MountState::detach` we run the legacy flow to unbind them.
3. When LithoView is re-attached, we would re-bind those components left by IM(excomponents exclude from IM and their parents) first.
4. Kick off IM again to re-mount and re-bind the rest of components.

### Out of scope:
Given we're addressing the issue with Incremental Mount that we never unmount component when it gets out of screen. And due to the fact that we're not able to get a correct visible rect of a view when it's detached, we have to resort to unmounting them when detached, and that also means we cannot ensure that all components will be mounted again if user detach and attach LithoView manually, which can only happen when Incremental Mount being triggered at least once.

Reviewed By: adityasharat

Differential Revision: D63028994

fbshipit-source-id: be4de0b7c1a039577434b9e762f61171414b0700

litho-core/src/main/java/com/facebook/litho/BaseMountingView.java

@@ -327,9 +327,17 @@ public void unbind() {
   }
 
   protected void onDetached() {
+    maybeUnmountComponents();
     mMountState.detach();
   }
 
+  private void maybeUnmountComponents() {
+    final @Nullable ComponentsConfiguration config = getConfiguration();
+    if (config != null && config.enableFixForIM && !mIsTemporaryDetached && !hasTransientState()) {
+      notifyVisibleBoundsChanged(EMPTY_RECT);
+    }
+  }
+
   boolean isAttached() {
     return mIsAttached;
   }

litho-it/src/main/java/com/facebook/litho/widget/MountSpecLifecycleTesterSpec.kt

@@ -48,6 +48,7 @@ import com.facebook.litho.annotations.OnPrepare
 import com.facebook.litho.annotations.OnUnbind
 import com.facebook.litho.annotations.OnUnmount
 import com.facebook.litho.annotations.Prop
+import com.facebook.litho.annotations.ShouldExcludeFromIncrementalMount
 import com.facebook.litho.annotations.State
 import com.facebook.rendercore.MountItemsPool
 
@@ -76,6 +77,14 @@ object MountSpecLifecycleTesterSpec {
     lifecycleTracker.addStep(LifecycleStep.ON_PREPARE)
   }
 
+  @JvmStatic
+  @ShouldExcludeFromIncrementalMount
+  fun shouldExcludeFromIncrementalMount(
+      @Prop(optional = true) shouldExcludeFromIncrementalMount: Boolean?
+  ): Boolean {
+    return shouldExcludeFromIncrementalMount ?: false
+  }
+
   @JvmStatic
   @OnMeasure
   fun onMeasure(

litho-it/src/test/java/com/facebook/litho/ClickEventHandlerUpdateTest.kt

@@ -16,6 +16,7 @@
 
 package com.facebook.litho
 
+import com.facebook.litho.config.ComponentsConfiguration
 import com.facebook.litho.testing.LithoTestRule
 import com.facebook.litho.testing.testrunner.LithoTestRunner
 import com.facebook.litho.widget.ClickEventTrackingImage
@@ -92,6 +93,11 @@ class ClickEventHandlerUpdateTest {
 
   @Test
   fun `click event handler on drawable mountspec should update`() {
+    if (ComponentsConfiguration.defaultInstance.enableFixForIM) {
+      // With IM fix we'll unmount all components when LV gets detached, which means Host will not
+      // be the same
+      return
+    }
     val context = mLithoTestRule.context
     val tracker = mutableListOf<String>()
 

litho-it/src/test/java/com/facebook/litho/InterStagePropsTest.kt

@@ -16,7 +16,7 @@
 
 package com.facebook.litho
 
-import com.facebook.litho.testing.LegacyLithoViewRule
+import com.facebook.litho.testing.LithoViewRule
 import com.facebook.litho.testing.testrunner.LithoTestRunner
 import com.facebook.litho.widget.MountSpecInterStagePropsTester
 import com.facebook.litho.widget.SimpleStateUpdateEmulator
@@ -34,38 +34,49 @@ class InterStagePropsTest {
 
   private lateinit var context: ComponentContext
 
-  @JvmField @Rule val legacyLithoViewRule = LegacyLithoViewRule()
+  @JvmField @Rule val lithoViewRule = LithoViewRule()
 
   @Before
   fun setUp() {
-    context = legacyLithoViewRule.context
-    legacyLithoViewRule.useLithoView(LithoView(context))
+    context = lithoViewRule.context
   }
 
   @Test
   fun interStageProp_FromPrepare_usedIn_OnBind() {
     val lifecycleTracker = LifecycleTracker()
     val stateUpdater = SimpleStateUpdateEmulatorSpec.Caller()
     val root = createComponent(lifecycleTracker, stateUpdater)
-    legacyLithoViewRule.setRoot(root).attachToWindow().measure().layout()
+    val testLithoView = lithoViewRule.render { root }.attachToWindow().measure().layout()
     lifecycleTracker.reset()
     stateUpdater.increment()
-    legacyLithoViewRule.lithoView.onDetachedFromWindowForTest()
+    testLithoView.detachFromWindow()
     // We need to make sure layout happens because which triggers mount and bind together
-    legacyLithoViewRule.attachToWindow().measure().layout()
-    assertThat(lifecycleTracker.steps)
-        .describedAs("On Bind should be called")
-        .contains(LifecycleStep.ON_UNBIND, LifecycleStep.ON_BIND)
+    testLithoView.attachToWindow()
+    val config = testLithoView.lithoView.configuration
+    if (config != null && config.enableFixForIM) {
+      testLithoView.lithoView.notifyVisibleBoundsChanged()
+      assertThat(lifecycleTracker.steps)
+          .describedAs("On Bind should be called")
+          .containsExactly(
+              LifecycleStep.ON_UNBIND,
+              LifecycleStep.ON_UNMOUNT,
+              LifecycleStep.ON_MOUNT,
+              LifecycleStep.ON_BIND)
+    } else {
+      assertThat(lifecycleTracker.steps)
+          .describedAs("On Bind should be called")
+          .containsExactly(LifecycleStep.ON_UNBIND, LifecycleStep.ON_BIND)
+    }
   }
 
   @Test
   fun interStageProp_FromBind_usedIn_OnUnbind() {
     val lifecycleTracker = LifecycleTracker()
     val stateUpdater = SimpleStateUpdateEmulatorSpec.Caller()
     val root = createComponent(lifecycleTracker, stateUpdater)
-    legacyLithoViewRule.setRoot(root).attachToWindow().measure().layout()
+    val testLithoView = lithoViewRule.render { root }.attachToWindow().measure().layout()
     lifecycleTracker.reset()
-    legacyLithoViewRule.detachFromWindow()
+    testLithoView.detachFromWindow()
     assertThat(lifecycleTracker.steps)
         .describedAs("On Unbind should be called")
         .contains(LifecycleStep.ON_UNBIND)
@@ -76,7 +87,7 @@ class InterStagePropsTest {
     val lifecycleTracker = LifecycleTracker()
     val stateUpdater = SimpleStateUpdateEmulatorSpec.Caller()
     val root = createComponent(lifecycleTracker, stateUpdater)
-    legacyLithoViewRule.setRoot(root).attachToWindow().measure().layout()
+    lithoViewRule.render { root }.attachToWindow().measure().layout()
     assertThat(lifecycleTracker.steps)
         .describedAs("On Mount should be called")
         .contains(LifecycleStep.ON_MOUNT)
@@ -87,9 +98,9 @@ class InterStagePropsTest {
     val lifecycleTracker = LifecycleTracker()
     val stateUpdater = SimpleStateUpdateEmulatorSpec.Caller()
     val root = createComponent(lifecycleTracker, stateUpdater)
-    legacyLithoViewRule.setRoot(root).attachToWindow().measure().layout()
+    val testLithoView = lithoViewRule.render { root }.attachToWindow().measure().layout()
     lifecycleTracker.reset()
-    legacyLithoViewRule.lithoView.unmountAllItems()
+    testLithoView.lithoView.unmountAllItems()
     assertThat(lifecycleTracker.steps)
         .describedAs("On Unmount should be called")
         .contains(LifecycleStep.ON_UNMOUNT)
@@ -99,10 +110,8 @@ class InterStagePropsTest {
       lifecycleTracker: LifecycleTracker,
       stateUpdater: SimpleStateUpdateEmulatorSpec.Caller
   ): Component =
-      Column.create(legacyLithoViewRule.context)
-          .child(
-              MountSpecInterStagePropsTester.create(legacyLithoViewRule.context)
-                  .lifecycleTracker(lifecycleTracker))
-          .child(SimpleStateUpdateEmulator.create(legacyLithoViewRule.context).caller(stateUpdater))
+      Column.create(context)
+          .child(MountSpecInterStagePropsTester.create(context).lifecycleTracker(lifecycleTracker))
+          .child(SimpleStateUpdateEmulator.create(context).caller(stateUpdater))
           .build()
 }

litho-it/src/test/java/com/facebook/litho/MountSpecLifecycleTest.kt

@@ -120,38 +120,95 @@ class MountSpecLifecycleTest {
   @Test
   fun lifecycle_onDetach_shouldCallLifecycleMethods() {
     val lifecycleTracker = LifecycleTracker()
+    // using a component that has an exactly the same size as the container to cover the edge case
     val testLithoView =
-        lithoViewRule.render {
+        lithoViewRule.render(widthPx = 800, heightPx = 600) {
           MountSpecLifecycleTester.create(lithoViewRule.context)
               .intrinsicSize(Size(800, 600))
               .lifecycleTracker(lifecycleTracker)
               .build()
         }
     lifecycleTracker.reset()
     testLithoView.detachFromWindow()
-    assertThat(lifecycleTracker.steps)
-        .describedAs("Should only call")
-        .containsExactly(LifecycleStep.ON_UNBIND)
+    val config = testLithoView.lithoView.configuration
+    if (config != null && config.enableFixForIM) {
+      assertThat(lifecycleTracker.steps)
+          .describedAs("Should only call")
+          .containsExactly(LifecycleStep.ON_UNBIND, LifecycleStep.ON_UNMOUNT)
+    } else {
+      assertThat(lifecycleTracker.steps)
+          .describedAs("Should only call")
+          .containsExactly(LifecycleStep.ON_UNBIND)
+    }
   }
 
   @Test
   fun lifecycle_onReAttach_shouldCallLifecycleMethods() {
-    val lifecycleTracker = LifecycleTracker()
+    val lifecycleTracker1 = LifecycleTracker()
+    val lifecycleTracker2 = LifecycleTracker()
+
     val testLithoView =
         lithoViewRule
             .render {
-              MountSpecLifecycleTester.create(lithoViewRule.context)
-                  .lifecycleTracker(lifecycleTracker)
-                  .intrinsicSize(Size(800, 600))
-                  .build()
+              Column {
+                child(
+                    MountSpecLifecycleTester.create(lithoViewRule.context)
+                        .lifecycleTracker(lifecycleTracker1)
+                        .intrinsicSize(Size(800, 600))
+                        // exclude this component from incremental mount
+                        .shouldExcludeFromIncrementalMount(true)
+                        .build())
+                child(
+                    MountSpecLifecycleTester.create(lithoViewRule.context)
+                        .lifecycleTracker(lifecycleTracker2)
+                        .intrinsicSize(Size(800, 600))
+                        .build())
+              }
             }
             .detachFromWindow()
 
-    lifecycleTracker.reset()
-    testLithoView.attachToWindow().measure().layout()
-    assertThat(lifecycleTracker.steps)
-        .describedAs("Should only call")
-        .containsExactly(LifecycleStep.ON_BIND)
+    val config = testLithoView.lithoView.configuration
+    if (config != null && config.enableFixForIM) {
+      assertThat(lifecycleTracker1.steps)
+          .describedAs("Should only call unbind because we mark it as excluded from IM")
+          .contains(LifecycleStep.ON_UNBIND)
+      assertThat(lifecycleTracker2.steps)
+          .describedAs("Should call unmount as well")
+          .contains(LifecycleStep.ON_UNBIND, LifecycleStep.ON_UNMOUNT)
+    } else {
+      assertThat(lifecycleTracker1.steps)
+          .describedAs("Should call")
+          .contains(LifecycleStep.ON_UNBIND)
+      assertThat(lifecycleTracker2.steps)
+          .describedAs("Should call")
+          .contains(LifecycleStep.ON_UNBIND)
+    }
+    lifecycleTracker1.reset()
+    lifecycleTracker2.reset()
+    testLithoView.attachToWindow().layout()
+
+    if (config != null && config.enableFixForIM) {
+      assertThat(lifecycleTracker1.steps)
+          .describedAs("Should call bind because it's mounted")
+          .containsExactly(LifecycleStep.ON_BIND)
+      assertThat(lifecycleTracker2.steps)
+          .describedAs("Should call nothing because it's still unmounted")
+          .isEmpty()
+    } else {
+      assertThat(lifecycleTracker1.steps)
+          .describedAs("Should call nothing")
+          .containsExactly(LifecycleStep.ON_BIND)
+      assertThat(lifecycleTracker2.steps)
+          .describedAs("Should call")
+          .containsExactly(LifecycleStep.ON_BIND)
+    }
+
+    if (config != null && config.enableFixForIM) {
+      testLithoView.lithoView.notifyVisibleBoundsChanged()
+      assertThat(lifecycleTracker2.steps)
+          .describedAs("Should be mounted as we notify visible bounds changed")
+          .containsExactly(LifecycleStep.ON_MOUNT, LifecycleStep.ON_BIND)
+    }
   }
 
   @Test

litho-rendercore-incremental-mount/src/main/java/com/facebook/rendercore/incrementalmount/IncrementalMountExtension.java

@@ -439,11 +439,21 @@ private static void maybeAcquireReference(
     // override that if the item is outside the visible bounds.
     // TODO (T64830748): extract animations logic out of this.
 
-    final boolean isMountable =
-        isMountedHostWithChildContent(content)
-            || Rect.intersects(localVisibleRect, incrementalMountOutput.getBounds())
-            || isRootItem(id)
-            || incrementalMountOutput.excludeFromIncrementalMount();
+    final boolean isMountable;
+    if (RenderCoreConfig.shouldEnableIMFix) {
+      // We should rely on the excludeFromIncrementalMount flag to determine if a host component is
+      // mountable
+      isMountable =
+          Rect.intersects(localVisibleRect, incrementalMountOutput.getBounds())
+              || incrementalMountOutput.excludeFromIncrementalMount();
+    } else {
+      isMountable =
+          isMountedHostWithChildContent(content)
+              || Rect.intersects(localVisibleRect, incrementalMountOutput.getBounds())
+              || isRootItem(id)
+              || incrementalMountOutput.excludeFromIncrementalMount();
+    }
+
     final boolean hasAcquiredMountRef = extensionState.ownsReference(id);
     if (isMountable && !hasAcquiredMountRef) {
       extensionState.acquireMountReference(incrementalMountOutput.getId(), isMounting);

litho-testing/src/main/java/com/facebook/litho/testing/testrunner/IncrementalMountConfiguration.kt

@@ -17,16 +17,19 @@
 package com.facebook.litho.testing.testrunner
 
 import com.facebook.litho.config.ComponentsConfiguration
+import com.facebook.rendercore.RenderCoreConfig
 import org.junit.runners.model.FrameworkMethod
 
 class IncrementalMountConfiguration : LithoTestRunConfiguration {
 
   override fun beforeTest(method: FrameworkMethod) {
+    RenderCoreConfig.shouldEnableIMFix = true
     ComponentsConfiguration.defaultInstance =
         ComponentsConfiguration.defaultInstance.copy(enableFixForIM = true)
   }
 
   override fun afterTest(method: FrameworkMethod) {
+    RenderCoreConfig.shouldEnableIMFix = false
     ComponentsConfiguration.defaultInstance =
         ComponentsConfiguration.defaultInstance.copy(enableFixForIM = false)
   }