-
Notifications
You must be signed in to change notification settings - Fork 460
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.
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
8330559: Trailing space not rendering correctly in TextFlow in RTL mode #1468
base: master
Are you sure you want to change the base?
8330559: Trailing space not rendering correctly in TextFlow in RTL mode #1468
Conversation
👋 Welcome back kpk! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -329,6 +329,13 @@ public int getWrapIndex(float width) { | |||
cacheWidth = x; | |||
return x; | |||
} | |||
int prevIdx = (glyphIndex - 1)<<1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I tested the issue on Linux. It is not reproducible. So this is specific to Mac only.
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. |
Fixed the issue in |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling: "trailing spaces"
/* 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 whatCTRunGetPositions
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.
I can see issue with this example. I have to check this case. |
As I will not be able to work actively on this, converted to draft |
@karthikpandelu This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
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
ofTextRun
class to handle negative values.Tested the changes manually with the sample code present in the bug and using the MonkeyTester.
Progress
Issue
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