#420 #421 Test, divide by 0 check and refactoring around justification.

+ Further test for justified text with nested non-justified text. + Defence against divide by zero error. + Remove unneeded casts in justify method. + Add comments around #421 + Fixes #420
danfickle · Dec 4, 2019 · 6d3b6b9 · 6d3b6b9
1 parent 19887e0
commit 6d3b6b9
Show file tree

Hide file tree

Showing 5 changed files with 43 additions and 5 deletions.
diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/render/InlineText.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/render/InlineText.java
@@ -314,6 +314,10 @@ public float calcTotalAdjustment(JustificationInfo info) {
         }
 
         if (_counts == null) {
+            // This will only happen for non-justifiable text nested inside
+            // justifiable text (eg. white-space: pre).
+            // Therefore the correct answer is 0.
+            // See InlineLayoutBox#countJustifiableChars.
             return 0f;
         }
 

diff --git a/openhtmltopdf-core/src/main/java/com/openhtmltopdf/render/LineBox.java b/openhtmltopdf-core/src/main/java/com/openhtmltopdf/render/LineBox.java
@@ -42,7 +42,6 @@
 import com.openhtmltopdf.layout.Layer;
 import com.openhtmltopdf.layout.LayoutContext;
 import com.openhtmltopdf.layout.PaintingInfo;
-import com.openhtmltopdf.render.FlowingColumnContainerBox.ColumnBreakStore;
 import com.openhtmltopdf.util.XRRuntimeException;
 
 /**
@@ -217,7 +216,6 @@ public void align(boolean dynamic, CssContext c) {
     }
 
     public void justify(CssContext c) {
-
         if (getParent().getStyle().hasLetterSpacing()) {
             // Do nothing, letter-spacing turns off text justification.
         } else if (! isLastLineWithContent()) {
@@ -239,19 +237,22 @@ public void justify(CssContext c) {
 
                 if (counts.getSpaceCount() > 0) {
                     if (counts.getNonSpaceCount() > 1) {
-                        info.setNonSpaceAdjust(Math.min((float)toAdd * JUSTIFY_NON_SPACE_SHARE / (counts.getNonSpaceCount()-1), maxInterChar));
+                        info.setNonSpaceAdjust(Math.min(toAdd * JUSTIFY_NON_SPACE_SHARE / (counts.getNonSpaceCount() - 1), maxInterChar));
                     } else {
                         info.setNonSpaceAdjust(0.0f);
                     }
 
                     if (counts.getSpaceCount() > 0) {
-                        info.setSpaceAdjust(Math.min((float)toAdd * JUSTIFY_SPACE_SHARE / counts.getSpaceCount(), maxInterWord));
+                        info.setSpaceAdjust(Math.min(toAdd * JUSTIFY_SPACE_SHARE / counts.getSpaceCount(), maxInterWord));
                     } else {
                         info.setSpaceAdjust(0.0f);
                     }
-                } else {
+                } else if (counts.getNonSpaceCount() > 1) {
                     info.setSpaceAdjust(0f);
                     info.setNonSpaceAdjust(Math.min((float) toAdd / (counts.getNonSpaceCount() - 1), maxInterChar)); 
+                } else {
+                    info.setSpaceAdjust(0f);
+                    info.setNonSpaceAdjust(0f);
                 }
 
                 adjustChildren(info);

diff --git a/...xamples/src/main/resources/visualtest/expected/issue-420-justify-text-white-space-pre.pdf b/...xamples/src/main/resources/visualtest/expected/issue-420-justify-text-white-space-pre.pdf
diff --git a/...f-examples/src/main/resources/visualtest/html/issue-420-justify-text-white-space-pre.html b/...f-examples/src/main/resources/visualtest/html/issue-420-justify-text-white-space-pre.html
@@ -0,0 +1,19 @@
+<html>
+<head>
+<style>
+@page {
+  size: 100px 200px;
+  margin: 0;
+}
+body {
+  margin: 0;
+  padding: 0;
+}
+</style>
+</head>
+<body>
+<p style="text-align: justify;">
+This is some text with a <br/><span style="white-space: pre;">white</span> space pre in the middle. Will it justify correctly?
+</p>
+</body>
+</html>
diff --git a/...-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java b/...-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java
@@ -924,10 +924,24 @@ public void testIssue399TableHeaderFooterWithNoRows() throws IOException {
     }
 
 
+    /**
+     * Tests that justified text with non-justified content (br) nested inside it
+     * will not throw a NPE.
+     * https://github.com/danfickle/openhtmltopdf/issues/420
+     */
     @Test
     public void testIssue420JustifyTextNullPointerException() throws IOException {
         assertTrue(vt.runTest("issue-420-justify-text-null-pointer-exception"));
     }
+
+    /**
+     * Tests that justified text with non-justified content nested inside it
+     * correctly justifies.
+     */
+    @Test
+    public void testIssue420JustifyTextWhiteSpacePre() throws IOException {
+        assertTrue(vt.runTest("issue-420-justify-text-white-space-pre"));
+    }
 
     // TODO:
     // + Elements that appear just on generated overflow pages.