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

[John Hendrikx]

On Thu, 8 Feb 2024 16:06:39 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> John Hendrikx has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 16 additional commits since the last revision:
>> 
>>  - Merge branch 'openjdk:master' into feature/wrapped-aligned-text-rendering
>>  - Fix test comment
>>  - More test cases (for hard wrapping) and another small fix
>>  - Improve tests, fix a bug
>>  - Fix bug which confused char index with glyph index
>>  - Move test that needs QuantumToolkit to system tests
>>  - Clean-up, re-enable and extend ancient TextLayout test
>>  - TextRun getAdvance should return 0 when positions array is null
>>  - Fix justify alignment to take all trailing spaces into account
>>  - Fix trailing space calculation to access positions array directly
>>  - ... and 6 more: https://git.openjdk.org/jfx/compare/69aaa708...388696a6
>
> The Group use in the MT is intentional: we want to see the preferred sizes of the content.  StackPane does not work correctly at all (example: change to StackPane in TextPage:197 and click on 'show characters').
> 
> Also, aren't all coordinates typically snapped?
> 
> But your observation about JUSTIFY are justified (pun intended).  I see horizontal scroll bar flickering on and off when resizing a long text due to the vertical scroll bar appearing.  But I still see the HSB even with the short text as you described.
> 
> Perhaps some variant of your resize algorithm should be used here as well (maybe together with my snapInnerSpace() method).

@andy-goryachev-oracle about tabs

There is a lot of special handling going on around tabs.  Tabs are always separate runs (one run for each tab).  When the logic that creates lines and looks for wrapping points encounters a tab, it will set the width of that run (containing the tab) to be the exact width it needs to reach the next tab stop.

            if (run.isTab()) {
                float tabStop = ((int)(lineWidth / tabAdvance) +1) * tabAdvance;
                run.setWidth(tabStop - lineWidth);
            }

Then it will continue with the code that finds a point to soft wrap. This code will call `run.getWrapIndex` which can return a tab as hit offset, but because a tab is always a run with just the tab character, the logic to "skip" white space is not triggered because the first condition in this while loop is `false` (the run is of length 1, so `offset + 1` will always be less than `runEnd`):

                // Don't take spaces into account at the preferred wrap index:
                while (offset + 1 < runEnd && Character.isWhitespace(chars[offset])) {
                    offset++;
                }

Since we don't really have a specification for how we want to deal with tabs, I think it is best for now to leave this as it works currently (it's the same as it was before, I checked that the old logic handles tabs the same).

If there is a good argument to handle these differently than they are now, we can revisit this, as it would be non-trivial change (we'd need to take multiple runs into account when skipping white space, while the current logic only looks at a single run). I will add a comment about tabs in this code to clarify why tabs (even though they're white space) are not handled.

There is also already this comment in the code that seems to exclude tabs specifically -- it already alludes to the fact that you'd need to take multiple runs into account (which are not "shaped" yet), which highlights that it would be a non-trivial change.

                /* Only keep whitespaces (not tabs) in the current run to avoid
                 * dealing with unshaped runs.
                 */

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

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