Skip to content

Commit

Permalink
Fix the SizeSpec of AT_MOST is not reset correctly
Browse files Browse the repository at this point in the history
Summary:
# Context
This issue is causing a crash(https://www.internalfb.com/logview/facebook_android_crashes/c0538e852d03501c9aaddfb4a2669bff), which is because we didn't reset auto size for Yoga correctly.

According to the doc(https://www.w3.org/TR/css-sizing-3/#sizing-properties), the default value of width/height for `YogaNode` should be `AUTO` instead of `UNDEFINED`. Otherwise, Yoga will use an invalid SizeSpec to re-measure RecyclerCollectionComponent and lead to a crash in the end.

Reviewed By: adityasharat

Differential Revision: D45947858

fbshipit-source-id: 7ad62b6f9a0db3d5ea7eaf65fccd1a0c90665f44
  • Loading branch information
Andrew Wang authored and facebook-github-bot committed May 17, 2023
1 parent 5845ff6 commit dc1bc0f
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 12 deletions.
6 changes: 2 additions & 4 deletions litho-core/src/main/java/com/facebook/litho/LithoNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -511,12 +511,10 @@ private static void resetSizeIfNecessary(
}
final YogaNode yogaNode = layoutResult.getYogaNode();
if (Float.compare(layoutResult.getWidthFromStyle(), yogaNode.getWidth().value) != 0) {
yogaNode.setWidth(YogaConstants.UNDEFINED);
yogaNode.setMaxWidth(YogaConstants.UNDEFINED);
yogaNode.setWidthAuto();
}
if (Float.compare(layoutResult.getHeightFromStyle(), yogaNode.getHeight().value) != 0) {
yogaNode.setHeight(YogaConstants.UNDEFINED);
yogaNode.setMaxHeight(YogaConstants.UNDEFINED);
yogaNode.setHeightAuto();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,15 +274,15 @@ class IncrementalMountTest {
assertThat(getCountOfLifecycleSteps(lifecycleTracker3.steps, ON_UNMOUNT)).isEqualTo(0)
assertThat(getCountOfLifecycleSteps(lifecycleTracker3.steps, ON_MOUNT)).isEqualTo(0)
}
}

/** Returns the amount of steps that match the given step in the given list of steps */
private fun getCountOfLifecycleSteps(steps: List<LifecycleStep>, step: LifecycleStep): Int {
var count = 0
for (i in steps.indices) {
if (steps[i] == step) {
count++
}
/** Returns the amount of steps that match the given step in the given list of steps */
fun getCountOfLifecycleSteps(steps: List<LifecycleStep>, step: LifecycleStep): Int {
var count = 0
for (i in steps.indices) {
if (steps[i] == step) {
count++
}
return count
}
return count
}
114 changes: 114 additions & 0 deletions litho-it/src/test/java/com/facebook/litho/LayoutCachingTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@
package com.facebook.litho

import com.facebook.litho.config.ComponentsConfiguration
import com.facebook.litho.sections.SectionContext
import com.facebook.litho.sections.common.DynamicComponentGroupSection
import com.facebook.litho.sections.widget.ListRecyclerConfiguration
import com.facebook.litho.sections.widget.RecyclerBinderConfiguration
import com.facebook.litho.sections.widget.RecyclerCollectionComponent
import com.facebook.litho.sections.widget.RecyclerConfiguration
import com.facebook.litho.testing.LegacyLithoViewRule
import com.facebook.litho.testing.exactly
import com.facebook.litho.testing.testrunner.LithoTestRunner
Expand Down Expand Up @@ -186,4 +192,112 @@ class LayoutCachingTest {
legacyLithoViewRule.setSizeSpecs(exactly(200), exactly(200)).measure().layout()
Assertions.assertThat(lifecycleTracker.steps).doesNotContain(LifecycleStep.ON_MEASURE)
}

/**
* Context: With layout caching we need to reset the size spec of the root component if measured
* size is different from specified one, to re-measure the whole tree. This case is to ensure that
* the SizeSpec of AT_MOST is also being reset correctly.
*/
@Test
fun `RecyclerCollectionComponent with wrapContent should be re-measured with the latest size specs when it changes`() {
if (!ComponentsConfiguration.enableLayoutCaching) {
return
}

val context = legacyLithoViewRule.context
val lifecycleTracker1 = LifecycleTracker()
val lifecycleTracker2 = LifecycleTracker()
val lifecycleTracker3 = LifecycleTracker()

val component =
buildRecyclerCollectionComponent(
context, lifecycleTracker1, lifecycleTracker2, lifecycleTracker3)

// Set LithoView with height so that it can fully show all the items
legacyLithoViewRule
.setRoot(component)
.attachToWindow()
.setSizeSpecs(exactly(100), unspecified())
.measure()
.layout()

Assertions.assertThat(
getCountOfLifecycleSteps(lifecycleTracker1.steps, LifecycleStep.ON_MEASURE))
.isEqualTo(5)
Assertions.assertThat(
getCountOfLifecycleSteps(lifecycleTracker2.steps, LifecycleStep.ON_MEASURE))
.isEqualTo(5)
Assertions.assertThat(
getCountOfLifecycleSteps(lifecycleTracker3.steps, LifecycleStep.ON_MEASURE))
.isEqualTo(5)

val height = legacyLithoViewRule.lithoView.height

lifecycleTracker1.reset()
lifecycleTracker2.reset()
lifecycleTracker3.reset()

val measuredSize = Size()
legacyLithoViewRule.lithoView.componentTree?.setSizeSpec(
exactly(200), unspecified(), measuredSize)
Assertions.assertThat(measuredSize.height).isEqualTo(height)
Assertions.assertThat(
getCountOfLifecycleSteps(lifecycleTracker1.steps, LifecycleStep.ON_MEASURE))
.isEqualTo(5)
.describedAs("Ensure that all children are re-measured")
Assertions.assertThat(
getCountOfLifecycleSteps(lifecycleTracker2.steps, LifecycleStep.ON_MEASURE))
.isEqualTo(5)
.describedAs("Ensure that all children are re-measured")
Assertions.assertThat(
getCountOfLifecycleSteps(lifecycleTracker3.steps, LifecycleStep.ON_MEASURE))
.isEqualTo(5)
.describedAs("Ensure that all children are re-measured")
}

private fun buildRecyclerCollectionComponent(
context: ComponentContext,
lifecycleTracker1: LifecycleTracker,
lifecycleTracker2: LifecycleTracker,
lifecycleTracker3: LifecycleTracker,
): RecyclerCollectionComponent {

val child1: Component =
MountSpecLifecycleTester.create(context)
.intrinsicSize(Size(10, 10))
.lifecycleTracker(lifecycleTracker1)
.build()
val child2: Component =
MountSpecLifecycleTester.create(context)
.intrinsicSize(Size(10, 15))
.lifecycleTracker(lifecycleTracker2)
.build()
val child3: Component =
MountSpecLifecycleTester.create(context)
.intrinsicSize(Size(10, 20))
.lifecycleTracker(lifecycleTracker3)
.build()

val item: Component =
Column.create(context)
.child(Wrapper.create(context).delegate(child1))
.child(Wrapper.create(context).delegate(child2))
.child(Wrapper.create(context).delegate(child3))
.build()
val config: RecyclerConfiguration =
ListRecyclerConfiguration.create()
.recyclerBinderConfiguration(
RecyclerBinderConfiguration.create().wrapContent(true).build())
.build()
val sectionContext = SectionContext(context)
return RecyclerCollectionComponent.create(context)
.recyclerConfiguration(config)
.section(
DynamicComponentGroupSection.create(sectionContext)
.component(item)
.totalItems(5)
.build())
.maxHeightDip(300f)
.build()
}
}

0 comments on commit dc1bc0f

Please sign in to comment.