jfx
jfx copied to clipboard
openjdk
8320965: Scrolling on a touch enabled display fails on Wayland
This PR replaces the deprecated gdk_pointer_grab with gdk_seat_grab, and gdk_pointer_ungrab with gdk_seat_ungrab, using runtime checks and wrapped functions for GTK 3.20+ (so systems without it still run with GTK 3.8+), and fixes the dragging issue on Wayland.
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)
Issue
- JDK-8320965: Scrolling on a touch enabled display fails on Wayland (Bug - P4)
Reviewers
- Kevin Rushforth (@kevinrushforth - Reviewer)
- Thiago Milczarek Sayao (@tsayao - Committer) 🔄 Re-review required (review applies to f8ffe87f)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1305/head:pull/1305
$ git checkout pull/1305
Update a local copy of the PR:
$ git checkout pull/1305
$ git pull https://git.openjdk.org/jfx.git pull/1305/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1305
View PR using the GUI difftool:
$ git pr show -t 1305
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1305.diff
Webrev
:wave: Welcome back jpereda! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
Webrevs
- 03: Full - Incremental (ca5766fd)
- 02: Full - Incremental (f8ffe87f)
- 01: Full - Incremental (99230ca6)
- 00: Full (f498b835)
@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).
The addition of the compile-time flags looks OK.
I did a build with GTK 3.22 (so it compiles the new code, does the dlsym, and does the runtime check) and verified that there are no regressions when running on an older system (Ubuntu 16.04).
I then did a full test run on our headful test systems, and there is one new test failure -- it seems to be intermittent, although fails pretty consistently on our Ubuntu 22.04 and Ubuntu 20.04 test systems. I can reproduce it locally on a VM running Ubuntu 22.04, where it fails about 1/2 the time with this patch applied (it fails more often on our physical test systems).
DatePickerTest > testDatePickerSceneChange FAILED
java.lang.AssertionError: Timeout: Failed to receive onAction call.
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.assertTrue(Assert.java:42)
at test.util.Util.waitForLatch(Util.java:400)
at test.robot.javafx.scene.DatePickerTest.clickDatePickerCalendarPopup(DatePickerTest.java:90)
at test.robot.javafx.scene.DatePickerTest.testDatePickerSceneChange(DatePickerTest.java:123)
Not sure what to make of this. I am not aware of any problems with this test, but it's possible that your fix has exposed a latent issue either in the test or somewhere else.
I did a build with GTK 3.22 (so it compiles the new code, does the dlsym, and does the runtime check) and verified that there are no regressions when running on an older system (Ubuntu 16.04).
That sounds good.
If we decide that is a reasonable thing to do, we might want the minimum build-time version of GTK to be higher than the minimum version we support at runtime.
That makes sense. We already do this with libc. When building (using the devkit based on the OpenJDK devkit), we use later versions of certain libraries, but we make sure the binaries still work on systems with older versions of the libraries. We should clearly state the minimum runtime requirements for some libraries (rather than for distributions, as it is always possible to hack new versions of libraries into old releases) . What I really want to avoid is having 2 (or more) versions of the binaries for the same platform/arch/os combination.
I did compile (updating gcc) and test on Ubuntu 16.04 and Ubuntu 23.10 with Xorg and Wayland. All OK. Code LGTM.
@jperedadnr This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@kevinrushforth is there anything else to be done before we can move this PR forward?
This dropped off my radar.
The only pending issue is the test failure I noted in an earlier comment:
I then did a full test run on our headful test systems, and there is one new test failure -- it seems to be intermittent, although fails pretty consistently on our Ubuntu 22.04 and Ubuntu 20.04 test systems. I can reproduce it locally on a VM running Ubuntu 22.04, where it fails about 1/2 the time with this patch applied (it fails more often on our physical test systems).
DatePickerTest > testDatePickerSceneChange FAILED java.lang.AssertionError: Timeout: Failed to receive onAction call. at org.junit.Assert.fail(Assert.java:89) at org.junit.Assert.assertTrue(Assert.java:42) at test.util.Util.waitForLatch(Util.java:400) at test.robot.javafx.scene.DatePickerTest.clickDatePickerCalendarPopup(DatePickerTest.java:90) at test.robot.javafx.scene.DatePickerTest.testDatePickerSceneChange(DatePickerTest.java:123)Not sure what to make of this. I am not aware of any problems with this test, but it's possible that your fix has exposed a latent issue either in the test or somewhere else.
@jperedadnr Can you try it on your system and see if you can reproduce this failure?
I'm testing on my local Linux Intel machine (Ubuntu 20.04), and running the test from head, it passes 5 out of 5 times. After applying this patch, it fails with your same stacktrace 2 out of 5 times.
With some printouts, I can see that when the test fails, in DatePickerTest::clickDatePickerCalendarPopup the call to mouseClick processes correctly the mouse move at the correct coordinates and calls mouse press and mouse release, but what it should trigger the datePicker setOnAction event (line 168), fails to do so. For some reason, the event doesn't make it to the control.
So it seems definitely related to the changes in this PR.
Running with GDK_DEBUG=nograbs works (10 out of 10).
Adding some debug to the native code, and running with grabs, I can see:
When it works:
DatePickerTest STANDARD_OUT
Glass GTK library to load is glassgtk3
loaded gdk_seat_grab
loaded gdk_display_get_default_seat
grab
grab
found schema 'org.gnome.desktop.interface' and key 'scaling-factor'
loaded gdk_seat_ungrab
ungrab
grab
ungrab
found schema 'org.gnome.desktop.interface' and key 'scaling-factor'
ungrab
grab
> Task :systemTests:test
...
(so test starts after window is grabbed and focused, as it should be).
When it fails:
DatePickerTest STANDARD_OUT
Glass GTK library to load is glassgtk3
loaded gdk_seat_grab
loaded gdk_display_get_default_seat
grab
grab
found schema 'org.gnome.desktop.interface' and key 'scaling-factor'
loaded gdk_seat_ungrab
ungrab
> Task :systemTests:test
...
so window is not grabbed and not focused, and therefore native mouse clicks go elsewhere.
For me it passes on XWayland and fails intermittently on Xorg (with Ubuntu 22.04).
It seems when it fails, button press is reported, but button release is not.
Yes, for some reason, after applying the changes from this PR, there are Leave events.
When it works:
loaded gdk_seat_grab
loaded gdk_display_get_default_seat
GlassApplication::process_events: GDK_ENTER_NOTIFY
found schema 'org.gnome.desktop.interface' and key 'scaling-factor'
GlassApplication::process_events: GDK_ENTER_NOTIFY
GlassApplication::process_events: GDK_MOTION_NOTIFY
GlassApplication::process_events: GDK_BUTTON_PRESS
GlassApplication::process_mouse_button: pressed
GlassApplication::process_events: GDK_BUTTON_RELEASE
GlassApplication::process_mouse_button: not pressed
...
When it fails:
loaded gdk_seat_grab
loaded gdk_display_get_default_seat
GlassApplication::process_events: GDK_LEAVE_NOTIFY
GlassApplication::process_events: GDK_ENTER_NOTIFY
GlassApplication::process_events: GDK_LEAVE_NOTIFY
GlassApplication::process_events: GDK_ENTER_NOTIFY
found schema 'org.gnome.desktop.interface' and key 'scaling-factor'
GlassApplication::process_events: GDK_LEAVE_NOTIFY
GlassApplication::process_events: GDK_MOTION_NOTIFY
GlassApplication::process_events: GDK_BUTTON_PRESS
GlassApplication::process_mouse_button: pressed
Java_com_sun_glass_ui_gtk_GtkWindow__1ungrabFocus()
ungrab focus()
...
Maybe this: https://docs.gtk.org/gdk3/enum.CrossingMode.html
Update: It's not.
A shot in the dark since I don't own a touch enabled monitor:
Test 1:
Add GDK_SCROLL_MASK on the original gdk_pointer_grab function;
Test 2:
This PR uses GDK_SEAT_CAPABILITY_ALL_POINTING which includes the touch masks.
So the equivalent would include GDK_TOUCH_MASK on gdk_pointer_grab.
I would bet on option 2.
Thanks @tsayao, I can definitely test those suggestions.
If I get it right, you are proposing this change:
status = gdk_pointer_grab(gdkWindow, owner_events, (GdkEventMask)
(GDK_POINTER_MOTION_MASK
| GDK_POINTER_MOTION_HINT_MASK
+ | GDK_TOUCH_MASK // or GDK_SCROLL_MASK
| GDK_BUTTON_MOTION_MASK
| GDK_BUTTON1_MOTION_MASK
| GDK_BUTTON2_MOTION_MASK
| GDK_BUTTON3_MOTION_MASK
| GDK_BUTTON_PRESS_MASK
| GDK_BUTTON_RELEASE_MASK
NULL, cursor, GDK_CURRENT_TIME);
But I don't see how this would fix the issue of the failing test on Linux that happens also without a touch enabled display. I tried both, the test still fails (two out of 5 times or so). Also, this is called only if there is no gdk_seat_grab function (which happens before GTK 3.20).
However, if I use GDK_SEAT_CAPABILITY_NONE instead of GDK_SEAT_CAPABILITY_ALL_POINTING, it passes 10 out of 10, but that would cancel pointer/touch events, wouldn't it?
Is there any chance that the robot implementation (via XTest, that uses XTestGrabControl) might not work fine with the new seat_grab approach after GTK 3.20+?
The rationale was:
This tells which events get delivered to the window while grabbing. XWayland might be sensitive to GDK_TOUCH_MASK while Xorg is not.
So the Idea was to keep the current way (with gdk_pointer_grab or gdk_device_grab, and adding the "deliver TOUCH events to me" might fix it.
Another place to investigate is:
#define GDK_FILTERED_EVENTS_MASK static_cast<GdkEventMask>(GDK_ALL_EVENTS_MASK \
& ~GDK_TOUCH_MASK)
It seems that Xorg converts touch events to regular mouse events, but XWayland might be different.
I see... so removing all the changes from this PR, and using this diff from head instead:
diff --git a/modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp b/modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp
index e7a230b2b6..e89db2a55f 100644
--- a/modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp
+++ b/modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp
@@ -605,7 +605,8 @@ glass_gdk_mouse_devices_grab_with_cursor(GdkWindow *gdkWindow, GdkCursor *cursor
| GDK_BUTTON2_MOTION_MASK
| GDK_BUTTON3_MOTION_MASK
| GDK_BUTTON_PRESS_MASK
- | GDK_BUTTON_RELEASE_MASK),
+ | GDK_BUTTON_RELEASE_MASK
+ | GDK_TOUCH_MASK),
NULL, cursor, GDK_CURRENT_TIME);
return (status == GDK_GRAB_SUCCESS) ? TRUE : FALSE;
it looks like dragging works on a touch enabled display and Wayland. (No changes needed in GDK_FILTERED_EVENTS_MASK).
Tested this change on X11 as well, I don't see any issue.
I've just reverted the previous changes, and just applied the touch mask to the gdk_pointer_grab function. Tests should be green now.
@jperedadnr Would you confirm that scroll on a touch screen still works on Xorg ?
My tests looks all good (manual and system/robot).
@tsayao yes, I've double checked again. On Wayland and Xorg, I can scroll with touch events on a touch screen with this fix. The only issue I still see on Wayland: the mouse cursor location doesn't get updated from touch events, only from mouse events (I guess this can be a follow-up issue).
@jperedadnr 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:
8320965: Scrolling on a touch enabled display fails on Wayland
Reviewed-by: kcr, tsayao
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 59 new commits pushed to the master branch:
- b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
- d9263ab268d33104279755dc1de139bd0835fdd7: 8301900: TextArea: Committing text with ENTER in an IME window inserts newline
- afa206b546a580a784d830712be174bb84f09ee9: 8321603: Bump minimum JDK version for JavaFX to JDK 21
- e0b88bc7cfede46afe28cbb4a2e333df933b5100: 8278021: Fix warnings in macOS glass native code and treat warnings as errors
- 9a06bf9c5bce40cf842ddf25fe3360c366c3156c: 8322748: Caret blinking in JavaFX should only stop when caret moves
- ee8633cb6d19b6da7bf32ad3cdee31261a7cf458: 8089373: Translation from character to key code is not sufficient
- 1fb56e333bc65860cc1abeebd1cbb01cd8b8e5f3: 8323880: Caret rendered at wrong position in case of a click event on RTL text
- 2754939556aa767a298fcf24f5caaa277351a4d7: 8309374: Accessibility Focus Rectangle on ListItem is not drawn when ListView is shown for first time
- 48be7d36d5033d215d585619f34c2dde518e4b53: 8324182: Deprecate for removal SimpleSelector and CompoundSelector classes
- 49d7d52fdf1c7341538e5bf4bd1a841dcc85c906: 8325798: Spinner throws uncatchable exception on tab out from garbled text
- ... and 49 more: https://git.openjdk.org/jfx/compare/f03bb74f8847126340cbf23379ad9669ff0074a6...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.
Going to push as commit df3707d7444c542ba55a8e76a8ed7e8f0637e874.
Since your change was applied there have been 59 commits pushed to the master branch:
- b43c4edf7590429fd051d1b0e2ccb6dd49a10b8b: 8325550: Grammatical error in AnchorPane.setLeftAnchor (and other setters) javadoc
- d9263ab268d33104279755dc1de139bd0835fdd7: 8301900: TextArea: Committing text with ENTER in an IME window inserts newline
- afa206b546a580a784d830712be174bb84f09ee9: 8321603: Bump minimum JDK version for JavaFX to JDK 21
- e0b88bc7cfede46afe28cbb4a2e333df933b5100: 8278021: Fix warnings in macOS glass native code and treat warnings as errors
- 9a06bf9c5bce40cf842ddf25fe3360c366c3156c: 8322748: Caret blinking in JavaFX should only stop when caret moves
- ee8633cb6d19b6da7bf32ad3cdee31261a7cf458: 8089373: Translation from character to key code is not sufficient
- 1fb56e333bc65860cc1abeebd1cbb01cd8b8e5f3: 8323880: Caret rendered at wrong position in case of a click event on RTL text
- 2754939556aa767a298fcf24f5caaa277351a4d7: 8309374: Accessibility Focus Rectangle on ListItem is not drawn when ListView is shown for first time
- 48be7d36d5033d215d585619f34c2dde518e4b53: 8324182: Deprecate for removal SimpleSelector and CompoundSelector classes
- 49d7d52fdf1c7341538e5bf4bd1a841dcc85c906: 8325798: Spinner throws uncatchable exception on tab out from garbled text
- ... and 49 more: https://git.openjdk.org/jfx/compare/f03bb74f8847126340cbf23379ad9669ff0074a6...master
Your commit was automatically rebased without conflicts.
@jperedadnr Pushed as commit df3707d7444c542ba55a8e76a8ed7e8f0637e874.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.