On Fri, 9 Feb 2024 12:54:17 GMT, John Hendrikx <jhendr...@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
