8276313: ScrollPane scroll delta incorrectly depends on content height by mstr2 · Pull Request #660 · openjdk/jfx

mstr2 · 2021-11-02T10:49:45Z

This PR fixes an issue where the scroll delta of ScrollPane incorrectly depends on the size of its content.
This leads to extremely slow scrolling when the content is only slightly larger than the ScrollPane.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8276313: ScrollPane scroll delta incorrectly depends on content height

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Ajit Ghaisas (@aghaisas - Reviewer)
Johan Vos (@johanvos - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/660/head:pull/660
$ git checkout pull/660

Update a local copy of the PR:
$ git checkout pull/660
$ git pull https://git.openjdk.java.net/jfx pull/660/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 660

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/660.diff

bridgekeeper · 2021-11-02T10:50:59Z

👋 Welcome back mstrauss! 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.

mlbridge · 2021-11-02T10:55:27Z

Webrevs

kevinrushforth · 2021-11-02T12:53:02Z

/reviewers 2

openjdk · 2021-11-02T12:53:46Z

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

aghaisas · 2021-12-01T09:45:40Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java

                double vPixelValue;
                if (nodeHeight > 0.0) {
-                    vPixelValue = vRange / nodeHeight;
+                    vPixelValue = vRange / (nodeHeight - contentHeight);


This may result in divide by 0.

Good catch! I've also fixed a few similar issues in other places.

aghaisas · 2021-12-01T09:45:54Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java

                double hPixelValue;
                if (nodeWidth > 0.0) {
-                    hPixelValue = hRange / nodeWidth;
+                    hPixelValue = hRange / (nodeWidth - contentWidth);


This may result in divide by 0.

kevinrushforth · 2021-12-02T21:51:59Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ScrollPaneSkin.java

+                double vPixelValue = vRange / (nodeHeight - contentHeight);
+                vPixelValue = Double.isFinite(vPixelValue) ? vPixelValue : 0.0;


I liked the previous logic better and would have just changed the comparison to check (nodeHeight - contentHeight) > 0.0 (possibly saving the adjusted width or height in a local various to avoid doing it twice). As it is, this is replacing an explicit check on the source values with an out of range check on the result, which seems less intuitive. Also, the previous code did a > 0 test and the new code effectively does a != 0 test.

I changed it to check the denominator values instead.

kevinrushforth

Looks good.

openjdk · 2021-12-06T06:10:51Z

@mstr2 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8276313: ScrollPane scroll delta incorrectly depends on content height

Reviewed-by: kcr, aghaisas, jvos

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 24 new commits pushed to the master branch:

d3fbb51: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
aa045c5: 8272118: ListViewSkin et al: must not cancel edit on scrolling
5bd72a7: 8277457: AccessControlException: access denied ("java.net.NetPermission" "getCookieHandler")
3d57213: 8277475: Update JDK_DOCS property to point to JDK 17 docs
b257647: 8276847: JSException: ReferenceError: Can't find variable: IntersectionObserver
fc3792d: 8276206: Rename TextBinding class to better express its purpose
0dbdec4: 8276490: Incorrect path for duplicate x and y values, when path falls outside axis bound
d14be6a: 8274061: Tree-/TableRowSkin: misbehavior on switching skin
423e1be: 8277133: Dragboard contents retrieved all over again during a DND process on WebView
6e6c711: 8160597: IllegalArgumentException when we initiate drag on Image
... and 14 more: https://git.openjdk.java.net/jfx/compare/6c8810634ec63af8116dd978656805b985eec800...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @aghaisas, @johanvos) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

johanvos · 2021-12-06T07:24:04Z

I'll review in the next hours.

johanvos

This looks good to me. This new approach is based on the delta between the total height and the visible height, instead of only the total height.
It might be good to capture this idea somehow in the commit message, but since it is clear from the code, I am totally ok with not changing the commit message.

mstr2 · 2021-12-06T10:24:00Z

/integrate

openjdk · 2021-12-06T10:24:48Z

@mstr2
Your change (at version 6b8985f) is now ready to be sponsored by a Committer.

johanvos · 2021-12-06T10:27:02Z

/sponsor

openjdk · 2021-12-06T10:27:49Z

Going to push as commit 5805bf8.
Since your change was applied there have been 24 commits pushed to the master branch:

d3fbb51: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
aa045c5: 8272118: ListViewSkin et al: must not cancel edit on scrolling
5bd72a7: 8277457: AccessControlException: access denied ("java.net.NetPermission" "getCookieHandler")
3d57213: 8277475: Update JDK_DOCS property to point to JDK 17 docs
b257647: 8276847: JSException: ReferenceError: Can't find variable: IntersectionObserver
fc3792d: 8276206: Rename TextBinding class to better express its purpose
0dbdec4: 8276490: Incorrect path for duplicate x and y values, when path falls outside axis bound
d14be6a: 8274061: Tree-/TableRowSkin: misbehavior on switching skin
423e1be: 8277133: Dragboard contents retrieved all over again during a DND process on WebView
6e6c711: 8160597: IllegalArgumentException when we initiate drag on Image
... and 14 more: https://git.openjdk.java.net/jfx/compare/6c8810634ec63af8116dd978656805b985eec800...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-12-06T10:28:22Z

@johanvos @mstr2 Pushed as commit 5805bf8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mstr2 added 2 commits November 2, 2021 11:27

failing test

e68e091

Fixed scrolling speed for ScrollPaneSkin

1605eec

openjdk bot added the rfr Ready for review label Nov 2, 2021

kevinrushforth requested review from aghaisas, johanvos and kevinrushforth November 2, 2021 12:52

aghaisas reviewed Dec 1, 2021

View reviewed changes

Handle division by zero

147858d

kevinrushforth reviewed Dec 2, 2021

View reviewed changes

Changes per review

6b8985f

kevinrushforth approved these changes Dec 3, 2021

View reviewed changes

aghaisas approved these changes Dec 6, 2021

View reviewed changes

openjdk bot added the ready Ready to be integrated label Dec 6, 2021

johanvos approved these changes Dec 6, 2021

View reviewed changes

openjdk bot added the sponsor Ready to sponsor label Dec 6, 2021

openjdk bot closed this Dec 6, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Dec 6, 2021

mstr2 deleted the fixes/scrollpane-speed branch January 22, 2023 16:59

		double vPixelValue = vRange / (nodeHeight - contentHeight);
		vPixelValue = Double.isFinite(vPixelValue) ? vPixelValue : 0.0;

Conversation

mstr2 commented Nov 2, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 2, 2021

Uh oh!

mlbridge bot commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Nov 2, 2021

Uh oh!

openjdk bot commented Nov 2, 2021

Uh oh!

aghaisas Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

mstr2 Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

aghaisas Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

mstr2 Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johanvos commented Dec 6, 2021

Uh oh!

johanvos left a comment

Choose a reason for hiding this comment

Uh oh!

mstr2 commented Dec 6, 2021

Uh oh!

openjdk bot commented Dec 6, 2021

Uh oh!

johanvos commented Dec 6, 2021

Uh oh!

openjdk bot commented Dec 6, 2021

Uh oh!

openjdk bot commented Dec 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

mstr2 commented Nov 2, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 2, 2021 •

edited

Loading

openjdk bot commented Dec 6, 2021 •

edited

Loading