8330559: Trailing space not rendering correctly in TextFlow in RTL mode #1468

karthikpandelu · 2024-05-31T14:39:00Z

The issue is specific to Mac. The glyph positions returned from native side for complex text is not handled in the text render logic. This issue is observed only when trailing spaces are present along with LTR text mixed with RTL text (Example: "Arabic: العربية").

Made changes in getPosX of TextRun class to handle negative values.

Tested the changes manually with the sample code present in the bug and using the MonkeyTester.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8330559: Trailing space not rendering correctly in TextFlow in RTL mode (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1468/head:pull/1468
$ git checkout pull/1468

Update a local copy of the PR:
$ git checkout pull/1468
$ git pull https://git.openjdk.org/jfx.git pull/1468/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1468

View PR using the GUI difftool:
$ git pr show -t 1468

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1468.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-05-31T14:39:27Z

👋 Welcome back kpk! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-05-31T14:40:28Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mlbridge · 2024-05-31T14:43:13Z

Webrevs

andy-goryachev-oracle

Seems to work with the example in the ticket and the TextArea in the Monkey Tester. No ill effects on Windows. (Did not test it on Linux)

Could you come up with a unit test?

andy-goryachev-oracle · 2024-05-31T16:54:58Z

modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextRun.java

@@ -329,6 +329,13 @@ public int getWrapIndex(float width) {
                cacheWidth = x;
                return x;
            }
+            int prevIdx = (glyphIndex - 1)<<1;


would it be possible to add a comment explaining why this code is doing what it's doing? specifically, the reason for the while() loop and Math.abs(). and perhaps mention that this happens on mac only.

I think if this is specific to mac, and doesn't occur on other platforms that the supplied positions may be incorrect by the underlying *GlyphLayout code. Under Windows it will likely use the DWGlyphLayout to supply positions. On Mac this will be CTGlyphLayout.

The more prudent course of action than seems to be to fix what is supplied by CTGlyphLayout instead of working around it in TextRun.

I haven't checked it that extensively yet, but I think positions is not supposed to contain negative x values at all when not in compact mode.

Yes the issue is specific to Mac only. Windows uses DWGlyphLayout for positions as you pointed out.
Since the position obtained from the underlying platform itself is not correct Mac, looked like fixing it in either TextRun or in the native side does not make huge difference so went with this approach. However I will explore how we can fix it on the native side.

If the positions is not supposed to contain negative value, it is an issue from Apple not in JavaFX. In this case should we actually fix the issue in JavaFX, as it becomes a workaround rather than a fix regardless of where we fix it.

The *GlyphLayout classes are adapters for a platform. Their job is to call native code, and to convert it to a standard format that JavaFX expects. The native code for Apple is probably perfectly fine, but it is not adapted to the standard format correctly for the case that you discovered. I'm suggesting therefore to change the CTGlyphLayout class to adapt/modify the positions array before it is given to JavaFX code that expects it to be in a specific format.

CTGlyphLayout calls:

run.shape(glyphCount, glyphs, positions, indices);

It should not be too hard to fix positions passed here to be in the correct format so TextRun can remain unchanged.

karthikpandelu · 2024-06-04T05:25:41Z

Seems to work with the example in the ticket and the TextArea in the Monkey Tester. No ill effects on Windows. (Did not test it on Linux)

I tested the issue on Linux. It is not reproducible. So this is specific to Mac only.

Could you come up with a unit test?

I couldn't find a way to write unit test for this. I will explore more to find out if it is possible. Please let me know if you have any idea regarding this.

karthikpandelu · 2024-06-06T14:12:02Z

Fixed the issue in CTGlyphLayout.
@andy-goryachev-oracle , @hjohn please re review

andy-goryachev-oracle · 2024-06-06T14:56:49Z

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

@@ -155,6 +155,20 @@ public void layout(TextRun run, PGFont font, FontStrike strike, char[] text) {
            if (size > 0) {
                positions[posStart] = (float)OS.CTLineGetTypographicBounds(lineRef);
            }
+            /* JDK-8330559 - Mac specific issue.
+             * When traling spces are present in the text containing LTR and RTL


spelling: "trailing spaces"

andy-goryachev-oracle · 2024-06-06T14:59:04Z

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

+            /* JDK-8330559 - Mac specific issue.
+             * When traling spces are present in the text containing LTR and RTL
+             * text together, negative position values are returned for spaces from
+             * the native side. Since TextRun expects positive value relative to the


should we limit the scope of the change to mac only (PlatformUtil.isMac()?) since CTGlyphLayout is common code?

CTGlyphLayout is not common code. It is mac only (so no need to mention mac)

I see, PrismFontFactory:164 getNativeFactoryName().
It would be nice to place platform-specific code in a package bearing the platform name, or at least mention this in a class-level comment, but I guess it's too late.

It means the scope is already limited to macOS, we are good.

I'm having some doubts about the solution still. It is mentioned that this problem can occur in text containing both LTR and RTL text together, however, as far as I can see, a TextRun never contains a mix of text directions. Can you clarify this?

Also in RTL text, I'm guessing "trailing" spaces are in fact what we call leading spaces in LTR text, ie. trailing spaces in a RTL text sample would be: قطة

I'm guessing that CT (Core Text) returns the positions of all the trailing spaces as negative values, not just the first, and the first glyph (seen from the left) with an offset of 0. Since the CT framework can also handle alignments, but FX is not making use of this, I think all calculations are done with the standard left alignment. It would make sense for spaces that "fall off" the left end (because they're "trailing") to have negative values. Similarly, if you did LTR text for cat with a right alignment, all glyphs would have negative values, except the two trailing spaces that would have positive values.

So I'm curious to see what happens if you have more than one trailing space (eg. قطة) and what CTRunGetPositions returns then.

Yes, I am also curious about the exact meaning of these negative values.

I'm having some doubts about the solution still. It is mentioned that this problem can occur in text containing both LTR and RTL text together, however, as far as I can see, a TextRun never contains a mix of text directions. Can you clarify this?

LTR and RTL text will not be together in a TextRun but negative value will not be obtained for spaces in all the cases.
For example: In the the case where Text nodes are present in TextFlow like shown below, none of the spaces will have negative value. In fact CTGlyphLayout class will not be called at all.

flow = new TextFlow( t("Arabic", Color.RED), t(" ", Color.YELLOW), t("Arabic", Color.RED));

If we consider cases like

flow = new TextFlow( t("العربية", Color.GREEN), t(" ", Color.YELLOW)

or

flow = new TextFlow( t(" ", Color.YELLOW), t("العربية", Color.GREEN)

all the spaces will have negative value obtained from the native function CTRunGetPositionsPtr using CTGlyphLayout class.
To make all the negative value positive, I'm adding the least value to all the positions so that lowest value becomes 0 and rest of the positions are shifted by the same offset.

Since the CT framework can also handle alignments, but FX is not making use of this, I think all calculations are done with the standard left alignment. It would make sense for spaces that "fall off" the left end (because they're "trailing") to have negative values. Similarly, if you did LTR text for cat with a right alignment, all glyphs would have negative values, except the two trailing spaces that would have positive values.

I need to check this case. All the testing I have done is by changing the node orientation of scene to RTL mode with above mentioned cases. Did not change text alignments separately.

So I'm curious to see what happens if you have more than one trailing space (eg. قطة) and what CTRunGetPositions returns then.

For this arabic text with scene node orientation set to RTL and default text alignment, trailing spaces get negative values. If the space is added in leading side, then I see positive values.

andy-goryachev-oracle · 2024-06-06T16:53:47Z

Found another problem: the offset of the whole string appears to be incorrect after the fix:

The code to reproduce was

        flow = new TextFlow(
                t("Arabic:", Color.RED),
                t("   ", Color.YELLOW),
                t("العربية", Color.GREEN),
                new Text("\n"),
                t("Hebrew: ", Color.BLUE),
                t("עברית", Color.BLACK));

karthikpandelu · 2024-06-07T07:52:01Z

The code to reproduce was

        flow = new TextFlow(
                t("Arabic:", Color.RED),
                t("   ", Color.YELLOW),
                t("العربية", Color.GREEN),
                new Text("\n"),
                t("Hebrew: ", Color.BLUE),
                t("עברית", Color.BLACK));

I can see issue with this example. I have to check this case.

karthikpandelu · 2024-06-10T19:25:45Z

As I will not be able to work actively on this, converted to draft

karthikpandelu added 2 commits May 31, 2024 16:26

Fix text render issue

cef8789

Fix render issue

25def30

openjdk bot added the rfr Ready for review label May 31, 2024

andy-goryachev-oracle reviewed May 31, 2024

View reviewed changes

Fixing the issue in CTGlyphLayout class

2798cc8

andy-goryachev-oracle reviewed Jun 6, 2024

View reviewed changes

Typo fix

148a109

karthikpandelu marked this pull request as draft June 10, 2024 19:25

openjdk bot removed the rfr Ready for review label Jun 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8330559: Trailing space not rendering correctly in TextFlow in RTL mode #1468

8330559: Trailing space not rendering correctly in TextFlow in RTL mode #1468

karthikpandelu commented May 31, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 31, 2024

openjdk bot commented May 31, 2024

mlbridge bot commented May 31, 2024 •

edited

Loading

andy-goryachev-oracle left a comment

andy-goryachev-oracle May 31, 2024

hjohn Jun 1, 2024

karthikpandelu Jun 4, 2024

hjohn Jun 4, 2024 •

edited

Loading

karthikpandelu commented Jun 4, 2024 •

edited

Loading

karthikpandelu commented Jun 6, 2024

andy-goryachev-oracle Jun 6, 2024

andy-goryachev-oracle Jun 6, 2024 •

edited

Loading

kevinrushforth Jun 6, 2024

andy-goryachev-oracle Jun 6, 2024

hjohn Jun 6, 2024 •

edited

Loading

andy-goryachev-oracle Jun 6, 2024

karthikpandelu Jun 7, 2024

andy-goryachev-oracle commented Jun 6, 2024 •

edited

Loading

karthikpandelu commented Jun 7, 2024

karthikpandelu commented Jun 10, 2024

8330559: Trailing space not rendering correctly in TextFlow in RTL mode #1468

Are you sure you want to change the base?

8330559: Trailing space not rendering correctly in TextFlow in RTL mode #1468

Conversation

karthikpandelu commented May 31, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewing

Webrev

bridgekeeper bot commented May 31, 2024

openjdk bot commented May 31, 2024

mlbridge bot commented May 31, 2024 • edited Loading

Webrevs

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjohn Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

karthikpandelu commented Jun 4, 2024 • edited Loading

karthikpandelu commented Jun 6, 2024

Choose a reason for hiding this comment

andy-goryachev-oracle Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjohn Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Jun 6, 2024 • edited Loading

karthikpandelu commented Jun 7, 2024

karthikpandelu commented Jun 10, 2024

karthikpandelu commented May 31, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented May 31, 2024 •

edited

Loading

hjohn Jun 4, 2024 •

edited

Loading

karthikpandelu commented Jun 4, 2024 •

edited

Loading

andy-goryachev-oracle Jun 6, 2024 •

edited

Loading

hjohn Jun 6, 2024 •

edited

Loading

andy-goryachev-oracle commented Jun 6, 2024 •

edited

Loading