jfx icon indicating copy to clipboard operation
jfx copied to clipboard
verified publisher icon openjdk

8326712: Robot tests fail on XWayland

Open azvegint opened this issue 1 year ago • 8 comments

Most of the headful test failures on XWayland are due to screen capture is not working.

Wayland, unlike X11, does not allow arbitrary applications to capture the screen contents directly. Instead, screen capture functionality is managed by the compositor, which can enforce stricter controls and permissions, requiring explicit user approval for screen capture operations.

This issue is already resolved in OpenJDK (base issue, there are subsequent fixes) by using the ScreenCast XDG portal.

The XDG ScreenCast portal is utilized for capturing screen data as it provides a secure and standardized method for applications to access screen content. Additionally, it allows for the reuse of user permissions without requiring repeated confirmations, streamlining the user experience and enhancing convenience.

So this changeset is a copy of the OpenJDK fixes with the addition of the JavaFX customization. For ease of review, you can skip the changes in the first two commits:

  • First commit is a direct copy of files from OpenJDK
  • Second commit removes the gtk- prefix before the gtk_... and g_... function calls (in AWT all gtk functions are dynamically loaded, we don't need that in JavaFX).

properties added:

  • javafx.robot.screenshotMethod, accepts gtk(existing gtk method) and dbusScreencast(added by this changeset, used by default for Wayland)
  • javafx.robot.screenshotDebug prints debug info if it is set to true

What are the remaining issues?

After applying this fix, system tests will pass except for the SwingNodeJDialogTest test.

This interop test calls java.awt.Robot#getPixelColor which internally gtk->g_main_context_iteration(NULL, TRUE); causes a blocking of javafx gtk loop, so the test hangs. So a change is required on OpenJDK side to fix this issue.

Even after solving the #1, the SwingNodeJDialogTest.testNodeRemovalBeforeShow case is still failing.

Internally the ScreenCast session keeps open for 2s. This is to reduce overhead in case of frequent consecutive screen captures.

There is a crash when an AWT ScreenCast session overlaps with the FX ScreenCast session. E.g. java.awt.Robot#getPixelColor() and javafx.scene.robot.Robot#getPixelColor() are called within 1 second.

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 2 Reviewers)

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1490

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1490.diff

Webrev

Link to Webrev Comment

azvegint avatar Jun 26 '24 11:06 azvegint

:wave: Welcome back azvegint! 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 Jun 26 '24 11:06 bridgekeeper[bot]

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

8326712: Robot tests fail on XWayland

Reviewed-by: kcr, prr, sykora

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 1 new commit pushed to the master branch:

  • 1fb8755dc6452bdb07685c02272ecfc578fb1eba: 8198830: BarChart: auto-range of CategoryAxis not working on dynamically setting data

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. 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[bot] avatar Jun 26 '24 11:06 openjdk[bot]

We'll want extra pairs of eyes on this one (so at least two "R"eviewers). I'd also like Gluon to verify that it builds on their CI system.

Reviewers: @kevinrushforth @prrace @tiainen

/reviewers 2 reviewers

kevinrushforth avatar Jun 27 '24 18:06 kevinrushforth

@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 2 Reviewers).

openjdk[bot] avatar Jun 27 '24 18:06 openjdk[bot]

@azvegint When you do take it out of Draft, please write up something in the Description about the changes as a help to those who will be reviewing it.

Also, can you merge in the latest master? In particular, I want to see a GHA run with the javac -Werror fix.

kevinrushforth avatar Jun 27 '24 18:06 kevinrushforth

Webrevs

mlbridge[bot] avatar Jun 28 '24 07:06 mlbridge[bot]

Just of out curiosity, please don't mind it too much:

Why the piperwire include files are included? Since the infraestucture should be present for it to work (pipewire is quite new and not present on older linux distributions) a runtime check is needed anyways.

What's the advantage of calling dbus functions directly and not use libportal?

tsayao avatar Jun 28 '24 13:06 tsayao

Just of out curiosity, please don't mind it too much:

Why the piperwire include files are included? Since the infraestucture should be present for it to work (pipewire is quite new and not present on older linux distributions) a runtime check is needed anyways.

What's the advantage of calling dbus functions directly and not use libportal?

This is mostly a copy and paste of the OpenJDK code, which should be able to be built on fairly old Linux distributions (consider OpenJDK backports to earlier versions), and we didn't want to add any new external dependencies there. So with this changeset, we are just bringing tested code with minimal changes to JFX.

azvegint avatar Jun 28 '24 15:06 azvegint

The code changes all look good. I've done a fair bit of testing already, and will finish up my testing tomorrow.

kevinrushforth avatar Jul 01 '24 22:07 kevinrushforth

/integrate

azvegint avatar Jul 03 '24 09:07 azvegint

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

  • 5656b80f8a584a2d4d791f9fab83f1719b29f986: 8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window
  • 6c3fb5f557597a5a226d4a7bd41d84b7feb4a162: 8289115: Touch events is not dispatched after upgrade to JAVAFX17+
  • 1fb8755dc6452bdb07685c02272ecfc578fb1eba: 8198830: BarChart: auto-range of CategoryAxis not working on dynamically setting data

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Jul 03 '24 09:07 openjdk[bot]

@azvegint Pushed as commit 459b15fc61e7a78f29c16fe665370657c1236b3f.

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

openjdk[bot] avatar Jul 03 '24 09:07 openjdk[bot]