FOP-1896: A font's descender is not taken from a typo descender above the baseline, nor guessed as 0

fop-core/src/main/java/org/apache/fop/fonts/truetype/OpenFont.java

@@ -1605,14 +1605,17 @@ protected boolean readPCLT() throws IOException {
     protected void determineAscDesc() {
         int hheaBoxHeight = hheaAscender - hheaDescender;
         int os2BoxHeight = os2Ascender - os2Descender;
-        if (os2Ascender > 0 && os2BoxHeight <= upem) {
+        // A typo descender above the baseline is a sign error in the font (Wingdings has
+        // sTypoDescender +420 where its hhea descender is -432), so the OS/2 values are not used
+        boolean os2Usable = os2Ascender > 0 && os2Descender <= 0;
+        if (os2Usable && os2BoxHeight <= upem) {
             ascender = os2Ascender;
             descender = os2Descender;
         } else if (hheaAscender > 0 && hheaBoxHeight <= upem) {
             ascender = hheaAscender;
             descender = hheaDescender;
         } else {
-            if (os2Ascender > 0) {
+            if (os2Usable) {
                 //Fall back to info from OS/2 if possible
                 ascender = os2Ascender;
                 descender = os2Descender;
@@ -1673,7 +1676,9 @@ protected void guessVerticalMetricsFromGlyphBBox() {
             log.debug("Ascender from glyph 'd': " + formatUnitsForDebug(localAscender));
             log.debug("Descender from glyph 'p': " + formatUnitsForDebug(localDescender));
         }
-        if (ascender - descender > upem) {
+        // Only values that were found, either side of the baseline: a font without the 'd' and 'p'
+        // glyphs (a symbol font, or one for another script) would otherwise get an ascender and descender of 0
+        if (ascender - descender > upem && localAscender > 0 && localDescender < 0) {
             log.debug("Replacing specified ascender/descender with derived values to get values"
                     + " which fit in the em box.");
             ascender = localAscender;

fop-core/src/test/java/org/apache/fop/fonts/truetype/TTFFileTestCase.java

@@ -25,6 +25,8 @@
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -408,11 +410,12 @@ public void testGetLowerCaseAscent() {
         // Curiously the same value
         assertEquals(droidmonoTTFFile.convertTTFUnit2PDFUnit(1556),
                 droidmonoTTFFile.getLowerCaseAscent());
-        //TODO:  Nedd to be fixed?
-        // 0 because the font miss letter glyph that are used in this method to guess the ascender:
-        // OpenFont.guessVerticalMetricsFromGlyphBBox
-        assertEquals(androidEmojiTTFFile.convertTTFUnit2PDFUnit(0),
+        // The hhea and OS/2 values: the font has no 'd' and 'p' glyphs from which to guess others
+        // (FOP-1896; this was 0)
+        assertEquals(androidEmojiTTFFile.convertTTFUnit2PDFUnit(2200),
                 androidEmojiTTFFile.getLowerCaseAscent());
+        assertEquals(androidEmojiTTFFile.convertTTFUnit2PDFUnit(-650),
+                androidEmojiTTFFile.getLowerCaseDescent());
     }
 
     /**
@@ -653,4 +656,49 @@ public void testSymbolCmap() throws Exception {
         Assert.assertEquals(symbolTTFFile.unicodeMappings.get(1).getGlyphIndex(), 0xF020);
         Assert.assertEquals(symbolTTFFile.unicodeMappings.size(), 2);
     }
+
+    /**
+     * A positive OS/2 sTypoDescender is a sign error in the font (Wingdings has +420 where its hhea
+     * descender is -432) and is not taken as the descender (FOP-1896). AndroidEmoji, with its
+     * sTypoDescender made +650, is Wingdings' case: its hhea box is larger than the em and it has no
+     * 'd' or 'p' glyph, so the hhea values stand. FOP took the OS/2 values, with the descender above
+     * the baseline; rejecting them alone gave an ascender and descender of 0.
+     */
+    @Test
+    public void testPositiveTypoDescenderNotTakenWithoutGlyphsToGuessFrom() throws IOException {
+        TTFFile ttfFile = readWithTypoDescender("test/resources/fonts/ttf/AndroidEmoji.ttf", 650);
+        assertEquals(ttfFile.convertTTFUnit2PDFUnit(2200), ttfFile.getLowerCaseAscent());
+        assertEquals(ttfFile.convertTTFUnit2PDFUnit(-650), ttfFile.getLowerCaseDescent());
+    }
+
+    /**
+     * The Lucida faces' case: DejaVuLGCSerif with its sTypoDescender made +492. Its hhea box is larger
+     * than the em, so the values come from the 'd' and 'p' glyphs, as they do for any such font.
+     */
+    @Test
+    public void testPositiveTypoDescenderNotTakenWithGlyphsToGuessFrom() throws IOException {
+        TTFFile ttfFile = readWithTypoDescender("test/resources/fonts/ttf/DejaVuLGCSerif.ttf", 492);
+        assertEquals(ttfFile.convertTTFUnit2PDFUnit(1556), ttfFile.getLowerCaseAscent());
+        assertEquals(ttfFile.convertTTFUnit2PDFUnit(-426), ttfFile.getLowerCaseDescent());
+    }
+
+    private static TTFFile readWithTypoDescender(String path, int typoDescender) throws IOException {
+        byte[] font = Files.readAllBytes(Paths.get(path));
+        int numTables = ((font[4] & 0xff) << 8) | (font[5] & 0xff);
+        int os2 = -1;
+        for (int i = 0; i < numTables; i++) {
+            int entry = 12 + 16 * i;
+            if (new String(font, entry, 4, "US-ASCII").equals("OS/2")) {
+                os2 = ((font[entry + 8] & 0xff) << 24) | ((font[entry + 9] & 0xff) << 16)
+                        | ((font[entry + 10] & 0xff) << 8) | (font[entry + 11] & 0xff);
+            }
+        }
+        assertTrue(os2 > 0);
+        font[os2 + 70] = (byte) (typoDescender >> 8); // sTypoDescender
+        font[os2 + 71] = (byte) typoDescender;
+        FontFileReader reader = new FontFileReader(new ByteArrayInputStream(font));
+        TTFFile ttfFile = new TTFFile();
+        ttfFile.readFont(reader, OFFontLoader.readHeader(reader));
+        return ttfFile;
+    }
 }