-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
8311689: Wrong visible amount in Adjustable of ScrollPane #14815
8311689: Wrong visible amount in Adjustable of ScrollPane #14815
Conversation
👋 Welcome back aivanov! A progress list of the required criteria for merging this PR into |
@aivanov-jdk The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
It ensures getViewportSize in the test is called after AwtScrollPane::_SetSpans completes on the toolkit thread.
@aivanov-jdk 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:
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 7 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally by applying the patch. Fix looks good.
/integrate |
Going to push as commit b3f3403.
Your commit was automatically rebased without conflicts. |
@aivanov-jdk Pushed as commit b3f3403. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
It proved that
ScrollPane.layout
depends on the result ofWScrollPanePeer.childResized
, specifically onsetSpans
which recalculates the spans and sets the insets.jdk/src/java.desktop/share/classes/java/awt/ScrollPane.java
Lines 514 to 521 in b285ed7
After the fix for JDK-8297923,
setSpans
that is called inchildResized
is run asynchronously on the toolkit thread. ThereforegetViewportSize
uses the wrong insets which don't take into account the size of the scroll bar. Because of it, thevisibleAmount
field of adjustables is also wrong, and the scroll pane cannot display a portion of its child component.I overlooked this dependency even when I was fixing the first regression, JDK-8310054. Had I followed Harshitha's advice in #14478, I would've fixed this problem too. Similarly, Phil's intuition was right:
setSpans
should be synchronous. @honkar-jdk @prraceFix
The fix is to run
AwtScrollPane::_SetSpans
synchronously usingAwtToolkit::InvokeFunction
.I also addressed Sergey's concern:
VerifyState
is now called afterSetInsets
. @mrserbClient tests are green.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14815/head:pull/14815
$ git checkout pull/14815
Update a local copy of the PR:
$ git checkout pull/14815
$ git pull https://git.openjdk.org/jdk.git pull/14815/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14815
View PR using the GUI difftool:
$ git pr show -t 14815
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14815.diff
Webrev
Link to Webrev Comment