jfx 8301121: RichTextArea Control (Incubator)

Incubating a new feature - rich text control, RichTextArea, intended to bridge the functional gap with Swing and its StyledEditorKit/JEditorPane. The main design goal is to provide a control that is complete enough to be useful out-of-the box, as well as open to extension by the application developers.

This is a complex feature with a large API surface that would be nearly impossible to get right the first time, even after an extensive review. We are, therefore, introducing this in an incubating module, jfx.incubator.richtext. This will allow us to evolve the API in future releases without the strict compatibility constraints that other JavaFX modules have.

Please check out two manual test applications - one for RichTextArea (RichTextAreaDemoApp) and one for the CodeArea (CodeAreaDemoApp). Also, a small example provides a standalone rich text editor, see RichEditorDemoApp.

Because it's an incubating module, please focus on the public APIs rather than implementation. There will be changes to the implementation once/if the module is promoted to the core by popular demand. The goal of the incubator is to let the app developers try the new feature out.

References

Proposal: https://github.com/andy-goryachev-oracle/Test/blob/main/doc/RichTextArea/RichTextArea.md
Discussion points: https://github.com/andy-goryachev-oracle/Test/blob/main/doc/RichTextArea/RichTextAreaDiscussion.md
API specification (javadoc): https://cr.openjdk.org/~angorya/RichTextArea/javadoc
RichTextArea RFE: https://bugs.openjdk.org/browse/JDK-8301121
Behavior doc: https://github.com/andy-goryachev-oracle/jfx/blob/8301121.RichTextArea/doc-files/behavior/RichTextAreaBehavior.md
CSS Reference: https://cr.openjdk.org/~angorya/RichTextArea/javadoc/javafx.graphics/javafx/scene/doc-files/cssref.html
InputMap (v3): https://github.com/andy-goryachev-oracle/Test/blob/main/doc/InputMap/InputMapV3.md
Previous Draft PR: https://github.com/openjdk/jfx/pull/1374

Progress

[x] Change must not contain extraneous whitespace
[ ] Change requires a CSR request matching fixVersion jfx24 to be approved (needs to be created)
[x] Commit message must refer to an issue
[ ] Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Warnings

⚠️ Patch contains a binary file (apps/samples/RichTextAreaDemo/src/com/oracle/demo/richtext/rta/animated.gif) ⚠️ Patch contains a binary file (modules/jfx.incubator.richtext/src/main/docs/jfx/incubator/scene/control/richtext/doc-files/CodeArea.png) ⚠️ Patch contains a binary file (modules/jfx.incubator.richtext/src/main/docs/jfx/incubator/scene/control/richtext/doc-files/RichTextArea.png)

Issue

JDK-8301121: RichTextArea Control (Incubator) (Enhancement - P2)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1524

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

Using diff file

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

Webrev

Link to Webrev Comment

Jul 30 '24 21:07 andy-goryachev-oracle

:wave: 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.

Jul 30 '24 21:07 bridgekeeper[bot]

@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:

8301121: RichTextArea Control (Incubator)
8343646: Public InputMap (Incubator)

Reviewed-by: kcr, lkostyra, arapte, aghaisas, kizune

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 8 new commits pushed to the master branch:

3cfd317df0d11391005f5ce753797fad3f30aead: 8346227: Seal Paint and Material
5b074c4c2b4f539301813322a6589cec28121553: 8342703: CSS transition is not started when initial value was not specified
6ec588c5635964769b354bce37e68d7a6c00985a: 8315873: [GHA] Update checkout and cache action to use v4
76d5e1ae1c396442dce095063cea7bcaff4cd497: 8343398: Add reducedData preference
98916feed6a9ff4da63ff778fdb0336d43ffaca8: 8342233: Regression: TextInputControl selection is backwards in RTL mode
f06b15b6e6c60fba18b6452464e75b89cc7054f9: 8346693: Update copyright header for files modified in 2024
e30d0d54f4269ffe3a9293fc9604f5467c3b5e37: 8345136: Update JDK_DOCS property to point to JDK 23 docs
5279be658350392e080d358ba7bdd1edb7310f07: 8345127: Add --sun-misc-unsafe-memory-access=allow when running tests until JDK-8334137 is fixed

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. 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.

Jul 30 '24 21:07 openjdk[bot]

Webrevs

Jul 30 '24 22:07 mlbridge[bot]

/reviewers 3

Jul 30 '24 22:07 andy-goryachev-oracle

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

Jul 30 '24 22:07 openjdk[bot]

/csr

Jul 31 '24 15:07 andy-goryachev-oracle

@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@andy-goryachev-oracle please create a CSR request for issue JDK-8301121 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

