8218745: TableView: visual glitch at borders on horizontal scrolling

[Maran23]

This PR fixes the border glitch/gap as explained in both linked tickets.

I noted that the ScrollPane control does not suffer from this problem, although the code is mostly the same as in VirtualFlow. The ScrollPane snaps the content correctly, no matter which scale. I carefully checked the code and it seems that the container structure in ScrollPane was explicitly written to handle this perfectly. There was definitely some thought on that.

So to finally fix this issue, I rewrote the VirtualFlow container/scrolling code to be exactly the same as in ScrollPane(Skin).
And this also fixes the issue, while behaving the same as before.

In the future it may makes sense to try to somewhat unify the code from ScrollPane and VirtualFlow. I already have some ideas.

Unfortunately though, one more container is introduced to VirtualFlow, so the css needs to be changed since it is very strictly written in the first place:
Before: .list-view:focused > .virtual-flow > .clipped-container > .sheet > .list-cell
After: .list-view:focused > .virtual-flow > .viewport > .clipped-container > .sheet > .list-cell

To better understand this, the container structure (tree) is shown below:

• ScrollPane
  • viewRect -> setClip -> clipRect (viewContent size)
    • viewContent -> setLayoutX
      • Content
  • vsb
  • hsb
  • corner

---

• VirtualFlow
  • viewRect (->NEW IN THIS PR<-) -> setClip -> clipRect (clippedContainer size)
    • clippedContainer/clipView -> setLayoutX (when scrolling)
      • sheet
        • Cell
  • vsb
  • hsb
  • corner

/issue add JDK-8146406

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires a CSR request matching fixVersion jfx23 to be approved (needs to be created)
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

• JDK-8218745: TableView: visual glitch at borders on horizontal scrolling (Bug - P4)
• JDK-8146406: Unexplainable gap between TableCell inside a TableView (Bug - P4)

Reviewers

• Andy Goryachev (@andy-goryachev-oracle - Reviewer) 🔄 Re-review required (review applies to c3594907)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1330

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1330.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]

@Maran23
Adding additional issue to issue list: 8146406: Unexplainable gap between TableCell inside a TableView.

[mlbridge]

Webrevs

• 01: Full (3072fbe3)
• 00: Full (c3594907)

[kevinrushforth]

This will need careful review and testing.

/reviewers 2

[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 1 Reviewer, 1 Author).

[kevinrushforth]

@johanvos : I presume you will want to review this?

[andy-goryachev-oracle]

This might require a CSR as it might break custom stylesheets that modify list/tree/table/views.

[Maran23]

Probably. Was not sure since this is a very specific case which we never had before (as far as I can tell).

[kevinrushforth]

Yes, I think a CSR would be in order. Thanks for bringing this up @andy-goryachev-oracle

[andy-goryachev-oracle]

The scrolling works correctly with the fix (tested TableView on macOS (scale=1 and 2) and win11 (scale=200%, 225%). I'll continue testing tomorrow.

It might be a dumb question: would it be possible to avoid creating an intermediate container and keep the existing .css files?

[Maran23]

It might be a dumb question: would it be possible to avoid creating an intermediate container and keep the existing .css files?

Not a dumb question, that is what I tried at first. See also my old PR: #630. I tried multiple things with the container and snapping, but never managed to fix all the issues with all scales.
So I didn't found a way to fix this issue other than having the same container structure as ScrollPane, which works very well and looks like it was structured like that on purpose.
And ScrollPane works with whatever you put into it -> reliable, so I think it is not a bad thing (other than the css changed 😐)

[andy-goryachev-oracle]

looks good, tested list/tree/table views on macOS 14.1.2 in light mode

[kevinrushforth]

/csr needed

[openjdk]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@Maran23 please create a CSR request, with the correct fix version, for at least one of the issues associated with this pull request. This pull request cannot be integrated until all the CSR request are approved.

[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!

[bridgekeeper]

@Maran23 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[Maran23]

/open

[openjdk]

@Maran23 This pull request is now open

[Maran23]

Note that I found out something interesting which can MAY help us here to solve the problem in another way.

What I found out:
If I set the the opacity of the clip rect to something under 1, this bug disappears.

So something in the low level clip rendering is different when an opacity is set. Will investigate if there could be another solution.

[Maran23]

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.

See here NGNode:

renderRectClip seems to be doing something wrong.

I see that the clip rect is floored there, which seems to be the cause. Rounding it is fixing this for me.
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)

Note that the minX is floored down, that explains the 1px gap.
I could track it down to be a typical floating point problem:

In AffineBase, line 860. He is calculating: 79,2 * 1,25 (render scale). Should the value be snapped here as well somehow?
Another idea is to use doubles instead of float. We lose information when we convert our JavaFX layout double values to float. A quick test showed that this seems to fix the floating point errors.

Another thing I noticed is that the rect calculation seems to be ... weird in renderRectClip.

And now I fully understand why this only happens for the VirtualFlow:
This is the only container and location where the clip is layouted as well! (setLayoutX/setLayoutY).
In ScrollPaneSkin, there was the decision, that the clip is set and the content is layouted only, not the clip.

It may still make sense to me to actually improve the VirtualFlow here - it seems like a good choice to just set the clip and never touch it again and only change the underlying content layout, might be even a bit faster.

But there is obviously a bug on the low level NGNode side as well. Not sure where to go from here.

[openjdk]

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

[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!

[Maran23]

open for discussion still, but there will be a counter PR from me at one point.

[andy-goryachev-oracle]

But there is obviously a bug on the low level NGNode side as well. Not sure where to go from here.

does it mean this PR is not ready, or do we have other issue(s)?

Also, a CSR is needed for this PR to go forward.

[openjdk]

@Maran23 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8218745-snapping-x-y-tableview-scroll-3
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[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!