-
Notifications
You must be signed in to change notification settings - Fork 6k
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 |
@DamonGuy 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 131 new commits pushed to the
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 |
Webrevs
|
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.
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.
I missed your earlier comment. Yes this seems to be true. Interestingly the cursor does not update when dragging across components until the left button is released with the standalone test on windows but with PFJ it does. I tried with PFJ's splitUI() & JPanel but the issue persists.
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.
Thanks for checking. I spent some time looking through the PFJFrame class and the standalone changing the test to better represent the PFJFrame version better, but I can't determine the exact cause. I will update this PR to include windows in the problem list and create a new JBS issue for windows.
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.
Since the standalone test works as expected on windows, I'm not sure if the test needs to be problemlisted on windows, as @aivanov-jdk mentioned earlier it may be a side-effect of PFJ.
I used the standalone test and added another dummy frame, even with extra frame the test works as expected. (cursor remains a I-beam until the left mouse btn is released)
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.
Since I reworked the test to avoid using PFJ, I don't need to do anything like PL the test for windows anymore. I'll just file a new bug for PFJ instead.
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.
Yes, that sounds correct. The cursor issue that was observed is specific to PFJ.
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.
I updated the test on @TejeshR13 's recommendation to refer to #22026 to work around having to use PFJFrame. Tested and this test works on Windows as expected. |
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 custom manual test frame changes LGTM apart for minor inline suggestions and works as expected on windows.
In fact, I also misinterpreted the scenario. I implied a text is dragged, but in this scenario no drag'n'drop operation happens, just text selection… as mouse is dragged. |
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.
Updated changes LGTM
Funny!
You submitted JDK-8359469 to find the problem… What Pause the timer, and the drag cursor doesn't change until you release the left mouse button. Harshitha was close to discovering it. |
That makes a lot of sense. I saw your updates on that issue as well. I guess we can use this workaround in this niche case if necessary. Thanks @aivanov-jdk and @honkar-jdk for your help in looking into this! |
Nice find ! Finally an answer to the strange behavior. |
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
Reviewers
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