Jul 31 '24 15:07 openjdk[bot]

/reviewers 2 reviewers

Aug 01 '24 22:08 andy-goryachev-oracle

@andy-goryachev-oracle 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 2 Reviewers).

Aug 01 '24 22:08 openjdk[bot]

/reviewers 3

Aug 01 '24 22:08 andy-goryachev-oracle

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

Aug 01 '24 22:08 openjdk[bot]

how do I set 3 reviewers and at least 2 "R"eviewers?

Aug 01 '24 22:08 andy-goryachev-oracle

how do I set 3 reviewers and at least 2 "R"eviewers?

You can't. You can set it to /reviewers 2 reviewers and ask others to also review.

Aug 01 '24 22:08 kevinrushforth

/reviewers 2 reviewers

Aug 01 '24 22:08 andy-goryachev-oracle

@andy-goryachev-oracle 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 2 Reviewers).

Aug 01 '24 22:08 openjdk[bot]

I started writing up some notes for myself, but then thought it might be worth sharing. Here are my meta- suggestions/ reminders for those who want to review this PR. Almost all of this would apply to reviewing any large feature.

Reviewing a large feature such as this presents a bit of a challenge. It might help focus review comments to think of the following two main areas of review:

API and feature set

This includes reviewing the feature set as described in the JEP and any other high-level docs, such as RichTextAreaBehavior.md, the public API and docs along with other public interfaces (css, modena stylesheet entries) or user-visible behavior, and how well the new feature works (ease-of-use, limitations, etc).

Code review of the implementation, including tests and demos

This includes reviewing the implementation from various perspectives:

Correctness, maintainability, efficiency, etc
The API coverage provided by the automated tests and demos
The implementation of the tests and demos
Code style and other similar aspects (best done after the important things have been addressed)

I would ask reviewers to concentrate on the API and feature set for now. After API issues are mostly settled, we can switch our attention to a more detailed code review.

Aug 30 '24 20:08 kevinrushforth

I'm going to be reviewing this in spurts over the next couple of weeks or so. My initial comments will be focused on the jfx.incubator.richtext module, specifically the RichTextArea control, although I will likely bounce around a little. I will defer any comments on the input map (jfx.incubator.input module) until after my review of the richtext module is done.

Overall, I like how the feature set of RichTextArea is shaping up. I feel that it strikes a good balance by providing a basic control that adds missing rich text functionality to JavaFX without trying to be a full-featured rich text editor that a word processor or other rich editing app could use. It should be able to provide the building blocks for a more complex control to be implemented on top of this.

Except where a name is confusing, I'll save comments about names of methods, fields, parameters, and classes for a later pass.

Aug 30 '24 20:08 kevinrushforth

I was looking into what "editable" does in RichTextArea, and I think we need a new name for one of the methods and might also need a second convenience method. There are at least three possible states that we need to consider:

A. The model supports editing; the control's editable property is true

This means that the document can be modified via the UI; it can be modified by calling control methods from the app (or by calling the editing methods on StyledTextModel, but apps don't typically do that).

B. The model supports editing, the control's editable property is false

This means that the document cannot be modified via the UI; it can be modified by calling control methods from the app (or by calling the editing methods on StyledTextModel, but apps don't typically do that).

C. The model does not support editing; it is read-only as far as the control and the StyledTextModel base class are concerned; the control's editable property is not relevant

This means that the document cannot be modified via the UI; it cannot be modified by calling control methods from the app (nor by calling the editing methods on StyledTextModel, but apps don't typically do that).

There are a couple of thoughts I have around this.

First, RichTextArea::canEdit is not sufficient to distinguish these three cases; it will return false for both B and C. There are methods in RTA that say "if canEdit is false, then this method does nothing". That's correct for case C, but some of those methods -- namely the ones not tied to a UI action (e.g., appendText, insertText, replaceText, applyStyle, setStyle, clear) -- will intentionally do something in case B. Consider adding a second convenience method that returns model != null && model.isUserEditable or similar, and use that new method to qualify whether the non-UI mutator methods of RTA do anything.

Second, StyledTextModel::userEditable doesn't seem like the right name for the method in the model to convey that it is effectively read-only as far as the control is concerned. Perhaps "writable" would be a better name, since from the point-of-view of the caller of the StyledTextModel, that's what it means. Maybe there is an even better name, but having "user" in the name is misleading (and editable by itself would be too confusing, since that's the name we want to keep for the control, and it doesn't have the same meaning). Or you could call it "readOnly", but then that would flip the sense of the boolean.

Aug 30 '24 20:08 kevinrushforth

Thank you @kevinrushforth . I just wanted to emphasize that unlike any other feature that goes into mainline, this module is an incubating module, with the primary goal of getting the new APIn the hands of the app developers, in order to receive their feedback.

This makes the acceptance criteria a bit less stringent, as the incubator may incubate over several releases (or be dropped altogether).

I would really like to target jfx24, even if the code and API has imperfections. And, as always, your feedback and suggestions are greatly appreciated!

Sep 03 '24 15:09 andy-goryachev-oracle

"editable" does in RichTextArea

I see your point. Let me see how we can make this clearer (I also welcome naming suggestions).

Sep 03 '24 18:09 andy-goryachev-oracle

follow-up:

RTA::canEdit() will be removed to avoid confusion.
will change to StyledTextModel::writable

Sep 04 '24 20:09 andy-goryachev-oracle

I wrote a very simple test, something you might see in a "HelloRichTextArea", and it seems quite easy to get started. It did prompt a couple quick suggestions:

RichTextArea:

Do you think it is worth adding a convenience method appendText(String) that calls appendText(String, StyleAttributeMap.EMPTY)?

StyleAttributeMap:

fromInlineStyle(String style)

This method seems redundant: fromInlineStyle(style) and fromStyles(style) are equivalent. Do we need both?

Sep 06 '24 22:09 kevinrushforth

This method seems redundant: fromInlineStyle(style) and fromStyles(style) are equivalent. Do we need both?

These are not the same.

fromInlineStyle(String) accepts as single string containing fx-style CSS with one more more styles. Example: "-fx-fill:red; -fx-font-size:200%;"

fromStyles(String style, String ... names) is more generic, allows for setting the inline style as well as CSS stylesheet names.

Sep 09 '24 16:09 andy-goryachev-oracle

It looks like you copied, rather than moved, RichTextAreaDemo from tests/manual to apps/samples. The former should be deleted.

Sep 10 '24 18:09 kevinrushforth

It looks like you copied, rather than moved

yep, thanks for noticing!

Sep 10 '24 20:09 andy-goryachev-oracle

This method seems redundant: fromInlineStyle(style) and fromStyles(style) are equivalent. Do we need both?

These are not the same.

fromInlineStyle(String) accepts as single string containing fx-style CSS with one more more styles. Example: "-fx-fill:red; -fx-font-size:200%;"

fromStyles(String style, String ... names) is more generic, allows for setting the inline style as well as CSS stylesheet names.

My point was that fromStyles with an empty list of names should do exactly the same thing as fromInlineStyle. Looking at the code that does appear to be the case (and if not, I would wonder why not). So, do we really need fromInlineStyle?

Sep 13 '24 20:09 kevinrushforth

So, do we really need fromInlineStyle?

Now I understand the question!

A bit of history: I had a single method before that accepted (String style, String ... names). Turns out it was a trap: it's easy to forget about it and simply supply a list of style names, which results in the first style name being interpreted as an inline style (and cause an error message). Since I fell into this trap myself, I thought it would be better to have two methods with descriptive names.

Is there a better solution?

Sep 13 '24 21:09 andy-goryachev-oracle

I had a single method before that accepted (String style, String ... names). Turns out it was a trap: it's easy to forget about it and simply supply a list of style names, which results in the first style name being interpreted as an inline style (and cause an error message).

Yeah, that's always a potential problem with a method like this where there is a single argument before the varargs list of the same type as the varargs list.

Since I fell into this trap myself, I thought it would be better to have two methods with descriptive names.

Is there a better solution?

I can't immediately think of one. At least you might add a note to the fromInlineStyle docs that it is equivalent to fromStyles(style).

Sep 13 '24 21:09 kevinrushforth

fromInlineStyle docs that it is equivalent to fromStyles(style).

good point, thanks!

edit: actually, these are hidden behind RichParagraph.Builder methods, so they can be made package-protected. Also, StyleAttributeMap.fromTextNode() can be hidden since it's an impl. detail.

Sep 13 '24 21:09 andy-goryachev-oracle

The following directories should be refactored / renamed from rich to richtext (the one in doc-files is causing the images to not show up in the generated docs)

modules/jfx.incubator.richtext/
  build/classes/java/test/test/com/sun/jfx/incubator/scene/control/rich/
  src/main/docs/jfx/incubator/scene/control/rich/
  src/test/java/test/com/sun/jfx/incubator/scene/control/rich/

Sep 13 '24 22:09 kevinrushforth

jfx jfx copied to clipboard

8301121: RichTextArea Control (Incubator)

Progress

Warnings

Issue

Reviewing

Webrev

Webrevs

jfx
jfx copied to clipboard