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

danfickle · danfickle · commit 6d3b6b9bc712 · 2019-12-04T23:10:57.000+11:00
+ 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
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/openhtmltopdf-examples/src/main/resources/visualtest/expected/issue-420-justify-text-white-space-pre.pdf b/openhtmltopdf-examples/src/main/resources/visualtest/expected/issue-420-justify-text-white-space-pre.pdf
diff --git a/openhtmltopdf-examples/src/main/resources/visualtest/html/issue-420-justify-text-white-space-pre.html b/openhtmltopdf-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/openhtmltopdf-examples/src/test/java/com/openhtmltopdf/visualregressiontests/VisualRegressionTest.java b/openhtmltopdf-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.

Original file line number	Diff line number	Diff line change
`@@ -314,6 +314,10 @@ public float calcTotalAdjustment(JustificationInfo info) {`
`314`	`314`	`}`
`315`	`315`
`316`	`316`	`if (_counts == null) {`
	`317`	`+ // This will only happen for non-justifiable text nested inside`
	`318`	`+ // justifiable text (eg. white-space: pre).`
	`319`	`+ // Therefore the correct answer is 0.`
	`320`	`+ // See InlineLayoutBox#countJustifiableChars.`
`317`	`321`	`return 0f;`
`318`	`322`	`}`
`319`	`323`