RFR: 8246104: Some complex text doesn't render correctly on macOS

Tue Apr 4 18:49:17 UTC 2023

On Fri, 24 Mar 2023 21:37:16 GMT, Phil Race <prr at openjdk.org> wrote:

> This PR addresses some font problems on macOS.
> (1) Garbled Arabic with the System font
> (2) Non-ideal fallback fonts for all fonts
> (3) No bold for System font.
> 
> In particular the standard System Font was garbling Arabic text - random glyphs from another font.
> The root of this issue is that several releases ago macOS introduced UI fonts that are hidden from
> enumeration. must be loaded by a special API and cannot be loaded by name, even if you extract
> the name from the core text font loaded by that special API.
> These fonts usually have a name that begins with "." such as the postscript name ".AppleUISystemFont"
> and "name" for the main instance of this will be "System Font Regular". These live in files called "SFNS.ttf"
> and "SFArabic.ttf" and similar.
> 
> JavaFX has its own "System" font family logical name and in order to map to the macOS system font,
> several releases ago we started using the special API referenced above.
> 
> Normally we have our own list of fall back fonts for other scripts.
> But when we encounter complex scripts such as Arabic, and ask CoreText to layout the glyphs,
> it may use other system fonts for Arabic (eg ".SFArabic") and the FX code dynamically adds
> this new font to its fallback list - by name.
> But this new font can't be directly referenced by name either, but that's what used to work and the FX code tries.
> 
> So to fix this we need to grab the reference to the font that was returned by CoreText and use
> that to create the PrismFont we add as a fall back.
> The code that first recognises it needs to go this route is in CTGlyphLayout.java when
> "getSlotForFont(String name") fails.
> Instead it calls new code in CTFactory which in turn calls a new constructor and supporting code in CTFontFile.java.
> There's also supporting changes in native code - coretext.c
> 
> Additionally to support that when we ask the platform to enumerate fallbacks, we might get such "." fonts,
> we need to make some more changes
> Now on to more "fallback" enhancements.
> Prior to this fix, on macOS we had a short, hard-coded global list.
> This fix calls the macOS API to get the cascading fallback list for a font.
> This improves the fallback behaviour in respect to providing
> (1) More appropriate styles, (2) More supported code pointd, (3) Code point support coming from a preferred font.
> There was some desirable re-factoring as part of this, as the location of fallbacks was pushed from
> "if <windows> else if <linux> else <mac> in shared code, down into the appropriate platform factory
> 
> This also made it easier to do some required changes to allow fallbacks to be pre-initialised with the
> native font resource, not just a name+ file. The new class FontFallbackInfo encapsulates this and changes
> were made to follow it.
> In practice on macOS it seems that some "." cascading fallbacks for the "." system font come with a "NULL"
> file name. Without being able to identify the file we aren't able to use the font to skip them.
> More changes would be needed to provide a PrismFont subclass that can handle this case.
> Fortunately that isn't happening for any of the fallbacks we get from complex text layout.
> 
> The final thing this fixes is that "System Bold" was the same as "System Regular".
> The needed name getting lost along the way to constructing the native font.
> 
> The work done here also highlighted that we really need to add code that at least recognises OpenType variation fonts.
> And some day after that we could expose API that allows apps to request non-named variations.

some minor comments; overall - a great improvement.
thank you, Phil!

I want to share two observations that came out of testing with a RichTextArea prototype.

1. I've got java.lang.OutOfMemoryError: Java heap space
UNSUPPORTED (log once): POSSIBLE ISSUE: unit 0 GLD_TEXTURE_INDEX_2D is unloadable and bound to sampler type (Float) - using zero texture because texture unloadable

<img width="1199" alt="Screenshot 2023-04-04 at 07 55 50" src="https://user-images.githubusercontent.com/107069028/229888096-277e5257-193c-4c13-b32d-2ae7f012d792.png">

I suspect it happened due to instrumentation and/or a breakpoint (I was running in Eclipse debugger and Scenic View 11.0.2 attached).  I can't reproduce the issue anymore.

2. the following sequence in the RichTextAreaDemo produces an unexpected result:
- type "bold plain", select "bold" and make it bold, select all set size 300%, change font to Courier New
 Result: the bold segment changes to regular and the regular changes to bold.
It's highly probably I have a bug in my code somewhere, mentioning it here just in case.

modules/javafx.graphics/src/main/java/com/sun/javafx/font/FallbackResource.java line 257:

> 255:              */
> 256:             if (PrismFontFactory.debugFonts) {
> 257:                 System.err.println("\tToo many font fallbacks!");

would it be better to use logging?

modules/javafx.graphics/src/main/java/com/sun/javafx/font/FallbackResource.java line 376:

> 374:            s+= "Slot " + i + "=null\n";
> 375:           } else {
> 376:           s += "Slot " + i + "="+getSlotResource(i).getFullName()+"\n";

minor: spaces and indentation

modules/javafx.graphics/src/main/java/com/sun/javafx/font/FallbackResource.java line 379:

> 377:           }
> 378:         }
> 379:         s+= "\n";

perhaps it might be better to use a StringBuilder here

modules/javafx.graphics/src/main/java/com/sun/javafx/font/FontFallbackInfo.java line 34:

> 32:    ArrayList<String> linkedFontFiles;
> 33:    ArrayList<String> linkedFontNames;
> 34:    ArrayList<FontResource> linkedFonts;

could these three be `private final`?

modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontFile.java line 41:

> 39: class CTFontFile extends PrismFontFile {
> 40: 
> 41:     private long cgFontRef = 0;

minor: the assignment is unnecessary

modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontFile.java line 65:

> 63:     }
> 64: 
> 65:     private long ctFontRef = 0;

minor: I'd add a blank line, remove the assignment.

modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontFile.java line 89:

> 87:         if (name != null) {
> 88:             String family = getFamilyName();
> 89:             if (family.equals("System Font")) {

very minor: I'd do `System Font".equals(...)`

modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTFontFile.java line 104:

> 102:     public boolean isBold() {
> 103:         // Need to do this until we add font variation support into the super-class
> 104:         return fullName.equals("System Font Bold") || super.isBold();

same here.  I am sure fullName cannot be null

modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTGlyphLayout.java line 83:

> 81:             if (fr == null) return -1;
> 82:             slot = fr.getSlotForFont(fontName);
> 83:             if (slot == -1) {

minor: may be `< 0`?

modules/javafx.graphics/src/main/java/com/sun/javafx/font/directwrite/DWFactory.java line 192:

> 190:         // CJK Ext B Supplementary character fallbacks.
> 191:         info.add("MingLiU-ExtB", getPathNameWindows("mingliub.ttc"), null);
> 192:         info.add("Segoe UI Symbol", getPathNameWindows("seguisym.ttf"), null);

a question: what if these fonts are *not* available in a particular system?  will the code break or it will be able to handle non-existing entry?

-------------

Marked as reviewed by angorya (Committer).

PR Review: https://git.openjdk.org/jfx/pull/1067#pullrequestreview-1371535673
PR Comment: https://git.openjdk.org/jfx/pull/1067#issuecomment-1496439369
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157609552
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157611498
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157612031
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157613189
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157624589
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157625222
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157625859
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157626171
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157627666
PR Review Comment: https://git.openjdk.org/jfx/pull/1067#discussion_r1157630093