Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cef8789
Fix text render issue
karthikpandelu May 31, 2024
25def30
Fix render issue
karthikpandelu May 31, 2024
2798cc8
Fixing the issue in CTGlyphLayout class
karthikpandelu Jun 6, 2024
148a109
Typo fix
karthikpandelu Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions modules/javafx.graphics/src/main/java/com/sun/javafx/font/coretext/CTGlyphLayout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 trailing spaces 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
Copy link
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@hjohn hjohn Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* first glyph in the text run, shifting the position of each glyph by the
* absolute value of negative value of first character */
if (positions[0] < 0) {
float offsetVal = Math.abs(positions[0]);
int idx = 0;
while (idx < positions.length - 2) {
positions[idx] += offsetVal;
idx += 2;
}
}
run.shape(glyphCount, glyphs, positions, indices);
}
OS.CFRelease(lineRef);
Expand Down