RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v7]

[John Hendrikx]

On Tue, 30 Jan 2024 00:12:52 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 one additional commit since the last revision:
> 
>   Fix bug which confused char index with glyph index

I think that it may be wise to do some clean-up of the code surrounding `TextRun` in some future PR.  It's a mish-mash of things currently:

- TextRun seems to be used in two ways.  
    - The "normal" way where multiple runs are constructed from a character array, and it retains references to the start/end in that array (although not a reference to the array itself).  Because it has no reference to the chars from which it was created, a lot of functionality which could be encapsulated is externalized, and many internals of `TextRun` need to be exposed to feed those external functions.
    - For javafx-web, which just wants to render a bunch of glyphs, unrelated to any text (so there is no character array it is based on, and its start/end values are bogus)
- There is a lot of code that pretends there is a difference between a `GlyphList` (clearly intended for rendering pure glyphs only) and `TextRun`, but there is only one implementation of `GlyphList` (`TextRun`).  The code that does the actual rendering in `NGText` bluntly always casts a `GlyphList` to a `TextRun`.  It does this for the sole purpose of finding out the start character position of the original characters the `TextRun` was created from (needed for selection rendering), yet, for `TextRun`s created by javafx-web this is irrelevant (it just always returns `0` for the start character position).
    - It would have been better to do a conditional cast to `TextRun` in `NGText` so that javafx-web can use a much simpler implementation of `GlyphList` not based on `TextRun`.
- In a lot of places in this code and surrounding code, fields have been failed to be marked `private` or `final` even though they can be; this may have implications for what JIT can do.
- There are unused fields (`isCanonical` always returns `false`, `TextLine` is only stored to get its `RectBounds`, could just store that directly, it's `final`)

A clean-up would entail:
- Include a reference to the characters it was created from in `TextRun`
    - Encapsulate a lot of code into `TextRun` (moved from `PrismTextLayout`) that does calculations that needed both internal information of `TextRun` (that no longer needs to be exposed) and the original characters.
- Have javafx-web just implement `GlyphList` and change `NGText` to work with `GlyphList`.  This class would be much smaller (`TextRun` has over a dozen fields).
- In `NGText` do an `instanceof` check to see if it is a `TextRun`, in which case its start offset can be used, otherwise assume it is `0`.
- Mark everything final / private that can be
- Remove any unused fields or fields/parameters that always have the same value (isCanonical, perhaps more)

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1915835190