8218745: TableView: visual glitch at borders on horizontal scrolling

[Maran23]

Alternative PR to #1330 which does not modify the layout of VirtualFlow.

This PR fixes the glitching by removing the code in NGNode.renderRectClip, which made many calculations leading to floating point errors.
Interestingly I found out, that getClippedBounds(..) is already returning the correct bounds that just need to be intersected with the clip of the Graphics object.

So the following code is effectively doing the same:

Old:

BaseBounds newClip = clipNode.getShape().getBounds();
if (!clipNode.getTransform().isIdentity()) {
    newClip = clipNode.getTransform().transform(newClip, newClip);
}
final BaseTransform curXform = g.getTransformNoClone();
final Rectangle curClip = g.getClipRectNoClone();
newClip = curXform.transform(newClip, newClip); // <- The value of newClip after the transform is what getClippedBounds(..) is returning
newClip.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));
Rectangle clipRect = new Rectangle(newClip)

New:

BaseTransform curXform = g.getTransformNoClone();
BaseBounds clipBounds = getClippedBounds(new RectBounds(), curXform);
Rectangle clipRect = new Rectangle(clipBounds);
clipRect.intersectWith(PrEffectHelper.getGraphicsClipNoClone(g));

As you can see, there are very similar, but getClippedBounds does a much better job in calculating the bounds.
I also wrote a tests proving the bug. I took 100% of the setup and values from a debugging session I did when reproducing this bug.

I checked several scenarios and code and could not find any regressions.
Still, since this is change affects all nodes with rectangular clips, we should be careful.
Performance wise I could not spot any difference, I do not expect any difference.
So I would like to have at least 2 reviewers.
Note that I will do more testing as well soon on all JavaFX applications I have access to.

---

As written in the other PR, I have some interesting findings on this particular problem.

Copy&Paste from the other PR:

Ok, so I found out the following:
When a Rectangle is used as clip without any effect or opacity modification, the rendering goes another (probably faster) route with rendering the clip. That's why setting the opacity to 0.99 fixes the issue - another route will be used for the rendering.
This happens at the low level (NGNode) side of JavaFX.
...
I could track it down to be a typical floating point problem
...
The bug always appears when I scroll and the clip RectBounds are something like:
RectBounds { minX:6.999996, minY:37.0, maxX:289.00003, maxY:194.0} (w:282.00003, h:157.0)
...
while this does not happen when the value is something like:
RectBounds { minX:7.000004, minY:37.0, maxX:289.0, maxY:194.0} (w:282.0, h:157.0

Even more details:

• As briefly explained above, due to floating point arithmetic, we may do not get the correct value, leading to an incorrect calculation where 1 pixel is missing. That is why this issue happens only on display scales other than integer values.
• And since only the ClippedContainer changes its layoutX AND the layoutX of its clip, the bug only appears there
• JavaFX Nodes uses double, while the low level side (NG) uses float mostly, which seems to make things less accurate, although not 100% sure if doubles will avoid this particular problem completely, probably not.
  I'm not sure why this decision was made.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

• JDK-8218745: TableView: visual glitch at borders on horizontal scrolling (Bug - P4)

Reviewers

• Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1462/head:pull/1462
$ git checkout pull/1462

Update a local copy of the PR:
$ git checkout pull/1462
$ git pull https://git.openjdk.org/jfx.git pull/1462/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1462

View PR using the GUI difftool:
$ git pr show -t 1462

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1462.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 01: Full - Incremental (bb27b3fe)
• 00: Full (9d20198f)

[kevinrushforth]

I'm reasonably sure there was a good reason for the code in NGNode doing what it did. This will need very careful review and testing before we would accept it.

/reviewers 2 reviewers

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

[kevinrushforth]

Reviewers: @arapte @andy-goryachev-oracle

@Maran23 wait for either @arapte or me to review the proposed Prism changes in this PR.

[Maran23]

I'm reasonably sure there was a good reason for the code in NGNode doing what it did. This will need very careful review and testing before we would accept it.

100% agree.
Note that the code there is a shortcut for performance reasons. Removing it will also fix the bug since the code below is doing the right thing, but will probably result in a performance impact.

Thats why I checked the other path and had a closer look what it does, since it still needs to the right for whatever clip is used. And there I saw that is does nearly the same thing, but with the getClippedBounds instead.

With that in mind, we need to especially check if the fast path did something completely unexpected what the other 'slow' path did not and we may miss now.

[andy-goryachev-oracle]

I see a problem on Windows 11 at 125% scale: using the tester app in the ticket, the table focus rectangle's right edge is not visible at some width (i.e. appears and disappears when resizing the window width):

When it is visible, there is no jitter described in the ticket. Also, it can be reproduced at 100% scale.

It may or may not be a separate issue.

[Maran23]

I see a problem on Windows 11 at 125% scale: using the tester app in the ticket, the table focus rectangle's right edge is not visible at some width (i.e. appears and disappears when resizing the window width):

It may or may not be a separate issue.

Yeah this is a 'known bug' for me, and has nothing to do with this fix.
Note that I only can reproduce this with 125% scale, not 100%. Windows 10 here.

[andy-goryachev-oracle]

Yeah this is a 'known bug' for me, and has nothing to do with this fix.

I do not see the issue without the fix though (at the same resolution).

[Maran23]

I do not see the issue without the fix though (at the same resolution).

I can, the TableView is the root of the Scene in this case. Happens inside a StackPane as well.

[andy-goryachev-oracle]

you are right: I see the focus rectangle jitter at 175% scale on win 11 (w/o the fix), so it must be a different issue. At this scale, it merely shows a thinner line, perhaps that's why I did not notice it earlier.

[andy-goryachev-oracle]

The code change looks good, and I can see it fixes the issue on windows with fractional scales.

Thank you for writing multiple test cases!

[Maran23]

you are right: I see the focus rectangle jitter at 175% scale on win 11 (w/o the fix), so it must be a different issue. At this scale, it merely shows a thinner line, perhaps that's why I did not notice it earlier.

Yeah exactly, it looks like when the focus rect is on the border of the window, it will sometimes disappear (probably jitter 'out' of the window).
When there is some padding inbetween, the focus rect looks like it is jittering instead. Probably the same root cause.

[bridgekeeper]

@Maran23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!