Skip to content
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.

Sign up for GitHub

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

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
076590d
Fixed TextArea multiLine prompt support and modified tests
Ziad-Mid Feb 11, 2025
c40c4e7
Merge branch 'openjdk:master' into 8335547.multiLinePrompt
Ziad-Mid Feb 19, 2025
6a7c468
changes to unbind logic only when needed
Ziad-Mid Feb 20, 2025
340e9d6
Merge branch 'master' into 8335547.multiLinePrompt
Ziad-Mid Mar 13, 2025
28e7428
Added logic to support multiline prompt text in the Skin and changed …
Ziad-Mid Mar 13, 2025
358ac0a
Fixed formatting issues
Ziad-Mid Mar 14, 2025
68b0c86
changed wildcard imports
Ziad-Mid Mar 21, 2025
549c199
Minor changes
Ziad-Mid Mar 24, 2025
4364489
Removed unused imports and code
Ziad-Mid Mar 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
View file
Original file line number Diff line number Diff line change
@@ -734,13 +734,7 @@ private void createPromptNode() {
promptNode.fontProperty().bind(getSkinnable().fontProperty());

promptNode.textProperty().bind(getSkinnable().promptTextProperty());
promptNode.textProperty().bind(Bindings.createStringBinding(() -> {
String s = getSkinnable().getPromptText();
if (s == null) {
return "";
}
return s.replace("\n", "");
}, getSkinnable().promptTextProperty()));
promptNode.textProperty().bind(getSkinnable().promptTextProperty().map(s -> s.replace("\n", "")));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to have arrived at these two options:

  1. acknowledge and defend the status quo in which the text layout considers only '\n' as newlines
  2. 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option 1 is intentionally the status quo, and matches what Swing's JComponent does, although @mstr2 is right that this isn't documented. An RFE to treat \r or \r\n as a newline could be considered in the future. We wouldn't do that as part of this PR.

So for this PR, the question is what characters should be elided for the prompt text of a TextField so that multiple lines as a single line? Limiting this to stripping \n is sufficient given the current implementation, unless and until something else changes. Also, it matches what the existing implementation tries to do when it modifies the actual property value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I am curious about: I don't see a similar stripping of newlines in the text itself for TextField, and yet it does render the whole string as if the newline had been stripped. Do you know why we need to strip it from promptTextProperty explicitly and not from textProperty?

promptNode.fillProperty().bind(promptTextFillProperty());
updateSelection();
}