Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 17:33:04 GMT, Andy Goryachev wrote: >> 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/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? I checked my changes along with the changes in this PR. The changes made in [PR#1323](https://github.com/openjdk/jfx/pull/1323) works fine with these changes and proper hit test values are obtained. - PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1485956832
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 12:54:17 GMT, John Hendrikx 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)
I see, so the test in graphics was @Ignore("JDK-8087615")
I'd suggest to move the failing cases into a separate method and @Ignore that.
We still have JDK-8087615 to deal with the failure, but at least we'll have a
good working test with the rest of the scenarios.
What do you think?
-
PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1936738038
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 12:54:17 GMT, John Hendrikx 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) Justification + tabs are an odd combination. You can't adjust the tabs as they're supposed to go to tab stops, and tabs in the middle of a piece of text that is being wrapped will make really odd jumps (even without justification). Also lines with tabs will usually have a hard wrap (a line feed) as you otherwise don't know where a next tab would end up if you leave it up to soft wrapping -- lines with a hard wrap are considered to be the "last" line for justification, and so are simply left aligned. Even when the text is mostly justifiable, a tab will often end up at the "end" of a line, leaving white space there (which is what you want to avoid with justified text). All in all, it's a bad combination... so I think it's doing the "correct" thing currently, for certain definitions of "correct". So... I think for the tab tests, it doesn't make much sense to add tests that do soft wrapping + tabs, so I only added a few hard wrapped cases. - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1936738229
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 22:44:07 GMT, Andy Goryachev wrote:
>> 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)
>
> The test is failing on macOS 14.3:
>
>
> 3a93cdf4f7 HEAD@{0}: checkout: moving from review.rtl_hittest_issue to
> review.wrapped-aligned-text-rendering
>
> TextLayoutTest > caseTest(Case) > [7] SOFT_WRAP_WITH_COMPLEX_TEXT FAILED
> org.opentest4j.AssertionFailedError: left aligned: line 0 for
> Parameters[text=The quick brown लोमड़ी jumps over the lazy कुत्ता,
> font=Font[name=Monaco, family=Monaco, style=Regular, size=12.0],
> wrapWidth=200.0, lineWidths=[189.89648, 122.583984], ascent=12.0,
> descent=4.001953] ==> expected: maxX:189.89648, maxY:4.001953} (w:189.89648, h:16.001953)> but was:
> (w:187.00978, h:16.001953)>
> at
> app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
> at
> app//org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
> at
> app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
> at
> app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1152)
> at
> app//test.com.sun.javafx.text.TextLayoutTest.caseTest(TextLayoutTest.java:468)
>
> TextLayoutTest > caseTest(Case) > [8]
> SOFT_WRAP_WITH_COMPLEX_TEXT_AND_EXTRA_TRAILING_SPACE FAILED
> org.opentest4j.AssertionFailedError: left aligned: line 0 for
> Parameters[text=The quick brown लोमड़ी jumps over the lazy कुत्ता,
> font=Font[name=Monaco, family=Monaco, style=Regular, size=12.0],
> wrapWidth=200.0, lineWidths=[189.89648, 122.583984], ascent=12.0,
> descent=4.001953] ==> expected: maxX:189.89648, maxY:4.001953} (w:189.89648, h:16.001953)> but was:
> (w:187.00978, h:16.001953)>
> at
> app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
> at
> app//org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
> at
> app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
> at
> app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1152)
> at
> app//test.com.sun.javafx.text.TextLayoutTest.caseTest(TextLayoutTest.java:468)
>
> TextLayoutTest > fixedComplexTestsToEnsureNoFurtherRegressions() FAILED
> org.opentest4j.AssertionFailedError: runCount ==> expected: <2> but was:
> <1>
> at
> app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
> at
> app//org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtil...
@andy-goryachev-oracle
> The test is failing on macOS 14.3:
I can't do much here, I don't own a mac, and it does work in the build
environment. Although I can't confirm that those tests actually run there...
My own two tests fail because there is some 3 pixel width difference when
complex text is used. The last test (an original one) fails due to a different
number of runs. This suggests that it is not switching between character sets
when it encounters the complex glyphs, so perhaps the font it is using doesn't
have those glyphs?
These are the reason I can think of why the test fails for you:
1. The test doesn't run on our build environment, and so it was never run on a
mac before
2. There are multiple versions of the Monaco font
3. You don't have an international version of the Monaco font (if there is a
distinction...) -- ie, the complex characters are rendered as "boxes" (that
would be consistent with a width increase, the boxes are likely to be wider,
and with a lower amount of runs)
4. You don't have the Monaco font at all, and it is falling back to something
5. Mac isn't consistent across different OS versions(?)
Perhaps we can add an assumption in this test so it only runs on our build
environment? All that matters (for me) is that no regressions get introduced,
we'd just discover that a bit later then.
-
PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1936732751
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 12:54:17 GMT, John Hendrikx 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)
The test is failing on macOS 14.3:
3a93cdf4f7 HEAD@{0}: checkout: moving from review.rtl_hittest_issue to
review.wrapped-aligned-text-rendering
TextLayoutTest > caseTest(Case) > [7] SOFT_WRAP_WITH_COMPLEX_TEXT FAILED
org.opentest4j.AssertionFailedError: left aligned: line 0 for
Parameters[text=The quick brown लोमड़ी jumps over the lazy कुत्ता,
font=Font[name=Monaco, family=Monaco, style=Regular, size=12.0],
wrapWidth=200.0, lineWidths=[189.89648, 122.583984], ascent=12.0,
descent=4.001953] ==> expected: but was:
at
app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at
app//org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
at
app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
at
app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1152)
at
app//test.com.sun.javafx.text.TextLayoutTest.caseTest(TextLayoutTest.java:468)
TextLayoutTest > caseTest(Case) > [8]
SOFT_WRAP_WITH_COMPLEX_TEXT_AND_EXTRA_TRAILING_SPACE FAILED
org.opentest4j.AssertionFailedError: left aligned: line 0 for
Parameters[text=The quick brown लोमड़ी jumps over the lazy कुत्ता,
font=Font[name=Monaco, family=Monaco, style=Regular, size=12.0],
wrapWidth=200.0, lineWidths=[189.89648, 122.583984], ascent=12.0,
descent=4.001953] ==> expected: but was:
at
app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at
app//org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
at
app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
at
app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1152)
at
app//test.com.sun.javafx.text.TextLayoutTest.caseTest(TextLayoutTest.java:468)
TextLayoutTest > fixedComplexTestsToEnsureNoFurtherRegressions() FAILED
org.opentest4j.AssertionFailedError: runCount ==> expected: <2> but was: <1>
at
app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 21:46:46 GMT, John Hendrikx wrote: >> 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? > > Multi line strings are I think addressed in the `hard wrap` cases (they > contain line breaks). I can add a case for leading space (I only have one > for trailing space), and can add cases for tabs (although there is > uncertainty on how they should be treated). I think we can leave the existing behavior with TABs as is. I would favor adding a test to capture the existing behavior, since you have the context and are the most knowledgeable person to do so right now :-) - PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484836844
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 21:51:15 GMT, John Hendrikx wrote: > I think it is easy enough for a user to sanitize the strings they deliver a > bit, while it would be impossible for the user to do so at the soft wrap > locations. I agree. - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1936653904
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 12:54:17 GMT, John Hendrikx 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) > So we treat trailing spaces before \n as hard wrap, does it mean the same > treatment is applied to the leading space (i.e. leading spaces in the first > line), as seen here with TextAlignment=CENTER > > ``` >aaabbb > ccc eee ff > aaa b c > ``` > >  Yes, white space is only collapsed when a soft wrap is inserted to keep it within `wrappingWidth` -- this is because a user can't predict where such wrapping occurs and would have to strip spaces there themselves (never mind that it wouldn't wrap anymore there then as there is no space). The other cases the user can easily solve themselves by ensuring that the strings they supply are `trim`ed. I think removing leading/trailing spaces (and double spaces) is a different feature (browsers like to do this) but not really suitable as a good default for FX. It could be made optional, but I think it is easy enough for a user to sanitize the strings they deliver a bit, while it would be impossible for the user to do so at the soft wrap locations. - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1936652246
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 17:48:09 GMT, Andy Goryachev wrote: >> 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) > > 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? Multi line strings are I think addressed in the `hard wrap` cases (they contain line breaks). I can add a case for leading space (I only have one for trailing space), and can add cases for tabs (although there is uncertainty on how they should be treated). - PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484834811
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 21:36:03 GMT, John Hendrikx wrote:
>> 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?
>
> They may not be present on all platforms, but that doesn't matter. All that
> matters is that they're present in our 3 build environments (linux, mac,
> windows), and since the build is succeeding, they must be present :)
>
> If they're not present, the test will simply fail.
However... it does mean these fonts need to be present on your local
environment as well if you want to build with test succeeding. I don't know if
we can make that a requirement. I could add JUnit 5 `assume` here, but then it
may magically stop executing the test if these fonts were ever removed from our
build environment.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484833587
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 17:38:16 GMT, Andy Goryachev wrote:
>> 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)
>
> 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?
This is from the original test, I went ahead and converted it to a `record`.
> 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);
This is not my code, it's from the original test.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484829896
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484830495
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 17:36:04 GMT, Andy Goryachev wrote:
>> 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)
>
> 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
It's sort of new, it's moved, it used to reside in the graphics project, but as
it needs real fonts for its tests to work, it had to be a system test.
> 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?
They may not be present on all platforms, but that doesn't matter. All that
matters is that they're present in our 3 build environments (linux, mac,
windows), and since the build is succeeding, they must be present :)
If they're not present, the test will simply fail.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484825164
PR Review Comment: https://git.openjdk.org/jfx/pull/1236#discussion_r1484826625
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 12:54:17 GMT, John Hendrikx 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) So we treat trailing spaces before \n as hard wrap, does it mean the same treatment is applied to the leading space (i.e. leading spaces in the first line), as seen here with TextAlignment=CENTER aaabbb ccc eee ff aaa b c  - PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1936378285
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
On Fri, 9 Feb 2024 12:54:17 GMT, John Hendrikx 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 = c
Re: RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v12]
> 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, and IMHO does not need further attent... 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) - Changes: - all: https://git.openjdk.org/jfx/pull/1236/files - new: https://git.openjdk.org/jfx/pull/1236/files/388696a6..3a93cdf4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1236&range=11 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1236&range=10-11 Stats: 178 lines in 5 files changed: 122 ins; 16 del; 40 mod Patch: https://git.openjdk.org/jfx/pull/1236.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1236/head:pull/1236 PR: https://git.openjdk.org/jfx/pull/1236
