Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8218745: TableView: visual glitch at borders on horizontal scrolling #630

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Maran23
Copy link
Member

@Maran23 Maran23 commented Sep 23, 2021

This PR fixes an issue which is probably in JavaFX since VirtualFlow exists.
While horizontal scrolling any VirtualFlow control the left blue border sometimes disappear/gets smaller. (see also image below)

This can be fixed by snapping the scroll bar value (similar like e.g. a ScrollPane does).
The same needs to be done on the table header as otherwise the column lines might be 1 px off to the cell lines.
As a side effect this also fixes that the column lines sometimes get's blurry when horizontal scrolling (see second image).

While testing with -Dglass.win.uiScale I found out that the problem is not fixed for a scale like 1.25 or 1.5, while it is fixed for 1 or 2. The border sometimes disappears only when the snapped value is a decimal number (which obviously does not happen on a scale of 1 or 2), e.g. something like 12.6 but it will never disappear when it's a normal number, so e.g. just 12.

That's why something like Math.round(..) or just a cast to an int instead of snapping fixes this problem for all scales. I also didn't notice any side effect. But not sure if this the right fix then.
How does JavaFX render a node when e.g. the x is a decimal number? And does a decimal number make sense (Why we e.g. don't round the value)?

Another explanation could also be that there is an issue somewhere deep inside the node layout/snapping/Clip/Group/pixel rendering and to simply round/cast the value just fixes the symptom here.

In any case I'm open for any feedback, help or explaination.
I'm also glad for anything which might help identify the root cause here, if any.


image

image


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

Issue

  • JDK-8218745: TableView: visual glitch at borders on horizontal scrolling

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 630

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 23, 2021

👋 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 openjdk bot added the rfr Ready for review label Sep 23, 2021
@mlbridge
Copy link

mlbridge bot commented Sep 23, 2021

Webrevs

@kevinrushforth
Copy link
Member

kevinrushforth commented Sep 24, 2021

/reviewers 2

@openjdk
Copy link

openjdk bot commented Sep 24, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth self-requested a review Sep 29, 2021
@openjdk
Copy link

openjdk bot commented Oct 15, 2021

@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
git fetch https://git.openjdk.java.net/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Oct 15, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Given that this doesn't fix the problem when Hi-DPI scaling is in effect, this will need a different solution. See the discussion in this thread on the openjfx-dev mailing list for some considerations regarding Hi-DPI scaling.

Since this PR isn't ready to be reviewed, I'm moving it to Draft.

double snappedClipX = snapPositionX(clipX);
setLayoutX(-snappedClipX);
clipRect.setLayoutX(snappedClipX);
Copy link
Member

@kevinrushforth kevinrushforth Oct 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely not the right place to snap the coordinates to a pixel boundary. This is usually done as part of layout. I'm also skeptical of the fact that you added it to setClipX but not setClipY.

Copy link
Member Author

@Maran23 Maran23 Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't set clipY because it's not part of the story and clipY is always set to 0 when the VirtualFlow is vertical (which is the default for all VirtualFlowContainer. Only ListView can be non-vertical - This should be another bug and test case). That's why there is no issue with it normally.

I'm not sure if this is the wrong place. E.g. ScrollPaneSkin also listens on the scrollbar valueProperty and sets the snapped scrollbar value with setLayoutX - which is very similar then here. We set the layoutX here as well (for clip and the container).

Finally: While I understand it does not fix every scale, isn't it at least a good first step? (When we agree this is the correct location here)

@kevinrushforth kevinrushforth marked this pull request as draft Oct 15, 2021
@openjdk openjdk bot removed the rfr Ready for review label Oct 15, 2021
…snapping-x-y-tableview-scroll

� Conflicts:
�	modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java
�	modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants