8335547: Support multi-line prompt text for TextArea #1716

Ziad-Mid · 2025-02-19T16:50:02Z

Added multi line prompt support for TextArea this will provide the ability to have multiple lines in textArea as expected,
Also fixed tests to meet the new changes

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)
Change requires CSR request JDK-8352564 to be approved

Issues

JDK-8335547: Support multi-line prompt text for TextArea (Enhancement - P4)
JDK-8352564: Support multi-line prompt text for TextArea (CSR)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1716

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-19T16:50:16Z

👋 Welcome back Ziad-Mid! 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 · 2025-02-19T16:51:11Z

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

andy-goryachev-oracle · 2025-02-20T00:17:24Z

modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java

-                if (isBound()) {
-                    unbind();
-                }
+            if (isBound()) {


I think we might want to bypass the whole thing if it is a TextArea.

Yes I did change the code a bit and now the tests do not fail

andy-goryachev-oracle · 2025-02-20T00:21:20Z

modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java

+            if (isBound()) {
+                unbind();
+            }
+            if (TextInputControl.this instanceof TextArea) {


Is this the right approach? One can always ask whether we should rely on the instanceof TextArea here, for example in the context of a custom TextInputControl.

Also think of:

Can a custom control override this behavior?

Do we want to give this capability to the application?

Should we create a public API instead? And if so, which kind?

The purpose of this is to remove the stripping of new lines in TextArea only as that is normally how it should be,
but the default behavior will stay as it was before for other classes that extends TextInputControl.
And to give the capability to applications to modify this behavior is not necessary.

andy-goryachev-oracle · 2025-02-20T00:23:48Z

Also notice the github actions (GHA) are failing, meaning this PR breaks some tests

TextAreaTest > checkPromptTextPropertyBind() FAILED
TextFieldTest > checkPromptTextPropertyBind() FAILED

Ziad-Mid · 2025-02-20T14:02:30Z

Also notice the github actions (GHA) are failing, meaning this PR breaks some tests
TextAreaTest > checkPromptTextPropertyBind() FAILED
TextFieldTest > checkPromptTextPropertyBind() FAILED

I have fixed the code for this , but there is still another fail

andy-goryachev-oracle · 2025-02-20T15:47:35Z

ServiceLifecycleTest > cancelCalledFromOnFailed() FAILED
this failure happens intermittently, see https://bugs.openjdk.org/browse/JDK-8088343 and is unrelated to the issue.

also, you can try re-running failed tests (look for a button in the top right corner).

andy-goryachev-oracle · 2025-02-20T15:56:31Z

What do you think of an alternative solution: instead of trying to fix the property value, we could leave it alone but change the way it's interpreted by the skin.

Look at TextFieldSkin::createPromptNode method. We could bind the Text.text property to the StringBinding created by Bindings::createStringBinding, something along the lines

        promptNode.textProperty().bind(Bindings.createStringBinding(() -> {
            String s = getSkinnable().getPromptText();
            // TODO replace newlines here
            return s;
        }, getSkinnable().promptTextProperty());

This will remove all the logic from the control and place it where it needs to happen (TextFieldSkin). The TextAreaSkin is unchanged and will display newlines when all the cruft is removed from TextInputControl::promptText.

(We could also change the said property to be lazily initialized since in most cases it's not used).

What do you think?

…tests

andy-goryachev-oracle

The idea is solid - you could make it ready for review (RFR) now.

I've left a number of general formatting suggestions, but the main concern is the decision to convert the tests to be specific to the TextField and the TextArea (omitting the PasswordField). Since the only difference is the expected values, maybe we can keep the existing tests and introduce a function that computes the expected value based on the (parameter) type.

andy-goryachev-oracle · 2025-03-13T17:36:35Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java

+                s = "";
+            s = s.replace("\n", "");
+            return s;
+        }, getSkinnable().promptTextProperty()));


two minor problems:

missing { } after if. (I would highly recommend always using { })

when s is null you call replace() unnecessarily

suggestion:

String s = getSkinnable().getPromptText(); if (s == null) { return ""; } return s.replace("\n", "");

You should consider using a fluent binding here, which is a more modern solution compared to the Bindings class. It is also simpler because you don't need to check for null:

promptNode.textProperty().bind(getSkinnable().promptTextProperty().map(s -> s.replace("\n", "")));

Have you considered the suggestion?

Yes I will push the new changes thanks for the suggestion @mstr2

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextAreaTest.java

andy-goryachev-oracle · 2025-03-13T17:41:52Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextAreaTest.java

+        txtArea.promptTextProperty().bind(promptProperty);
+        root.getChildren().add(txtArea);
+        Text promptNode = TextInputSkinShim.getPromptNode(txtArea);
+        assertEquals("Prompt\nwith\nLineBreaks", promptNode.getText());


here you can use promptWithLineBreaks string declared earlier
(also applies to other tests)

andy-goryachev-oracle · 2025-03-13T17:42:52Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextAreaTest.java

+        txtArea.setPromptText(promptWithLineBreaks);
+        root.getChildren().add(txtArea);
+        Text promptNode = TextInputSkinShim.getPromptNode(txtArea);
+        System.out.println(promptNode.getText());


no need to print to stdout. please remove all System.out.println()'s

andy-goryachev-oracle · 2025-03-13T17:46:40Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextFieldTest.java

+        assertEquals("Prompt without LineBreaks", promptNode.getText());
+        txtField.setPromptText(promptWithLineBreaks);
+        assertEquals("PromptwithLineBreaks", promptNode.getText());
+


please remove extra newline

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextAreaTest.java

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java

andy-goryachev-oracle · 2025-03-14T16:28:27Z

you can make this PR ready for review (RFR) now.

mlbridge · 2025-03-15T05:01:16Z

Webrevs

mstr2 · 2025-03-16T09:15:27Z

What happens if the text contains line breaks other than \n, such as \r or \r\n? There's System.lineSeparator(), but we might want to remove all kinds of line separators regardless.

andy-goryachev-oracle · 2025-03-17T14:58:16Z

What happens if the text contains line breaks other than \n

This is a good question.
I guess it depends on what other characters cause line break in JavaFX.
Wikipedia (https://en.wikipedia.org/wiki/Newline) specifies a few:

1. vert tab \u000b ---
2. form feed \u000c ---
3. cr \u000d ---
4. crlf \u000d\u000a ---
5. lf \u000a ---
6. nel \u0085 ---
7. ls \u2028 ---
8. ps \u2029 ---

@Ziad-Mid could you investigate please?

Ziad-Mid · 2025-03-17T16:23:11Z

cr \u000d ---

Yes , this approach do not handle all the line Seperators only "\n" , I will change it to handle it all

andy-goryachev-oracle · 2025-03-17T16:24:56Z

@Ziad-Mid please make sure we need to change - does the code (with no filtering) render other newline characters?

Ziad-Mid · 2025-03-18T15:41:00Z

I did test the mentioned line separators without filtering and the line separators are not rendered as newlines, so there is no need to add filtering for other line separators as they are already handled by default.

andy-goryachev-oracle

I think it currently looks good, even though PasswordField lost its portion of tests. Since we know the PasswordField implementation does not add any functionality in the area being tested, the TextFieldTest should be sufficient.

Left some minor comments.

I would like to have another pair of eyes though.

/reviewers 2

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextAreaTest.java

openjdk · 2025-03-20T14:45:15Z

@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 1 Reviewer, 1 Author).

mstr2 · 2025-03-21T06:06:42Z

I think that we should treat all usual line separators in the same way (that is, \n, \r, and \r\n), as I don’t know if we have a specification that only one kind of line separator visually breaks lines in JavaFX.

andy-goryachev-oracle · 2025-03-21T18:32:48Z

Confirming that no other character (see earlier comment) other than LF render as newlines, on macOS and Windows.

The ticket mentions a possible "workaround" of using \r (CR) on Windows, but it is not true.

andy-goryachev-oracle

@Ziad-Mid :
please do not resolve the conversations without the original commenter confirming that the conversation is resolved (it's really a bug in guthub, as only the originator of the comment can say if the conversation has been resolved). I think there are still some questions left unanswered, please check.

(I've started to use thumbs-up emoji to mark the comment as addressed, if you want my opinion).

Other than that, the changes look good to me.

Ziad-Mid · 2025-03-22T05:20:26Z

@Ziad-Mid : please do not resolve the conversations without the original commenter confirming that the conversation is resolved (it's really a bug in guthub, as only the originator of the comment can say if the conversation has been resolved). I think there are still some questions left unanswered, please check.

(I've started to use thumbs-up emoji to mark the comment as addressed, if you want my opinion).

Other than that, the changes look good to me.

I did unresolved them, and I will check the suggestions whenever possible and answer,
thanks

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java

mstr2 · 2025-03-26T08:44:28Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java

@@ -733,7 +732,7 @@ private void createPromptNode() {
        promptNode.visibleProperty().bind(usePromptText);
        promptNode.fontProperty().bind(getSkinnable().fontProperty());

-        promptNode.textProperty().bind(getSkinnable().promptTextProperty());
+        promptNode.textProperty().bind(getSkinnable().promptTextProperty().map(s -> s.replace("\n", "")));


I recommend to replace all usual newline characters with replaceAll("[\r\n]", "") as I can't think of a compelling reason to remove only some, but leave others in the string.

The tests show that only LF "\n" is rendered as a new line, there is no need to add more restrictions that is not needed
and the same was tested by @andy-goryachev-oracle previously in the comments and it confirms the same.

That doesn't sound like a compelling reason to me. In fact, it makes it seems like a bug in JavaFX that a line break is only rendered with \n, but not with \r\n or \r.

In any case, the goal here is to (semantically) transform a string such that it doesn't contain line breaks, and line breaks come in three different usual forms. Our goal should always be to do the right thing, and not stop half-way and rely on unspecified rendering quirks for the rest.

We seem to have arrived at these two options:

acknowledge and defend the status quo in which the text layout considers only '\n' as newlines

enhance the text layout to handle other paragraph separators (as well as maybe add support for other symbols that impact layout, such as soft hyphen)

@kevinrushforth what do you think?

Ziad-Mid and others added 2 commits February 11, 2025 15:47

Fixed TextArea multiLine prompt support and modified tests

076590d

Merge branch 'openjdk:master' into 8335547.multiLinePrompt

c40c4e7

andy-goryachev-oracle reviewed Feb 20, 2025

View reviewed changes

changes to unbind logic only when needed

6a7c468

Ziad-Mid closed this Feb 20, 2025

Ziad-Mid reopened this Feb 20, 2025

Ziad-Mid added 2 commits March 13, 2025 12:54

Merge branch 'master' into 8335547.multiLinePrompt

340e9d6

Added logic to support multiline prompt text in the Skin and changed …

28e7428

…tests

andy-goryachev-oracle reviewed Mar 13, 2025

View reviewed changes

Fixed formatting issues

358ac0a

Ziad-Mid marked this pull request as ready for review March 15, 2025 04:58

openjdk bot added the rfr Ready for review label Mar 15, 2025

andy-goryachev-oracle reviewed Mar 20, 2025

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java Outdated Show resolved Hide resolved

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextAreaTest.java Show resolved Hide resolved

openjdk bot added the csr Need approved CSR to integrate pull request label Mar 20, 2025

changed wildcard imports

68b0c86

andy-goryachev-oracle approved these changes Mar 21, 2025

View reviewed changes

Minor changes

549c199

andy-goryachev-oracle reviewed Mar 24, 2025

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java Show resolved Hide resolved

andy-goryachev-oracle reviewed Mar 24, 2025

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java Outdated Show resolved Hide resolved

Removed unused imports and code

4364489

andy-goryachev-oracle approved these changes Mar 25, 2025

View reviewed changes

mstr2 reviewed Mar 26, 2025

View reviewed changes

8335547: Support multi-line prompt text for TextArea #1716

Are you sure you want to change the base?

8335547: Support multi-line prompt text for TextArea #1716

Conversation

Ziad-Mid commented Feb 19, 2025 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Feb 19, 2025

openjdk bot commented Feb 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Feb 20, 2025

Ziad-Mid commented Feb 20, 2025 • edited Loading

andy-goryachev-oracle commented Feb 20, 2025

andy-goryachev-oracle commented Feb 20, 2025

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Mar 14, 2025

mlbridge bot commented Mar 15, 2025 • edited Loading

Webrevs

mstr2 commented Mar 16, 2025

andy-goryachev-oracle commented Mar 17, 2025 • edited Loading

Ziad-Mid commented Mar 17, 2025

andy-goryachev-oracle commented Mar 17, 2025

Ziad-Mid commented Mar 18, 2025 • edited Loading

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 20, 2025

mstr2 commented Mar 21, 2025

andy-goryachev-oracle commented Mar 21, 2025

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Ziad-Mid commented Mar 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ziad-Mid commented Feb 19, 2025 •

edited by openjdk bot

Loading

Ziad-Mid commented Feb 20, 2025 •

edited

Loading

mlbridge bot commented Mar 15, 2025 •

edited

Loading

andy-goryachev-oracle commented Mar 17, 2025 •

edited

Loading

Ziad-Mid commented Mar 18, 2025 •

edited

Loading