Skip to content

Conversation

@Ziad-Mid
Copy link
Contributor

@Ziad-Mid Ziad-Mid commented Nov 26, 2025

Fix trailing space present for complex text ( LTR text with RTL text ) Example: "Arabic: العربية"
Added case to handle complex text in getPosX of TextRun
Tested the changes with the code present in the bug.


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/1988/head:pull/1988
$ git checkout pull/1988

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1988

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

Using diff file

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

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 26, 2025

👋 Welcome back zelmidaoui! 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
Copy link

openjdk bot commented Nov 26, 2025

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

@Ziad-Mid Ziad-Mid marked this pull request as ready for review November 26, 2025 16:37
@openjdk openjdk bot added the rfr Ready for review label Nov 26, 2025
@mlbridge
Copy link

mlbridge bot commented Nov 26, 2025

Webrevs

@andy-goryachev-oracle
Copy link
Contributor

andy-goryachev-oracle commented Nov 26, 2025

The fix looks good in the reproducer, good job!

I have less luck in the monkey tester where we hit other issues (JDK-8318095 comes to mind).
I want to test this change more, and perhaps ask @nlisker to take a look, since I am less exposed to RTL in my environment.

@andy-goryachev-oracle
Copy link
Contributor

andy-goryachev-oracle commented Nov 26, 2025

@Ziad-Mid do you think this fix is independent of (JDK-8318095 , in other words, would fixing (JDK-8318095 will require undoing changes in this PR?

@andy-goryachev-oracle
Copy link
Contributor

@Maran23
Copy link
Member

Maran23 commented Nov 27, 2025

Can this be tested with a JUnit Test? I'm asking as there are quite some problems right now with complex text, fonts and RTL. So every regression test will help in the future

@Ziad-Mid
Copy link
Contributor Author

Ziad-Mid commented Dec 1, 2025

@Ziad-Mid do you think this fix is independent of (JDK-8318095

I am checking the JDK-8318095 with more tests, but a first thought they look independant to me. I will check both and let you know.

Can this be tested with a JUnit Test?

I am working on a JUnit Test for this bug as suggested .

@Ziad-Mid
Copy link
Contributor Author

Ziad-Mid commented Dec 3, 2025

@andy-goryachev-oracle I have provided a fix for JDK-8318095 in PR 1995.
It doesn't impact the changes in this PR as mentioned, you can review it and let me know your suggestions for both PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rfr Ready for review

Development

Successfully merging this pull request may close these issues.

3 participants