-
Notifications
You must be signed in to change notification settings - Fork 5.9k
8359061: Update and ProblemList manual test java/awt/Cursor/CursorDragTest/ListDragCursor.java #25705
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back dnguyen! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
test/jdk/ProblemList.txt
Outdated
@@ -834,6 +834,7 @@ java/awt/Checkbox/CheckboxNullLabelTest.java 8340870 windows-all | |||
java/awt/dnd/WinMoveFileToShellTest.java 8341665 windows-all | |||
java/awt/Menu/MenuVisibilityTest.java 8161110 macosx-all | |||
java/awt/Modal/NativeDialogToFrontBackTest.java 7188049 windows-all,linux-all | |||
java/awt/Cursor/CursorDragTest/ListDragCursor.java 8359061 macosx-all |
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.
Did you mean to use 7177297 in problemlist entry?
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.
Doesn't seem right either. There's a warning about this from the Skara bot.
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.
Should I update the issue number here to 7177297? I'm not fully grasping the Skara bot's error message.
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.
Currently, you problem-listing the test referencing the bug JDK-8359061, which is being fixed by this PR. Once you integrate it, JDK-8359061 will be resolved. Thus, the problem-list entry refers to the bug that's already resolved, which implies the problem why the test is problem-listed is addressed.
Yes, ListDragCursor.java
is problem-listed because of JDK-7177297. When JDK-7177297 is fixed, the test will be removed from the problem list.
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.
In other words, the bug id in the problem list refers to a JBS issue where the test failure is described, and that issue has to be open.
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.
Makes sense. Thanks. I have updated the bugID in the PL.
If the mouse cursor starts as a Text Line Cursor (I-beam), | ||
and does not change until you reached the List and | ||
released the left mouse button (and becomes a Hand Cursor), | ||
pass the test. This test fails if the cursor updates | ||
when pointing over the different components before | ||
dragging is complete. |
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.
The new instructions doesn't seem to hold true when I tested on windows.
Additionally the instructions require rephrasing for clarity?
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.
Perhaps, the instructions need to be adjusted for each OS, or the test should be limited to a specific OS only.
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.
It seems like there's a bug with this test when using it in a PassFailJFrame
. When I move the createUI
logic into a standalone java class's main method and add setVisible(true)
, the test works as expected for all OS's except macOS.
In this PassFailJFrame
it looks like it "works" correctly for 1 second, and then the cursor updates anyway afterwards. I'm going to try to look into why this is, but is this what you are seeing too @honkar-jdk ?
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.
I have attached the standalone test to the issue. I tried altering how ListDragCursor.java
creates the UI but it seems that no matter what I do, the cursor still updates after 1 second. I'm looking into what PassFailJFrame
does differently at the moment.
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.
I guess it's not the first time that we've encountered such an issue where using PassFailJFrame
changes the behaviour of the test. Was it @TejeshR13's test?
I'm confused because PassFailJFrame
does nothing more than provides a standard UI. It uses Swing components to display UI. Could this affect AWT components?
We have to look into what may cause the differences in behaviour.
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.
I have been looking at it and fiddling with the standalone (non-PassFailJFrame) test to see if I can re-create the behavior without PassFailJFrame
. I suspected AWT components being the issue but I can't see why.
I tried using EventQueue and SwingUtilities invokeAndWait()
method to see if either made a difference, but to no success. I tried different layouts for the frame but I got the same result. I'll keep looking but I may also append windows
to the ProblemList temporarily if I cannot find the source in a reasonable amount of time.
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.
Could it be that mere existence of other windows created by the same Java process somehow changes the cursors?
I mean PassFailJFrame
displays another window. What if you create a second window (that may display nothing inside) in your stand-alone test? Does it change what cursors you see when dragging?
If the mouse cursor starts as a Text Line Cursor (I-beam), | ||
and does not change until you reached the List and | ||
released the left mouse button (and becomes a Hand Cursor), | ||
pass the test. This test fails if the cursor updates | ||
when pointing over the different components before | ||
dragging is complete. |
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.
Perhaps, the instructions need to be adjusted for each OS, or the test should be limited to a specific OS only.
If the mouse cursor starts as a Text Line Cursor (I-beam), | ||
and does not change until you reached the List and |
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.
Is it really how native applications behave on macOS?
At least on Windows, the cursor changes as soon as drag operation is started. The cursor shouldn't remain the I-beam when you drag from one component to another.
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.
In Windows 11, I tried using the file explorer's
search text field. It turns into an I-beam and stays one until I let go of the left mouse button. I also tried with Cisco's VPN
settings button (cog wheel) that is a hand cursor and it behaves the same way. I also tried dragging from the file explorer's
search field to Cisco's
cog wheel and it stays an I-beam throughout.
In macOS, I used the search bar in Finder
to get an I-beam and drag. It also stays an I-beam until released.
test/jdk/ProblemList.txt
Outdated
@@ -834,6 +834,7 @@ java/awt/Checkbox/CheckboxNullLabelTest.java 8340870 windows-all | |||
java/awt/dnd/WinMoveFileToShellTest.java 8341665 windows-all | |||
java/awt/Menu/MenuVisibilityTest.java 8161110 macosx-all | |||
java/awt/Modal/NativeDialogToFrontBackTest.java 7188049 windows-all,linux-all | |||
java/awt/Cursor/CursorDragTest/ListDragCursor.java 8359061 macosx-all |
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.
Doesn't seem right either. There's a warning about this from the Skara bot.
This change is to restore the original intent of the test by updating the instructions to check that the type of Cursor is preserved when clicked and dragged. Now the test correctly has instructions to check that an I-beam cursor stays an I-beam until released over a List with its cursor being updated to a Hand cursor.
There is a bug where this does not correctly update in macOS (found in JDK-7177297). So, this test needs to be problem-listed.
I have confirmed that preserving the cursor image when dragging is native behavior across macOS, Windows, and Ubuntu. And I have checked that the test passes on both Windows and Ubuntu, while macOS fails and immediately updates the cursor as it leaves the TextArea.
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25705/head:pull/25705
$ git checkout pull/25705
Update a local copy of the PR:
$ git checkout pull/25705
$ git pull https://git.openjdk.org/jdk.git pull/25705/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25705
View PR using the GUI difftool:
$ git pr show -t 25705
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25705.diff
Using Webrev
Link to Webrev Comment