8351878: RichTextArea: copy/paste issues

[andy-goryachev-oracle]

Fixed several issues found in importing RTF text:

• charset translation (brought back removed code)
• missing font size attribute
• missing strike-through attribute

Also, HTML copy suffered from the following issues:

• incorrect font size
• incorrect handling of boolean character attributes (bold, italic, etc.)

The charset issue was caused by my removal of the character decoder code present in the original JDK RTF parser/reader. Why did I do that?

This PR does not add import of RTL paragraph attribute needed to align RTL text correctly.

---

Progress

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

Issue

• JDK-8351878: RichTextArea: copy/paste issues (Bug - P4)

Reviewers

• Lukasz Kostyra (@lukostyra - Reviewer)
• Ziad El Midaoui (@Ziad-Mid - Author)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1735

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back angorya! 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]

@andy-goryachev-oracle This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8351878: RichTextArea: copy/paste issues

Reviewed-by: lkostyra, zelmidaoui

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 17 new commits pushed to the master branch:

• f31d00d: 8328716: [TestBug] Screen capturing utility for failed tests
• a52e2fa: 8352746: [TestBug] Monkey Tester Application Update 5
• 5d41364: 8353557: Skip some system tests on Linux
• 76282bc: 8353587: Spelling errors and dead code
• 714f17f: 8353668: Rename internal c.s.javafx.text.TextLine class
• 62f94bb: 8353620: Make some systems tests robust for Ubuntu 24.04
• 1a65f4c: 8353617: Remove deprecated TransitionEvent constructor
• 9ab2036: 8353548: [macOS] DragEvent.getScreenY() returns incorrect value in secondary monitor
• c0db2dc: 8352982: gradle TEST_SDK_PATH param doesn't work with relative paths
• ab94b5e: 8351276: Prevent redundant computeValue calls when a chain of mappings becomes observed
• ... and 7 more: https://git.openjdk.org/jfx/compare/91433775e75c9c662774f28de5310eab7da0fe7e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 04: Full - Incremental (6a89c153)
• 03: Full - Incremental (63dbbb3d)
• 02: Full - Incremental (f7834f61)
• 01: Full - Incremental (894ee93e)
• 00: Full (5c4285ec)

[andy-goryachev-oracle]

@Ziad-Mid : please take a look at this PR.

Please refer to https://github.com/openjdk/jfx/blob/master/README-code-reviews.md for general guidance.

The areas I would like you to focus on specifically are:

• pasting Arabic from other RTF sources (outlook, other applications?)
• think of any other possible problems or side effects of the proposed change
• verifying the tests fail in the master branch and passing with the fix
• anything else you might find suboptimal or unclear

[openjdk]

@andy-goryachev-oracle Syntax: /reviewer (credit|remove) [@user | openjdk-user]+. For example:

• /reviewer credit @openjdk-bot
• /reviewer credit duke
• /reviewer credit @user1 @user2

[andy-goryachev-oracle]

Reviewers: @Ziad-Mid @kevinrushforth

[openjdk]

@andy-goryachev-oracle Usage: /reviewers <n> [<role>] where <n> is the number of required reviewers. If role is set, the reviewers need to have that project role. If omitted, role defaults to authors.

[kevinrushforth]

/reviewers 2

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[Ziad-Mid]

The tests run successfully with the fix however there are a few things that do not work as expected I guess :
1- Pasting text from notes app (MacOS) that uses numbered list in some cases shows 0. instead of the item number example:

• in note app do something like this

Test
1. one
2. two
3. three
Test

• Copy and paste it is rendred like this

Test
0. one
0. two
0. three
Test

2- Tested with text written in outlook and pasting it and it do not apply formatting neither for RTL or LTR text
3- another thing the arabic diacritics text is not rendered correctly (this may not be related to this issue specifically) you can try with this text السَّلَامُ عَلَيْكُمْ

[andy-goryachev-oracle]

@Ziad-Mid :
interesting, thank you!

