Fix aspect ratio checking in MeasureUtils

Summary: As per title, we've detected multiple bad tests, which have been fixed in previous diffs. Reviewed By: zielinskimz Differential Revision: D51978982 fbshipit-source-id: 1efa62e263a6e24d48d57c1635eb1cf5f42abb44
facebook · Dec 11, 2023 · f4d1e05 · f4d1e05
1 parent 2c488aa
commit f4d1e05
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 6 deletions.
diff --git a/litho-core/src/main/java/com/facebook/litho/utils/MeasureUtils.java b/litho-core/src/main/java/com/facebook/litho/utils/MeasureUtils.java
@@ -140,7 +140,7 @@ public static void measureWithEqualDimens(int widthSpec, int heightSpec, Size ou
    * @param heightSpec A SizeSpec for the height
    * @param intrinsicWidth A pixel value for the intrinsic width of the measured component
    * @param intrinsicHeight A pixel value for the intrinsic height of the measured component
-   * @param aspectRatio The aspect ration size against
+   * @param aspectRatio The aspect ratio size against
    * @param outputSize The output size of this measurement
    */
   public static void measureWithAspectRatio(
@@ -170,7 +170,7 @@ public static void measureWithAspectRatio(
    * @param heightSpec A SizeSpec for the height
    * @param intrinsicWidth A pixel value for the intrinsic width of the measured component
    * @param intrinsicHeight A pixel value for the intrinsic height of the measured component
-   * @param aspectRatio The aspect ration size against
+   * @param aspectRatio The aspect ratio size against
    */
   public static MeasureResult measureResultUsingAspectRatio(
       final int widthSpec,
@@ -192,14 +192,21 @@ public static MeasureResult measureResultUsingAspectRatio(
    *
    * @param widthSpec A SizeSpec for the width
    * @param heightSpec A SizeSpec for the height
-   * @param aspectRatio The aspect ration size against
+   * @param aspectRatio The aspect ratio size against
    * @param outputSize The output size of this measurement
    */
   public static void measureWithAspectRatio(
       int widthSpec, int heightSpec, float aspectRatio, Size outputSize) {
 
-    if (aspectRatio < 0) {
-      throw new IllegalArgumentException("The aspect ratio must be a positive number");
+    if (Float.isNaN(aspectRatio)
+        || Float.isInfinite(aspectRatio)
+        || Float.compare(aspectRatio, 0) <= 0) {
+      outputSize.width = -1;
+      outputSize.height = -1;
+      if (ComponentsConfiguration.IS_INTERNAL_BUILD) {
+        Log.d(TAG, String.format("The aspect ratio:[%f] must be > 0", aspectRatio));
+      }
+      return;
     }
 
     final int widthMode = SizeSpec.getMode(widthSpec);

diff --git a/litho-it/src/test/java/com/facebook/litho/utils/MeasureUtilsTest.java b/litho-it/src/test/java/com/facebook/litho/utils/MeasureUtilsTest.java
@@ -47,6 +47,28 @@ public void testWidthExactlyHeightAtMost() {
     assertThat(size.height).isEqualTo(20);
   }
 
+  public void testIncorrectAspectRatioWithZero() {
+    final Size size = new Size();
+    measureWithAspectRatio(makeSizeSpec(10, EXACTLY), makeSizeSpec(30, AT_MOST), 0f, size);
+    assertThat(size.width).isEqualTo(-1);
+    assertThat(size.height).isEqualTo(-1);
+  }
+
+  public void testIncorrectAspectRatioWithNaN() {
+    final Size size = new Size();
+    measureWithAspectRatio(makeSizeSpec(10, EXACTLY), makeSizeSpec(30, AT_MOST), Float.NaN, size);
+    assertThat(size.width).isEqualTo(-1);
+    assertThat(size.height).isEqualTo(-1);
+  }
+
+  public void testIncorrectAspectRatioWithInfinity() {
+    final Size size = new Size();
+    measureWithAspectRatio(
+        makeSizeSpec(10, EXACTLY), makeSizeSpec(30, AT_MOST), Float.POSITIVE_INFINITY, size);
+    assertThat(size.width).isEqualTo(-1);
+    assertThat(size.height).isEqualTo(-1);
+  }
+
   @Test
   public void testWidthExactlyHeightUnspecified() {
     final Size size = new Size();

diff --git a/litho-rendercore/src/main/java/com/facebook/rendercore/utils/SizeUtils.kt b/litho-rendercore/src/main/java/com/facebook/rendercore/utils/SizeUtils.kt
@@ -133,7 +133,7 @@ inline fun Size.Companion.withAspectRatio(
  *   [Size.Invalid]
  */
 fun Size.Companion.withAspectRatio(sizeConstraints: SizeConstraints, aspectRatio: Float): Size {
-  if (aspectRatio <= 0f) {
+  if (aspectRatio.isNaN() || aspectRatio.isInfinite() || (aspectRatio.compareTo(0) <= 0f)) {
     return Size.Invalid
   }
 

diff --git a/litho-rendercore/src/test/java/com/facebook/rendercore/utils/SizeUtilsTest.kt b/litho-rendercore/src/test/java/com/facebook/rendercore/utils/SizeUtilsTest.kt
@@ -296,6 +296,22 @@ class SizeUtilsTest {
                 height = sizeConstraints.MaxPossibleHeightValue))
   }
 
+  @Test
+  fun `aspect ratio - with ratio of NaN - returns Invalid Size`() {
+    val sizeConstraints =
+        SizeConstraints(minWidth = 0, maxWidth = 100, minHeight = 0, maxHeight = 100)
+    assertThat(Size.withAspectRatio(sizeConstraints, aspectRatio = Float.NaN))
+        .isEqualTo(Size.Invalid)
+  }
+
+  @Test
+  fun `aspect ratio - with ratio of Infinity - returns Invalid Size`() {
+    val sizeConstraints =
+        SizeConstraints(minWidth = 0, maxWidth = 100, minHeight = 0, maxHeight = 100)
+    assertThat(Size.withAspectRatio(sizeConstraints, aspectRatio = Float.POSITIVE_INFINITY))
+        .isEqualTo(Size.Invalid)
+  }
+
   @Test
   fun `equal dimensions - with bounded width and bounded height - returns size with equal dimensions`() {
     assertThat(