#417 Keep aspect ratio of image with width/height and max values.

This differs from browsers which will warp the aspect ratio in such circumstances (tested with Chrome/Safari). However, nearly all uses need the aspect ratio preserved. We can revisit this if we implement the object-fit property. Thanks a lot to @swarl who provided code which inspired the test and fix for this issue, as well as reporting the problem.
danfickle · Jan 14, 2020 · 7c4a95a · 7c4a95a · swarl · Jan 15, 2020
1 parent c520a52
commit 7c4a95a
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 12 deletions.
diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/render/BlockBox.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/render/BlockBox.java
@@ -745,20 +745,46 @@ private void sizeReplacedElement(LayoutContext c, ReplacedElement re) {
 
         boolean haveExactDims = cssWidth >= 0 && cssHeight >= 0;
 
+        boolean usedMinWidth = false;
+        boolean usedMinHeight = false;
+        boolean usedMaxWidth = false;
+        boolean usedMaxHeight = false;
+
         int intrinsicWidth = re.getIntrinsicWidth();
         int intrinsicHeight = re.getIntrinsicHeight();
 
-        cssWidth = !getStyle().isMaxWidthNone() && 
-                (intrinsicWidth > getCSSMaxWidth(c) || cssWidth > getCSSMaxWidth(c)) ? 
-                          getCSSMaxWidth(c) : cssWidth;
-        cssWidth = cssWidth >= 0 && getCSSMinWidth(c) > 0 && cssWidth < getCSSMinWidth(c) ?
-                          getCSSMinWidth(c) : cssWidth;
+        int minWidth = getCSSMinWidth(c);
+        int minHeight = getCSSMinHeight(c);
+
+        // Clamp w to max-width if required.
+        if (!getStyle().isMaxWidthNone() &&
+            (intrinsicWidth > getCSSMaxWidth(c) || cssWidth > getCSSMaxWidth(c))) {
+            cssWidth = getCSSMaxWidth(c);
+            usedMaxWidth = true;
+        }
+
+        // Clamp w to min-width if required.
+        if (cssWidth >= 0 &&
+            minWidth > 0 &&
+            cssWidth < minWidth) {
+            cssWidth = minWidth;
+            usedMinWidth = true;
+        }
 
-        cssHeight = !getStyle().isMaxHeightNone() &&
-                (intrinsicHeight > getCSSMaxHeight(c) || cssHeight > getCSSMaxHeight(c)) ?
-                          getCSSMaxHeight(c) : cssHeight;
-        cssHeight = cssHeight >= 0 && getCSSMinHeight(c) > 0 && cssHeight < getCSSMinHeight(c) ?
-                          getCSSMinHeight(c) : cssHeight;
+        // Clamp h to max-height if required.
+        if (!getStyle().isMaxHeightNone() &&
+            (intrinsicHeight > getCSSMaxHeight(c) || cssHeight > getCSSMaxHeight(c))) {
+            cssHeight = getCSSMaxHeight(c);
+            usedMaxHeight = true;
+        }
+
+        // Clamp h to min-height if required.
+        if (cssHeight >= 0 &&
+            minHeight > 0 && 
+            cssHeight < minHeight) {
+            cssHeight = minHeight;
+            usedMinHeight = true;
+        }
 
         if (getStyle().isBorderBox()) {
             BorderPropertySet border = getBorder(c);
@@ -770,9 +796,13 @@ private void sizeReplacedElement(LayoutContext c, ReplacedElement re) {
         int nw;
         int nh;
 
+        boolean useExact = 
+                (haveExactDims && !usedMaxHeight && !usedMaxWidth && !usedMinWidth && !usedMinHeight);
+
         if (cssWidth > 0 && cssHeight > 0) {
-            if (haveExactDims) {
-                // We only warp the aspect ratio if we have explicit width and height values.
+            if (useExact) {
+                // We can warp the aspect ratio if we have explicit width and height values
+                // and the max/min values have not taken precedence.
                 nw = cssWidth;
                 nh = cssHeight;
             } else if (intrinsicWidth > cssWidth || intrinsicHeight > cssHeight) {

diff --git a/...rc/main/resources/visualtest/expected/issue-417-replaced-sizing-width-height-with-max.pdf b/...rc/main/resources/visualtest/expected/issue-417-replaced-sizing-width-height-with-max.pdf
diff --git a/...s/src/main/resources/visualtest/html/issue-417-replaced-sizing-width-height-with-max.html b/...s/src/main/resources/visualtest/html/issue-417-replaced-sizing-width-height-with-max.html
@@ -0,0 +1,20 @@
+<html>
+<head>
+    <style>
+        @page {
+            size: 300px 500px;
+            margin: 0;
+        }
+        body {
+            max-width: 300px;
+            margin: 0;
+            padding: 0;
+        }
+    </style>
+</head>
+<body>
+   <img src="../../demos/images/portrait-shuttle.jpg"
+        width="640" height="975"
+        style="max-width: 300px; max-height: 300px;"/>
+</body>
+</html>
diff --git a/...-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java b/...-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java
@@ -695,6 +695,16 @@ public void testReplacedImgInTableCell3() throws IOException {
         assertTrue(vt.runTest("replaced-img-in-table-cell-3"));
     }
 
+    /**
+     * Tests that an image with width and height specified but max-width or max-height
+     * takes precedence, that the image will keep its aspect ratio.
+     * https://github.com/danfickle/openhtmltopdf/issues/417
+     */
+    @Test
+    public void testIssue417ReplacedSizingWidthHeightWithMax() throws IOException {
+        assertTrue(vt.runTest("issue-417-replaced-sizing-width-height-with-max"));
+    }
+
     /**
      * Tests that a fixed position element correctly resizes to the sum of its child boxes
      * using border-box sizing.