8307779: Relax the java.awt.Robot specification #13809
Conversation
|
👋 Welcome back azvegint! A progress list of the required criteria for merging this PR into |
resetScreenCapturePermission -> revokeScreenCapturePermission
| * and {@link #createScreenCapture(Rectangle)} may request | ||
| * a new permission from the user on applicable platforms. | ||
| */ | ||
| public void revokeScreenCapturePermission() { |
There was a problem hiding this comment.
I do not understand why we need this method.
Also we are separating spec. relaxation into this bug / PR
which BTW means it needs a new bug ID.
But this would be a new API, so it would need to go along with the code that implements it.
Perhaps that's why you added the default method below .. but it isn't really the same thing.
There was a problem hiding this comment.
I do not understand why we need this method.
We need it in order to be able to revoke the saved permission.
For example, it may be necessary if the user has two monitors, but the user only allowed access to one of them.
If he later decides to get a screen capture of two monitors, with the current implementation it will be impossible to do so without this method.
Alternatively, we can replace it with a system property that says should we keep the permission or not.
It is discussed here #13803 (comment)
it needs a new bug ID.
done
There was a problem hiding this comment.
Updated according to your comments.
I still left revokeScreenCapturePermission here for now, so that its documentation can be easily compared and aligned with the other documentation changes.
After finalization it will be moved to #13803 (if we decide to keep the method)
There was a problem hiding this comment.
I agree with Phil (and Sergey, who mentioned the same earlier). I'm not sure I see enough motivation for this method to justify adding it. It seems more like a workaround for an implementation quirk of Wayland to me.
There was a problem hiding this comment.
Like I wrote elsewhere, I am not sure how the app would know when to call this.
And it is easy enough to use the preferences API to clear this out whilst we think more on this. BTW "reset" or "clear" might be better than "revoke" .. for all we know the current token denies access anyway and what we really mean is more like those other suggestions.
| @@ -383,8 +395,25 @@ private static void checkKeycodeArgument(int keycode) { | |||
|
|
|||
| /** | |||
| * Returns the color of a pixel at the given screen coordinates. | |||
| * | |||
| * @implNote On Linux systems using Wayland, permission may be requested | |||
There was a problem hiding this comment.
We need more than an implNote here. It needs to be spec.
Also the words about system dialog don't belong. There may be other ways.
And "may be black" ? That's not very useful. Either guarantee its black or say "unspecified".
Is black guaranteed today if we fail to grab it ?
But also it seems to contradict the SecurityException docs .. how do you get black if an exception is thrown ?
There was a problem hiding this comment.
Is black guaranteed today if we fail to grab it ?
Currently we are passing a java pixelArray it to native to fill it up with screen capture data.
If we do not change this array(or part related to denied screen) it remains black.
But I'll change it to "unspecified"
how do you get black if an exception is thrown ?
Exception is thrown only if user has denied screen capture at all(does not grant permission to at least one screen).
In the case where user is trying to retrieve pixels from a screen to which the user has not given access he'll get the black pixels.
Alternatively, we can also give up on the SecurityException and not throw it at all in this case.
| * @param x X position of pixel | ||
| * @param y Y position of pixel | ||
| * @throws SecurityException if {@code readDisplayPixels} permission | ||
| * is not granted, or user has denied screen capture for all | ||
| * screens |
There was a problem hiding this comment.
* SecurityException if
(1) {@code readDisplayPixels} permission is not granted, or
(2) user has denied screen capture for all screens
(1) is the java SecurityManager scenario. Since that's not yet removed, we do need those words, so OK
(2) Is there some subtlety about "ALL" screens ? Do you really mean all screens which are in the area being captured ? Does that also imply you can get a partial capture if it spans 2 screens and you only have permission for one of them ? I'm not sure how useful that is in the "screenCapture" case and its really odd for the single pixel capture .. which surely is only on one screen ?
We do also need to think forward to a pure wayland scenario - any differences there. ?
The word "Wayland" shouldn't appear in the spec
My proposal -
If the desktop environment requires that permissions be granted to
capture screen content, and the required permissions are not granted,
then a {@code SecurityException} may be thrown, or the contents of
the returned {@code BufferedImage} are undefined.
@apiNote:
It is recommended to avoid calling this method on the
AWT Event Dispatch Thread since screen capture may be a lengthy operation,
particularly if acquiring permissions is needed and involves user interaction.
@throws SecurityException if {@code readDisplayPixels} permission
* is not granted, or user has denied screen capture for all
* required screens
====
I added the word REQUIRED here, since if we only need screen 1 contents, surely it
doesn't matter if we don't have screen 2 permission ?
There was a problem hiding this comment.
Is there some subtlety about "ALL" screens ?
It is more like "the user has not allowed any of his screens to be captured."
In fact, we may not throw out this exception and just provide black pixels(and call it "unspecified" behavior).
Do you really mean all screens which are in the area being captured ?
I added the word REQUIRED here, since if we only need screen 1 contents, surely it
doesn't matter if we don't have screen 2 permission ?
That's not exactly true.
There are screens that the user may or may not have given permission to capture.
This permission(specifically restore_token) is one at a single moment in time, not several separate permissions for each screen. This restore_token is associated with the permissions for specific screens.
There is an area that the user wants to capture.
The intersection of the capture area and the screens allowed for capture will have actual data from the screen, otherwise - black pixels.
So if a user allows access only to screen A and then asks for pixels from screen B he will get black pixels.
This is about the same when we have a single monitor and we capture an area outside of it (e.g. -100, -100, 50, 50) - we get a black square.
Does that also imply you can get a partial capture if it spans 2 screens and you only have permission for one of them ?
Yes, the plan is to provide what we can and leave the other parts black.
We do also need to think forward to a pure wayland scenario - any differences there. ?
As a screen capture - should work as it is.
The only concern is the impossibility to specify and get the window coordinates.
This way we won't know where the window we are displaying is located.
It will affect the testing approach and we will have to add a method like captureWindowContents to check the window contents.
| * Trying to interact with the Wayland area of responsibility may have no effect | ||
| * (e.g. interacting with window decorations, trying to change the | ||
| * window stacking order with the mouse or keyboard shortcuts) | ||
| * |
There was a problem hiding this comment.
We won't be running in compatibility mode when we have a pure wayland port .. I think it would
be nice if we could get this wording done in such a way that it applies to both .. and even covers
the situation on macOS too.
Also this sort of overlooks that we don't support wayland today and won't be able to do screen
capture in "default" mode .. so its really tricky to know whether to introduce wording like this.
Here's my first stab. This is pure SPEC, not "notes" or "impl"
* Platforms and desktop environments may impose restrictions or limitations
* on the access required to implement all functionality in the Robot class.
* For example
* + preventing access to the contents of any part of a desktop or Window on the desktop that is not owned by the running application
* + treating window decorations as non-owned content.
* + ignoring or limiting specific requests to manipulate windows.
* + ignoring or limiting specific requests for Robot generated (synthesized) events related to keyboard and mouse etc.
* + requiring specific or global permissions to any access to window contents, even application owned content,
* or to perform even limited synthesizing of events.
*
* The Robot API specification requires that approvals for these be granted for full operation.
* If they are not granted, the API will be degraded as discussed here.
* Relevant specific API methods may document more specific limitations and requirements.
* Depending on the policies of the desktop environment, the approvals mentioned above may
+ be required every time
+ or persistent for the lifetime of an application,
+ or persistent across multiple user desktop sessions
+ be fine-grained permissions
+ be associated with a specific binary application, or a class of binary applications.
When such approvals need to given interactively, it may impede the normal operation of the
application until approved, and if approval is denied or not possible, or cannot be made persistent
then it will degrade the functionality of this class and in turn any part of the operation of the
application which is dependent on it.
azvegint
changed the title
|
/csr |
|
@azvegint has indicated that a compatibility and specification (CSR) request is needed for this pull request. @azvegint please create a CSR request for issue JDK-8307779 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
Webrevs
|
| @@ -397,10 +455,27 @@ public synchronized Color getPixelColor(int x, int y) { | |||
| /** | |||
| * Creates an image containing pixels read from the screen. This image does | |||
| * not include the mouse cursor. | |||
There was a problem hiding this comment.
We have seen in macOS specifically that absence of mouse cursor is not guaranteed and screen capture can contain mouse cursor, so I guess we should remove this line since we are updating the Robot specification now..
There was a problem hiding this comment.
Sure, this is a good time to do it.
| * @implNote the mouse pointer may not visually move on Linux systems | ||
| * using Wayland, while the subsequent mousePress and mouseRelease | ||
| * can be delivered to the correct location |
There was a problem hiding this comment.
Since Wayland is an example, maybe say something more like this?
"...may not visually move on some platforms, such as Linux systems using Wayland..."
There was a problem hiding this comment.
Updated.
Probably this can be covered by the line 80:
<li> ignoring or limiting specific requests for Robot generated (synthesized) events related to keyboard and mouse etc.</li>
| * then a {@code SecurityException} may be thrown, | ||
| * or the content of the returned {@code Color} is undefined. |
There was a problem hiding this comment.
Given that a SecurityException is only thrown on some platforms, is it worth throwing at all? Undefined colors can still occur, so I'm not sure I see much value in an exception. What do you envision that an application would do with the exception?
There was a problem hiding this comment.
I agree, I'll try to come up with something less confusing.
There was a problem hiding this comment.
If there's no exception, how does the application know the colours aren't valid ?
I think throwing the exception should be preferred behaviour and undefined colours would be just if we had no way to detect that there was a problem.
There was a problem hiding this comment.
BTW in the implementation PR, Alexander wrote
The DENY case propagated to java and we are throwing the security exception.
So I interpret this as there is a use case for the exception.
| * and {@link #createScreenCapture(Rectangle)} may request | ||
| * a new permission from the user on applicable platforms. | ||
| */ | ||
| public void revokeScreenCapturePermission() { |
There was a problem hiding this comment.
I agree with Phil (and Sergey, who mentioned the same earlier). I'm not sure I see enough motivation for this method to justify adding it. It seems more like a workaround for an implementation quirk of Wayland to me.
| @@ -189,6 +226,11 @@ private static void checkIsScreenDevice(GraphicsDevice device) { | |||
|
|
|||
| /** | |||
| * Moves mouse pointer to given screen coordinates. | |||
| * @implNote the mouse pointer may not visually move on some platforms, | |||
There was a problem hiding this comment.
This doesn't seem to be an implNote in the way that the JEP
intends since it has nothing to do with anything we did in our implementation. I think it would have to be just spec, in which case I am not sure we should call out Linux+Wayland
See https://openjdk.org/jeps/8068562
"Implementation Notes. This section contains informative notes about the implementation, such as advice to implementors, or performance characteristics that are specific to the implementation in this class of this version of the JDK. The information in this section is subject to change from release to release. These characteristics are also allowed to vary across platforms, vendors, and versions."
| * then a {@code SecurityException} may be thrown, | ||
| * or the content of the returned {@code Color} is undefined. |
There was a problem hiding this comment.
If there's no exception, how does the application know the colours aren't valid ?
I think throwing the exception should be preferred behaviour and undefined colours would be just if we had no way to detect that there was a problem.
| * <p> | ||
| * The {@link #revokeScreenCapturePermission()} can be used to revoke | ||
| * a previously granted permission. | ||
| * <p> |
There was a problem hiding this comment.
I would like to completely separate any decision on adding revokeScreenCapturePermission from
this spec. relaxation.
So we should remove the reference to it here into a separate PR about adding that API although I'm still not convinced we should add it.
| * are not greater than zero | ||
| * @throws SecurityException if {@code readDisplayPixels} permission | ||
| * is not granted, or user has not allowed any of his screens | ||
| * to be captured. |
There was a problem hiding this comment.
"user" -> "the user"
"his" -> gender neutral "their".
Or maybe better ", or access to the screen is denied by the desktop environment"
There was a problem hiding this comment.
Updated to the former for now.
There was a problem hiding this comment.
I think you should draft the CSR now based on the current text.
We may still need to tweak words like
"or the user has not allowed any of their screens to be captured."
based on developing understanding, or perhaps being careful not to be describe in the docs what happens to be the pipewire behaviour.
In fact that is why I suggested my other words, because (1) there may be no user involved and
(2) screen vs screens seems to be a pipewire / wayland concept. I'm pretty sure macOS just treats it as "the desktop" no matter how many monitors are involved.
| * and {@link #createScreenCapture(Rectangle)} may request | ||
| * a new permission from the user on applicable platforms. | ||
| */ | ||
| public void revokeScreenCapturePermission() { |
There was a problem hiding this comment.
Like I wrote elsewhere, I am not sure how the app would know when to call this.
And it is easy enough to use the preferences API to clear this out whilst we think more on this. BTW "reset" or "clear" might be better than "revoke" .. for all we know the current token denies access anyway and what we really mean is more like those other suggestions.
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * Copyright (c) 1999, 2020, Oracle and/or its affiliates. All rights reserved. | |||
| * Copyright (c) 1999, 2023, Oracle and/or its affiliates. All rights reserved. | |||
There was a problem hiding this comment.
This file no longer needed in this PR
| * are not greater than zero | ||
| * @throws SecurityException if {@code readDisplayPixels} permission | ||
| * is not granted, or user has not allowed any of his screens | ||
| * to be captured. |
There was a problem hiding this comment.
I think you should draft the CSR now based on the current text.
We may still need to tweak words like
"or the user has not allowed any of their screens to be captured."
based on developing understanding, or perhaps being careful not to be describe in the docs what happens to be the pipewire behaviour.
In fact that is why I suggested my other words, because (1) there may be no user involved and
(2) screen vs screens seems to be a pipewire / wayland concept. I'm pretty sure macOS just treats it as "the desktop" no matter how many monitors are involved.
| * are not greater than zero | ||
| * @throws SecurityException if {@code readDisplayPixels} permission | ||
| * is not granted, or access to the screen is denied | ||
| * by the desktop environment | ||
| * @see SecurityManager#checkPermission | ||
| * @see AWTPermission | ||
| */ |
There was a problem hiding this comment.
BTW, all of the above also needs to be added to createMultiResolutionScreenCapture(..) !
|
@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: 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 228 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 |
|
/integrate |
|
Going to push as commit 21aa057.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
We need to relax the java.awt.Robot specification according to the latest operating system trends.
This should at least cover the case of Wayland, which has changed many familiar concepts in Linux.
https://bugs.openjdk.org/browse/JDK-8280982 [Wayland] [XWayland] java.awt.Robot taking screenshots
https://bugs.openjdk.org/browse/JDK-8280995 [XWayland] Robot.mouseMove does not visually move mouse cursor
https://bugs.openjdk.org/browse/JDK-8280990 [XWayland] XTest emulated mouse click does not bring window to front.
https://bugs.openjdk.org/browse/JDK-8280988 [XWayland] Click on title to request focus test failures
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13809/head:pull/13809$ git checkout pull/13809Update a local copy of the PR:
$ git checkout pull/13809$ git pull https://git.openjdk.org/jdk.git pull/13809/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 13809View PR using the GUI difftool:
$ git pr show -t 13809Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13809.diff
Webrev
Link to Webrev Comment