RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
Andy Goryachev
angorya at openjdk.org
Fri Feb 9 18:03:07 UTC 2024
On Fri, 9 Feb 2024 12:54:17 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:
>> There are a number of tickets open related to text rendering:
>>
>> https://bugs.openjdk.org/browse/JDK-8314215
>>
>> https://bugs.openjdk.org/browse/JDK-8145496
>>
>> https://bugs.openjdk.org/browse/JDK-8129014
>>
>> They have in common that wrapped text is taking the trailing spaces on each wrapped line into account when calculating where to wrap. This looks okay for text that is left aligned (as the spaces will be trailing the lines and generally aren't a problem, but looks weird with CENTER and RIGHT alignments. Even with LEFT alignment there are artifacts of this behavior, where a line like `AAA BBB CCC` (note the **double** spaces) gets split up into `AAA `, `BBB ` and `CCC`, but if space reduces further, it will wrap **too** early because the space is taken into account (ie. `AAA` may still have fit just fine, but `AAA ` doesn't, so the engine wraps it to `AA` + `A ` or something).
>>
>> The fix for this is two fold; first the individual lines of text should not include any trailing spaces into their widths; second, the code that is taking the trailing space into account when wrapping should ignore all trailing spaces (currently it is ignoring all but one trailing space). With these two fixes, the layout in LEFT/CENTER/RIGHT alignments all look great, and there is no more early wrapping due to a space being taking into account while the actual text still would have fit (this is annoying in tight layouts, where a line can be wrapped early even though it looks like it would have fit).
>>
>> If it were that simple, we'd be done, but there may be another issue here that needs solving: wrapped aligned TextArea's.
>>
>> TextArea don't directly support text alignment (via a setTextAlignment method like Label) but you can change it via CSS.
>>
>> For Left alignment + wrapping, TextArea will ignore any spaces typed before a line that was wrapped. In other words, you can type spaces as much as you want, and they won't show up and the cursor won't move. The spaces are all getting appended to the previous line. When you cursor through these spaces, the cursor can be rendered out of the control's bounds. To illustrate, if you have the text `AAA BBB CCC`, and the text gets wrapped to `AAA`, `BBB`, `CCC`, typing spaces before `BBB` will not show up. If you cursor back, the cursor may be outside the control bounds because so many spaces are trailing `AAA`.
>>
>> The above behavior has NOT changed, is pretty standard for wrapped text controls,...
>
> John Hendrikx has updated the pull request incrementally with two additional commits since the last revision:
>
> - Add some clarifying documentation
> - Do not collapse trailing spaces of last line (where no soft wrap occurs)
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextSpan.java line 36:
> 34: * A text span can contain line breaks if the text should span multiple
> 35: * lines.
> 36: */
thank you for adding this explanation!
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 1245:
> 1243: * no matter where it occurs).
> 1244: */
> 1245:
@karthikpandelu could you please take a look at this comment here and this PR in general (since you both are modifying the same file)?
Is there anything to add?
tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java line 2:
> 1: /*
> 2: * Copyright (c) 2012, 2023, Oracle and/or its affiliates. All rights reserved.
weird, github shows this file as new, but it is not.
2024
tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java line 58:
> 56: private final PrismTextLayout layout = new PrismTextLayout();
> 57: private final PGFont font = (PGFont) FontHelper.getNativeFont(Font.font("Monaco", 12));
> 58: private final PGFont font2 = (PGFont) FontHelper.getNativeFont(Font.font("Tahoma", 12));
are these fonts available on all platforms?
what would happen if the font with this name is not found?
tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java line 60:
> 58: private final PGFont font2 = (PGFont) FontHelper.getNativeFont(Font.font("Tahoma", 12));
> 59:
> 60: class TestSpan implements TextSpan {
could this class be static?
tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java line 62:
> 60: class TestSpan implements TextSpan {
> 61: String text;
> 62: Object font;
final text and font?
tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java line 83:
> 81: int i = 0;
> 82: while (i < content.length) {
> 83: spans[i>>1] = new TestSpan(content[i++], content[i++]);
I would seriously recommend adhering to one statement per line here:
Object text = content[i++];
Object font = content[i++];
spans[i>>1] = new TestSpan(text, font);
tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java line 338:
> 336: HARD_WRAP(new Parameters(
> 337: "The quick brown fox jumps\nover the lazy dog",
> 338: Font.font("Monaco", 12),
same comment about font not found on all the platforms
tests/system/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java line 414:
> 412: 12.0f, 4.001953f
> 413: ));
> 414:
should we add tests for leading/trailing tabs and milti-line strings, and multi-line with leading/trailing spaces?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484613629
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484616479
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484619240
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484620756
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484621432
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484622292
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484628361
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484630360
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484632163
More information about the openjfx-dev
mailing list