1. The list formatting was not explicitly supported, so in a sense it works as designed (all numbered items are represented by {\listtext 0. }. This will likely be a separate enhancement.

2. RTL/LTR attributes are not yet supported by the demo due to many bugs in RTL.

3. Arabic diacritics rendered with the System font look bad, I agree. Maybe @prrace can weigh on it, looks like changing the font to one of the Arabic ones does improve the rendering, although the result appears not as good compared to Chrome:

Can you try setting different fonts?

السَّلَامُ عَلَيْكُمْ =

\u0627\u0644\u0633\u0651\u064e\u0644\u064e\u0627\u0645\u064f \u0639\u064e\u0644\u064e\u064a\u0652\u0643\u064f\u0645\u0652

[andy-goryachev-oracle]

Weird, even "Noto Sans Arabic" font produces weirdly positioned diacritics in JavaFX, while it's ok in MS Word:

[Ziad-Mid]

2. RTL/LTR attributes are not yet supported by the demo due to many bugs in RTL.

By RTL and LTR I just meant normal text orientation and arabic one , when writing a text in outlook and copying it then pasting it doesn't keep the formatting

[andy-goryachev-oracle]

doesn't keep the formatting

please be more specific - maybe include a screenshot of the outlook message vs. rich text area?

I can see the pasted Arabic text rendered correctly (that is, it looks similar to the text in outlook and going from right to left). The RTL paragraph attribute is not yet supported, so the paragraph consisting of only Arabic text might look left-aligned contrary to expectation.

[Ziad-Mid]

please be more specific - maybe include a screenshot of the outlook message vs. rich text area?

The font and size are not applied

[andy-goryachev-oracle]

The font and size are not applied

Thank you, the screenshot helped!

The problem was in the way HTML text was copied to Outlook (for some reason, Outlook prioritizes HTML over RTF when pasting). The font sizes in HTML were using points (pt.) instead of pixels (px.).

In addition to that, copying to HTML did not process boolean attributes correctly.

Thank you for finding these issues!

[andy-goryachev-oracle]

Created https://bugs.openjdk.org/browse/JDK-8353003 for the diacritic marks issue.

[kevinrushforth]

Reviewers: @lukostyra @Ziad-Mid

[lukostyra]

LGTM - did some testing, both with issue-attached Word file and some standard Polish diacritic test sentences and everything seems to work fine.

Question coming from my own curiosity - when you post a sentence containing diacritics (ex. shortest tester for all diacritics in Polish - "Zażółć gęślą jaźń") you can see that some fonts don't support those special characters (ex. Arial Rounded MT Bold on Windows) so as a fallback they are drawn with a different font that supports them. It's quite normal behavior to do so I've seen often, but do you know what is the process for determining which font to use as a replacement in RTA? Is it something we have to worry about?

[andy-goryachev-oracle]

LGTM
than you!

some fonts don't support those special characters (ex. Arial Rounded MT Bold on Windows) so as a fallback they are drawn with a different font that supports them. It's quite normal behavior to do so I've seen often, but do you know what is the process for determining which font to use as a replacement in RTA?

A good question! Maybe @prrace can explain the process. Looking in the Monkey Tester, it is clear that the font substitution algorithm is unable to pick the right fallback font from Arial Rounded MT Bold on macOS (while Swing simply refuses to render anything):

[andy-goryachev-oracle]

Thank you all for reviewing!
/integrate

[openjdk]

Going to push as commit 1b26b66.
Since your change was applied there have been 17 commits pushed to the master branch:

• f31d00d: 8328716: [TestBug] Screen capturing utility for failed tests
• a52e2fa: 8352746: [TestBug] Monkey Tester Application Update 5
• 5d41364: 8353557: Skip some system tests on Linux
• 76282bc: 8353587: Spelling errors and dead code
• 714f17f: 8353668: Rename internal c.s.javafx.text.TextLine class
• 62f94bb: 8353620: Make some systems tests robust for Ubuntu 24.04
• 1a65f4c: 8353617: Remove deprecated TransitionEvent constructor
• 9ab2036: 8353548: [macOS] DragEvent.getScreenY() returns incorrect value in secondary monitor
• c0db2dc: 8352982: gradle TEST_SDK_PATH param doesn't work with relative paths
• ab94b5e: 8351276: Prevent redundant computeValue calls when a chain of mappings becomes observed
• ... and 7 more: https://git.openjdk.org/jfx/compare/91433775e75c9c662774f28de5310eab7da0fe7e...master

Your commit was automatically rebased without conflicts.

[openjdk]

@andy-goryachev-oracle Pushed as commit 1b26b66.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.