6318027: BasicScrollBarUI does not disable timer when enclosing frame is disabled. #20346

prsadhuk · 2024-07-26T06:03:28Z

Issue is
BasicScrollBarUI.ArrowButtonListener starts a timer in mousePressed(), and stops it in mouseReleased(). If the frame containing the scrollbar is disabled between the MOUSE_PRESSED and the MOUSE_RELEASED events, the mouseReleased() method is never called. If the frame is then re-enabled, the still-running timer causes it to scroll all the way to the end.
Fix is to check if ArrowButtonListener.handledEvent is still set when ActionEvent is processed then stop the timer and reset this variable.

CI testing is green and also SwingSet2 JScrollPane scrolling with this modification..

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Warning

⚠️ Found trailing period in issue title for 6318027: BasicScrollBarUI does not disable timer when enclosing frame is disabled.

Issue

JDK-6318027: BasicScrollBarUI does not disable timer when enclosing frame is disabled. (Bug - P4)

Reviewers

Abhishek Kumar (@kumarabhi006 - Reviewer)
Tejesh R (@TejeshR13 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20346

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20346.diff

Webrev

Link to Webrev Comment

… is disabled.

bridgekeeper · 2024-07-26T06:03:57Z

👋 Welcome back psadhukhan! 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 · 2024-07-26T06:04:39Z

@prsadhuk 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:

6318027: BasicScrollBarUI does not disable timer when enclosing frame is disabled.

Reviewed-by: abhiscxk, tr

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 360 new commits pushed to the master branch:

88ccbb6: 8336934: Clean up JavaLangReflectAccess
d728107: 8338482: com/sun/jdi/ThreadMemoryLeakTest.java requires that compressed oops are enabled
1ebf2cf: 8336756: Improve ClassFile Annotation writing
0267284: 8338611: java.lang.module specification wording not aligned with JEP 261
c646efc: 8205957: setfldw001/TestDescription.java fails with bad field value
285ceb9: 8336529: (fs) UnixFileAttributeViews setTimes() failing on armhf, Ubuntu noble
55a97ec: 8336729: C2: Div/Mod nodes without zero check could be split through iv phi of outer loop of long counted loop nest resulting in SIGFPE
b442003: 8338623: StackCounter adding extraneous slots for receiver invoke instructions
bc2700b: 8282944: GHA: Add Alpine Linux x86_64 pre-integration check
686eb23: 8336817: Several methods on DatagramSocket and MulticastSocket do not specify behaviour when already closed or connected
... and 350 more: https://git.openjdk.org/jdk/compare/871362870ea8dc5f4ac186876e91023116891a5b...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-07-26T06:05:05Z

@prsadhuk The following label will be automatically applied to this pull request:

client

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.

mlbridge · 2024-07-26T06:14:00Z

Webrevs

kumarabhi006 · 2024-08-01T04:42:36Z

test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java

+                    frame = new JFrame(DisableFrameFromScrollBar.class.getName());
+                    bar = new JScrollBar();
+                    bar.getModel().addChangeListener(new DisableChangeListener(frame));
+                    frame.getContentPane().setLayout(new FlowLayout());
+                    frame.getContentPane().add(bar);
+
+                    frame.pack();
+                    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
+                    frame.setSize(150, 150);
+                    frame.setLocationRelativeTo(null);
+                    frame.setVisible(true);


Can be pushed to a helper method.

kumarabhi006 · 2024-08-01T04:53:14Z

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicScrollBarUI.java

+            // Stop the timer if handledEvent is still set indicating
+            // mouseReleased is not called after mousePressed when
+            // this AcionEvent is being processed
+            if (buttonListener.handledEvent) {


Since the fix is added in BasicScrollBarUI, will it affect Aqua ScrollBar also?

I think most L&F's ScrollBarUI classes are extending BasicScrollBarUI so it might be worth checking if this issue also occurs in those L&Fs too

Aqua ScrollBar does not have arrowbutton so ArrowButtonListener, as mentioned in JBS, is not there but there is trackpad and there is similar issue with it but it cannot be handled with this "handledEvent" and needs to be handled separately so as of now AquaL&F is omitted from the testing..Other L&F are fine as per the testing..

kumarabhi006 · 2024-08-01T05:02:37Z

test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java

+                    frame.getContentPane().setLayout(new FlowLayout());
+                    frame.getContentPane().add(bar);


I guess getContentPane may be removed.

kumarabhi006 · 2024-08-01T05:03:58Z

test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java

+
+                    frame.pack();
+                    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
+                    frame.setSize(150, 150);


setting frame size explicitly after calling frame.pack() looks redundant.

kumarabhi006 · 2024-08-01T05:05:52Z

test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java

+            public void run() {
+                Point p = bar.getLocationOnScreen();
+                Rectangle rect = bar.getBounds();
+                result[0] = new Point((int) (p.x + rect.width/2),


Suggested change

result[0] = new Point((int) (p.x + rect.width/2),

result[0] = new Point((int) (p.x + rect.width / 2),

kumarabhi006 · 2024-08-01T05:07:27Z

test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java

+
+    public static class DisableChangeListener implements ChangeListener {
+        private final JFrame m_frame;
+        private boolean m_done = false;


default value for a boolean is false, no need to set it explicitly.

kumarabhi006 · 2024-08-01T05:08:18Z

test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java

+        }
+
+        public void stateChanged(ChangeEvent p_e) {
+            if (! m_done) {


Suggested change

if (! m_done) {

if (!m_done) {

kumarabhi006 · 2024-08-09T06:50:20Z

@prsadhuk I was verifying the test and for Aqua L&F looks like the timer is not stopped and scrollbar scrolls down to the end and it doesn't throw any exception also.
Test ran fine for Metal, Nimbus and Motif L&Fs.

kumarabhi006 · 2024-08-09T06:54:27Z

src/java.desktop/macosx/classes/com/apple/laf/AquaScrollBarUI.java

@@ -527,6 +527,12 @@ void setScrollByBlock(final boolean block) {
        }

        public void actionPerformed(final ActionEvent e) {
+            if (fTrackHighlight != Hit.NONE && !fTrackListener.fStillInTrack) {


fTrackListener.fStillInTrack value is true and here the condition is to check for negation which makes it to false and overall condition evaluates to false and that may be the reason that the timer is not stopped.

Yes thats the drawback of indirect checking....I have modified PR to check for frame disable directly and stop the timer..

TejeshR13 · 2024-08-14T15:02:34Z

test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java

+                    Thread.sleep(200);
+                } while(isAdjusting && !doCheck);
+                if (bar.getValue() == (bar.getMaximum() - bar.getVisibleAmount())) {
+                    throw new RuntimeException("ScrollBar did't disable timer");


Suggested change

throw new RuntimeException("ScrollBar did't disable timer");

throw new RuntimeException("ScrollBar didn't disable timer");

TejeshR13 · 2024-08-14T15:06:39Z

test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java

+
+    private static JFrame frame;
+    private static JScrollBar bar;
+    private static Robot robot;


robot can be local variable.

mrserb · 2024-08-16T23:35:58Z

BasicScrollBarUI.ArrowButtonListener starts a timer in mousePressed(), and stops it in mouseReleased(). If the frame containing the scrollbar is disabled between the MOUSE_PRESSED and the MOUSE_RELEASED events, the mouseReleased() method is never called. If

What happens if you make the scrollbar invisible in this case?

mrserb · 2024-08-16T23:36:46Z

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicScrollBarUI.java

+            do {
+                if (parent instanceof javax.swing.JFrame par) {
+                    if (!par.isEnabled()) {
+                        ((Timer)e.getSource()).stop();


Is the "TImer" always the source for the event?

Yes, the method employs same way to stop the timer in other cases too..
Also, if the scrollbar is made invisible and then visible instead of frame, it doesn't scroll to the end and behaves as expected..

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicScrollBarUI.java

kumarabhi006 · 2024-08-20T04:30:25Z

src/java.desktop/macosx/classes/com/apple/laf/AquaScrollBarUI.java

+            do {
+                if (parent instanceof javax.swing.JFrame par) {
+                    if (!par.isEnabled()) {
+                        ((Timer)e.getSource()).stop();


Suggested change

((Timer)e.getSource()).stop();

((Timer) e.getSource()).stop();

same spacing issue in other places too as mentioned above...

Ok... as you are changing the file I guess you can expand the wild imports, It's up to you.

test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java

kumarabhi006

minor sorting of imports.

kumarabhi006 · 2024-08-20T09:59:08Z

src/java.desktop/macosx/classes/com/apple/laf/AquaScrollBarUI.java

+import java.awt.Graphics;
+import java.awt.Point;
+import java.awt.Rectangle;
+import java.awt.Insets;


Suggested change

import java.awt.Graphics;

import java.awt.Point;

import java.awt.Rectangle;

import java.awt.Insets;

import java.awt.Graphics;

import java.awt.Insets;

import java.awt.Point;

import java.awt.Rectangle;

kumarabhi006 · 2024-08-20T09:59:57Z

src/java.desktop/macosx/classes/com/apple/laf/AquaScrollBarUI.java

+import apple.laf.JRSUIConstants.Hit;
+import apple.laf.JRSUIConstants.Orientation;
+import apple.laf.JRSUIConstants.NothingToScroll;


Suggested change

import apple.laf.JRSUIConstants.Hit;

import apple.laf.JRSUIConstants.Orientation;

import apple.laf.JRSUIConstants.NothingToScroll;

import apple.laf.JRSUIConstants.Hit;

import apple.laf.JRSUIConstants.NothingToScroll;

import apple.laf.JRSUIConstants.Orientation;

kumarabhi006

Ran the test and verified the latest fix on different platforms (mac, windows and linux). It works as expected and looks good to me now.

TejeshR13

Looks good to me.

prsadhuk · 2024-08-21T07:02:48Z

/integrate

openjdk · 2024-08-21T07:04:07Z

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

88ccbb6: 8336934: Clean up JavaLangReflectAccess
d728107: 8338482: com/sun/jdi/ThreadMemoryLeakTest.java requires that compressed oops are enabled
1ebf2cf: 8336756: Improve ClassFile Annotation writing
0267284: 8338611: java.lang.module specification wording not aligned with JEP 261
c646efc: 8205957: setfldw001/TestDescription.java fails with bad field value
285ceb9: 8336529: (fs) UnixFileAttributeViews setTimes() failing on armhf, Ubuntu noble
55a97ec: 8336729: C2: Div/Mod nodes without zero check could be split through iv phi of outer loop of long counted loop nest resulting in SIGFPE
b442003: 8338623: StackCounter adding extraneous slots for receiver invoke instructions
bc2700b: 8282944: GHA: Add Alpine Linux x86_64 pre-integration check
686eb23: 8336817: Several methods on DatagramSocket and MulticastSocket do not specify behaviour when already closed or connected
... and 350 more: https://git.openjdk.org/jdk/compare/871362870ea8dc5f4ac186876e91023116891a5b...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-21T07:04:13Z

@prsadhuk Pushed as commit cafb3dc.

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

prsadhuk added 2 commits July 26, 2024 06:51

6318027: BasicScrollBarUI does not disable timer when enclosing frame…

c08a345

… is disabled.

Test format

bf58294

openjdk bot added the client client-libs-dev@openjdk.org label Jul 26, 2024

prsadhuk added 2 commits July 26, 2024 07:08

Test format

2bd61e9

Test format

dfe4532

openjdk bot added the rfr Pull request is ready for review label Jul 26, 2024

kumarabhi006 reviewed Aug 1, 2024

View reviewed changes

prsadhuk added 3 commits August 7, 2024 08:52

Test update

53d6f32

Restore AquaL&F testing

373442e

copyright

00fd5e5

kumarabhi006 reviewed Aug 9, 2024

View reviewed changes

Stop the timer if frame is disabled

526c6a1

TejeshR13 reviewed Aug 16, 2024

View reviewed changes

mrserb reviewed Aug 16, 2024

View reviewed changes

test typo

de7ad6b

kumarabhi006 reviewed Aug 20, 2024

View reviewed changes

prsadhuk added 2 commits August 20, 2024 07:31

Formatting

612b3ce

Expand imports

e15a263

kumarabhi006 reviewed Aug 20, 2024

View reviewed changes

Sort imports

f3a0e60

kumarabhi006 approved these changes Aug 21, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 21, 2024

TejeshR13 approved these changes Aug 21, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Aug 21, 2024

openjdk bot closed this Aug 21, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 21, 2024

prsadhuk deleted the JDK-6318027 branch August 21, 2024 07:12

		frame.getContentPane().setLayout(new FlowLayout());
		frame.getContentPane().add(bar);

	result[0] = new Point((int) (p.x + rect.width/2),
	result[0] = new Point((int) (p.x + rect.width / 2),

	throw new RuntimeException("ScrollBar did't disable timer");
	throw new RuntimeException("ScrollBar didn't disable timer");

	((Timer)e.getSource()).stop();
	((Timer) e.getSource()).stop();

6318027: BasicScrollBarUI does not disable timer when enclosing frame is disabled. #20346

6318027: BasicScrollBarUI does not disable timer when enclosing frame is disabled. #20346

Uh oh!

Conversation

prsadhuk commented Jul 26, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Warning

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jul 26, 2024

Uh oh!

openjdk bot commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jul 26, 2024

Uh oh!

mlbridge bot commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumarabhi006 commented Aug 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrserb commented Aug 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kumarabhi006 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumarabhi006 left a comment

prsadhuk commented Jul 26, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 26, 2024 •

edited

Loading

mlbridge bot commented Jul 26, 2024 •

edited

Loading