FOP-2918: A surrogate pair is not split by word breaking, and both its units get one bidi level

fop-core/src/main/java/org/apache/fop/complexscripts/bidi/UnicodeBidiAlgorithm.java

@@ -75,7 +75,17 @@ public static int[] resolveLevels(CharSequence cs, Direction defaultLevel) {
      * @param levels array to receive levels, one for each character in chars array
      */
     public static int[] resolveLevels(int[] chars, int defaultLevel, int[] levels) {
-        return resolveLevels(chars, getClasses(chars), defaultLevel, levels, false);
+        resolveLevels(chars, getClasses(chars), defaultLevel, levels, false);
+        // The placeholder that stands for the low surrogate of a pair takes the level of the
+        // character it belongs to, as resolveLevels(CharSequence, Direction) documents. With the
+        // placeholder classed as its character (getClasses) the rules already agree; this holds
+        // the promise whatever a rule does with a repeated class.
+        for (int i = 1, n = chars.length; i < n; i++) {
+            if (chars [ i ] < 0) {
+                levels [ i ] = levels [ i - 1 ];
+            }
+        }
+        return levels;
     }
 
     /**
@@ -618,6 +628,13 @@ private static int[] getClasses(int[] chars) {
             int ch = chars [ i ];
             if (ch >= 0) {
                 bc = BidiClass.getBidiClass(chars [ i ]);
+            } else if (i > 0) {
+                // The placeholder for a low surrogate takes the class of the character it belongs
+                // to, so that the rules see the pair as the one character it is. As a class of its
+                // own it ended a run of neutrals in rule N1, so a neutral outside the BMP inside
+                // right-to-left text fell to the embedding direction where one in the BMP takes the
+                // text's.
+                bc = classes [ i - 1 ];
             } else {
                 bc = SURROGATE;
             }

fop-core/src/main/java/org/apache/fop/layoutmgr/inline/TextLayoutManager.java

@@ -779,6 +779,7 @@ public List<KnuthSequence> getNextKnuthElements(final LayoutContext context, fin
         boolean inWord = false;
         boolean inWhitespace = false;
         char ch = 0;
+        char prevChar = 0;
         int level = -1;
         int prevLevel = -1;
         boolean retainControls = false;
@@ -819,8 +820,9 @@ public List<KnuthSequence> getNextKnuthElements(final LayoutContext context, fin
                 boolean processWord = breakOpportunity
                         || GlyphMapping.isSpace(ch)
                         || CharUtilities.isExplicitBreak(ch)
-                        || ((prevLevel != -1) && (level != prevLevel));
-                if (!processWord && foText.getCommonFont().getFontSelectionStrategy() == EN_CHARACTER_BY_CHARACTER) {
+                        || ((prevLevel != -1) && (level != prevLevel) && !Character.isHighSurrogate(prevChar));
+                if (!processWord && !Character.isHighSurrogate(prevChar)
+                        && foText.getCommonFont().getFontSelectionStrategy() == EN_CHARACTER_BY_CHARACTER) {
                     if (lastFont == null || lastFontPos != nextStart - 1) {
                         lastFont = FontSelector.selectFontForCharactersInText(
                                 foText, nextStart - 1, nextStart, foText, this);
@@ -887,6 +889,7 @@ public List<KnuthSequence> getNextKnuthElements(final LayoutContext context, fin
             inWhitespace = ch == CharUtilities.SPACE
                     && foText.getWhitespaceTreatment() != Constants.EN_PRESERVE;
             prevLevel = level;
+            prevChar = ch;
             nextStart++;
         }
 

fop-core/src/main/java/org/apache/fop/util/CharUtilities.java

@@ -409,7 +409,7 @@ public static boolean containsSurrogatePairAt(CharSequence chars, int index) {
         char ch = chars.charAt(index);
 
         if (Character.isHighSurrogate(ch)) {
-            if ((index + 1) > chars.length()) {
+            if ((index + 1) >= chars.length()) {
                 throw new IllegalArgumentException(
                         "ill-formed UTF-16 sequence, contains isolated high surrogate at end of sequence");
             }

fop-core/src/test/java/org/apache/fop/complexscripts/bidi/SurrogatePairLevelsTestCase.java

@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/* $Id$ */
+
+package org.apache.fop.complexscripts.bidi;
+
+import org.junit.Test;
+import static org.junit.Assert.assertArrayEquals;
+
+import org.apache.fop.traits.Direction;
+
+/**
+ * Both UTF-16 units of a supplementary-plane character must resolve to one bidi level, as
+ * {@link UnicodeBidiAlgorithm#resolveLevels(CharSequence, Direction)} documents. U+10826 is
+ * a Cypriot syllable, a right-to-left script outside the BMP (FOP-2918).
+ */
+public class SurrogatePairLevelsTestCase {
+
+    private static final String CYPRIOT = "𐠦";
+
+    /** U+1F300 CYCLONE, a neutral (ON) outside the BMP that FOP's bidi class table knows as one. */
+    private static final String CYCLONE = "\uD83C\uDF00";
+
+    private static final String SHALOM = "\u05E9\u05DC\u05D5\u05DD";
+
+    private static final String OLAM = "\u05E2\u05D5\u05DC\u05DD";
+
+    @Test
+    public void testPairAlone() {
+        assertArrayEquals(new int[] {1, 1}, UnicodeBidiAlgorithm.resolveLevels(CYPRIOT, Direction.LR));
+    }
+
+    @Test
+    public void testPairBetweenLatinLetters() {
+        assertArrayEquals(new int[] {0, 1, 1, 0},
+                UnicodeBidiAlgorithm.resolveLevels("a" + CYPRIOT + "b", Direction.LR));
+    }
+
+    @Test
+    public void testTwoPairs() {
+        assertArrayEquals(new int[] {1, 1, 1, 1},
+                UnicodeBidiAlgorithm.resolveLevels(CYPRIOT + CYPRIOT, Direction.LR));
+    }
+
+    /**
+     * A neutral outside the BMP inside right-to-left text resolves as a neutral in the BMP does
+     * (U+263A here): it takes the text's direction by rule N1, so the run is not cut in two. The
+     * placeholder for the low surrogate, as a class of its own, ended the run of neutrals, and the
+     * pair fell to the embedding direction; copying the level after resolution cannot mend that.
+     */
+    @Test
+    public void testNeutralPairInsideRightToLeftText() {
+        assertArrayEquals(new int[] {1, 1, 1, 1, 1, 1, 1, 1, 1, 1},
+                UnicodeBidiAlgorithm.resolveLevels(SHALOM + "\u263A " + OLAM, Direction.LR));
+        assertArrayEquals(new int[] {1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1},
+                UnicodeBidiAlgorithm.resolveLevels(SHALOM + CYCLONE + " " + OLAM, Direction.LR));
+        assertArrayEquals(new int[] {1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1},
+                UnicodeBidiAlgorithm.resolveLevels(SHALOM + " " + CYCLONE + " " + OLAM, Direction.LR));
+    }
+
+    /** Between left-to-right and right-to-left text the same neutral takes the embedding direction (N2). */
+    @Test
+    public void testNeutralPairBetweenDirections() {
+        assertArrayEquals(new int[] {0, 0, 0, 0, 0, 1, 1, 1, 1},
+                UnicodeBidiAlgorithm.resolveLevels("ab" + CYCLONE + " " + OLAM, Direction.LR));
+        assertArrayEquals(new int[] {2, 2, 1, 1, 1, 1, 1, 1, 1},
+                UnicodeBidiAlgorithm.resolveLevels("ab" + CYCLONE + " " + OLAM, Direction.RL));
+    }
+}

fop-core/src/test/java/org/apache/fop/render/pdf/PDFEncodingTestCase.java

@@ -117,6 +117,26 @@ public void testPDFEncodingWithNonBMPFont() throws Exception {
         runTest("test-custom-non-bmp-font.fo", testPatterns);
     }
 
+    /**
+     * Test a non-BMP character under per-character font selection. The selected font changes at the
+     * surrogate pair, which must not end the word between the high surrogate and its low surrogate.
+     * Before that was fixed, layout raised IllegalArgumentException and no PDF was produced.
+     *
+     * @throws Exception
+     *              checkstyle wants a comment here, even a silly one
+     */
+    @Test
+    public void testPDFEncodingWithNonBMPFontCharacterByCharacter() throws Exception {
+
+        final String[] testPatterns = {
+            TEST_MARKER + "1", "\uD800\uDF00",
+            TEST_MARKER + "2", "\uD800\uDF00",
+            TEST_MARKER + "3", "\uD800\uDF00",
+        };
+
+        runTest("test-non-bmp-character-by-character.fo", testPatterns);
+    }
+
     /** Test encoding using specified input file and test patterns array */
     private void runTest(String inputFile, String[] testPatterns)
             throws Exception {

fop-core/src/test/java/org/apache/fop/util/CharUtilitiesTestCase.java

@@ -69,4 +69,14 @@ public void testContainsSurrogatePairAtWithMalformedUTF8Sequence() {
 
         CharUtilities.containsSurrogatePairAt(malformedUTF8Sequence, 3);
     }
+
+    /** A high surrogate as the last character is ill-formed UTF-16, so the documented
+     * IllegalArgumentException is what callers must see - not the IndexOutOfBoundsException
+     * that reading one past the end would raise. */
+    @Test(expected = IllegalArgumentException.class)
+    public void testContainsSurrogatePairAtWithIsolatedHighSurrogateAtEndOfSequence() {
+        String isolatedHighSurrogateAtEnd = "012\uD83D";
+
+        CharUtilities.containsSurrogatePairAt(isolatedHighSurrogateAtEnd, 3);
+    }
 }

fop/test/layoutengine/standard-testcases/wordbreak_surrogates.xml

@@ -0,0 +1,47 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  Licensed to the Apache Software Foundation (ASF) under one or more
+  contributor license agreements.  See the NOTICE file distributed with
+  this work for additional information regarding copyright ownership.
+  The ASF licenses this file to You under the Apache License, Version 2.0
+  (the "License"); you may not use this file except in compliance with
+  the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+  -->
+
+<testcase>
+  <info>
+    <p>
+      This test checks that a right to left surrogate pair does not get
+      word broken in the middle causing an exception. This is for issue FOP-2918
+      java.lang.IllegalArgumentException: ill-formed UTF-16 sequence,
+      contains isolated high surrogate at end of sequence.
+    </p>
+  </info>
+  <fo>
+    <fo:root xmlns:fo="http://www.w3.org/1999/XSL/Format">
+      <fo:layout-master-set>
+        <fo:simple-page-master master-name="TestPage">
+          <fo:region-body margin="1cm"/>
+        </fo:simple-page-master>
+      </fo:layout-master-set>
+      <fo:page-sequence master-reference="TestPage">
+        <fo:flow flow-name="xsl-region-body">
+          <fo:block>
+            <fo:block>&#x10826;</fo:block>
+          </fo:block>
+        </fo:flow>
+      </fo:page-sequence>
+    </fo:root>
+  </fo>
+  <checks>
+    <eval expected="1" xpath="count(/areaTree/pageSequence)"/>
+  </checks>
+</testcase>

fop/test/xml/pdf-encoding/test-non-bmp-character-by-character.fo

@@ -0,0 +1,40 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!--
+  Licensed to the Apache Software Foundation (ASF) under one or more
+  contributor license agreements.  See the NOTICE file distributed with
+  this work for additional information regarding copyright ownership.
+  The ASF licenses this file to You under the Apache License, Version 2.0
+  (the "License"); you may not use this file except in compliance with
+  the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+-->
+
+<!--
+	A non-BMP code point under per-character font selection. Helvetica has no
+	glyph for U+10300 and Aegean600 does, so the selected font changes at the
+	surrogate pair. The word must not be ended between the high surrogate and
+	its low surrogate.
+-->
+
+<fo:root xmlns:fo="http://www.w3.org/1999/XSL/Format" font-family="Helvetica, Aegean600">
+  <fo:layout-master-set>
+    <fo:simple-page-master master-name="A4" page-height="29.7cm" page-width="21cm" margin="2cm">
+      <fo:region-body/>
+    </fo:simple-page-master>
+  </fo:layout-master-set>
+  <fo:page-sequence master-reference="A4">
+    <fo:flow flow-name="xsl-region-body" font-selection-strategy="character-by-character">
+      <fo:block>PDFE_TEST_MARK_1: pair between words &#x10300; here</fo:block>
+      <fo:block>PDFE_TEST_MARK_2: pair last in the block &#x10300;</fo:block>
+      <fo:block>PDFE_TEST_MARK_3: consecutive pairs &#x10300;&#x10300;&#x10300;</fo:block>
+    </fo:flow>
+  </fo:page-sequence>
+</fo:root>