8279227: Access Bridge: Wrong frame position and hit test result on HiDPI display by VeselovAlex · Pull Request #72 · openjdk/jdk18

VeselovAlex · 2021-12-24T09:51:14Z

Perform scaling while converting sizes and coordinates to provide correct positions of accessible elements on HiDPI screen and make hit-test work properly. It uses FontRenderContext to receive display scale without API changes.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8279227: Access Bridge: Wrong frame position and hit test result on HiDPI display

Reviewers

Anton Tarasov (@forantar - Reviewer) ⚠️ Review applies to 4ad508f
Alexander Zuev (@azuev-java - Reviewer)
Alexey Ivanov (@aivanov-jdk - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk18 pull/72/head:pull/72
$ git checkout pull/72

Update a local copy of the PR:
$ git checkout pull/72
$ git pull https://git.openjdk.java.net/jdk18 pull/72/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 72

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk18/pull/72.diff

…iDPI display Perform scaling while converting sizes and coordinates to provide correct positions of accessible elements on HiDPI screen and make hit-test work properly. It uses FontRenderContext to receive display scale without API changes.

bridgekeeper · 2021-12-24T09:53:02Z

👋 Welcome back VeselovAlex! 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.

openjdk · 2021-12-24T09:55:49Z

@VeselovAlex The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-12-24T09:59:38Z

Webrevs

openjdk · 2021-12-24T14:23:30Z

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

8279227: Access Bridge: Wrong frame position and hit test result on HiDPI display

Reviewed-by: ant, kizune, aivanov

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 41 new commits pushed to the master branch:

bd35f97: 8278628: jdk/jfr/jmx/streaming/TestMaxSize.java Expected only one or two chunks
e38df21: 8256291: RunThese30M fails "assert(_class_unload ? true : ((((JfrTraceIdBits::load(class_loader_klass)) & ((1 << 4) << 8)) != 0))) failed: invariant"
4d9b3f4: 8279998: PPC64 debug builds fail with "untested: RangeCheckStub: predicate_failed_trap_id"
09d61b6: 8280034: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64
c809d34: 8279924: [PPC64, s390] implement frame::is_interpreted_frame_valid checks
4b520f0: 8279702: [macosx] ignore xcodebuild warnings on M1
c6b0275: 8279930: Synthetic cast causes generation of store barriers when using heap segments
45f2063: 8279597: [TESTBUG] ReturnBlobToWrongHeapTest.java fails with -XX:TieredStopAtLevel=1 on machines with many cores
064ee6a: 8278434: timeouts in test java/time/test/java/time/format/TestZoneTextPrinterParser.java
ff85659: 8279833: Loop optimization issue in String.encodeUTF8_UTF16
... and 31 more: https://git.openjdk.java.net/jdk18/compare/6588bedc19ab42cec9e5bb6f13be14fb4dc5a655...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@forantar, @azuev-java, @aivanov-jdk) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

aivanov-jdk · 2021-12-24T16:19:42Z

@mrserb take a look

mrserb · 2021-12-24T23:57:51Z

+                                    r.y = (int) Math.floor(r.y * at.getScaleY());
+                                    r.width = (int) Math.ceil(r.width * at.getScaleX());
+                                    r.height = (int) Math.ceil(r.height * at.getScaleY());
+                                }


This logic will not work on the multiscreen configs when the monitors have different DPI, the logic should be similar to the SunGraphicsEnvironment.toDeviceSpace().
and probably the round logic should use Region.clipRound as well.

If this data will be used in native then we can try to use device->ScaleDownAbsX/Y, etc methods.

mrserb · 2021-12-25T00:00:03Z

+        if (acomp != null) {
+            FontMetrics fm = acomp.getFontMetrics(acomp.getFont());
+            if (fm != null) {
+                return fm.getFontRenderContext().getTransform();


I am not sure do we really update the FRC for the component when the DPI is changed?

I've checked it - it does not change, which is unexpected... Doesn't it seem like a bug?

Actually, the problem is that AccessibleComponent does not provide the component's GC, which is a missing. Ideally would be to add it to the API and then use - because it already contains actual scales for the component. It's a pity to do all that calculations just because we lack API calls...

Is it worth trying to fix the font metrics update instead and use it until the new API is ready and available?

I've checked it - it does not change, which is unexpected... Doesn't it seem like a bug?

Yes, I think this is a bug.

Actually, the problem is that AccessibleComponent does not provide the component's GC, which is a missing. Ideally would be to add it to the API and then use - because it already contains actual scales for the component. It's a pity to do all that calculations just because we lack API calls...

But do we actually need the GC where the component is located, or do we need a GC where the a11y frame should be placed? I am not sure that both are always the same. Note that you can find the screen where the "virtual coordinates are located" and then convert them to the device space.

But do we actually need the GC where the component is located, or do we need a GC where the a11y frame should be placed? I am not sure that both are always the same. Note that you can find the screen where the "virtual coordinates are located" and then convert them to the device space.

Well, in AccessBridge.getAccessibleContextAt_1(int x, int y, AccessibleContext parent) we expect [x, y] within the parent bounds and so we can assume [x, y] has the same scaling. However, AccessBridge.getAccessibleContextAt_2 works with generic [x, y] and there we have to perform generic transformation, indeed...

Ok, if we can't avoid it, then I'm ok with the latest version (taking into account my inline comments). Thanks!

VeselovAlex · 2021-12-25T17:42:20Z

Hi, @mrserb, thanks for the review. Your idea is awesome and I've tried to implement it. It contains some copied code from sun.java2d but I can't import necessary methods without adding it to dependencies.

But the issue with different scale factors on multi-display configuration is still present, frame size scaling seems to be stuck at primary display's scale factor. jaccesibilityinspector shows correctly scaled bounds, may it be an issue with NVDA?

forantar · 2021-12-27T14:55:10Z

+         *
+         * @param  current the default configuration which is checked in the first
+         *         place
+         * @param  x the x coordinate of the given point


Please specify in which space the coordinates are expected.

Hi, @mrserb, thanks for the review. Your idea is awesome and I've tried to implement it. It contains some copied code from sun.java2d but I can't import necessary methods without adding it to dependencies.

Probably we can export that to the "jdk.accessibility" module? We already export "sun.awt" the same way. @prrace any thoughts?
But it will be good to make sure that it will work on a multiscreen config before doing that. At least need to understand the root cause of that.

forantar · 2021-12-27T14:57:31Z

+         */
+        public static GraphicsConfiguration getGraphicsConfigurationAtPoint(
+                GraphicsConfiguration current, double x, double y) {
+            if (current.getBounds().contains(x, y)) {


Here you do not translate the bounds to device space. Actually, I'd remove this check at all, as below you duplicate it. Also, when you call the method you pass the default GC, so it seems it does not bring much profit...

Also move GraphicsConfiguration search into AccessibilityGraphicsEnvironment

VeselovAlex · 2022-01-14T17:34:11Z

I've fixed receiving GC at given point and now hit-test and frame bounds seem to be calculated correctly. But it looks like NVDA does not work correctly with different DPI on the second display, so it (at least v. 2021.3.1) has to be started in compatibility mode, see the attached picture

forantar · 2022-01-17T16:25:45Z

Tested your patch with NVDA 2021.3.1 and it worked well for me.

forantar · 2022-01-17T16:38:10Z

I've fixed receiving GC at given point and now hit-test and frame bounds seem to be calculated correctly. But it looks like NVDA does not work correctly with different DPI on the second display, so it (at least v. 2021.3.1) has to be started in compatibility mode, see the attached picture

Seems worth reporting this to NVDA (https://github.com/nvaccess/nvda/#communication-channels).

azuev-java

Tested on Windows 10 and Windows 11 with dual monitor configuration with different scale factors - last version of the fix seems to be working fine.

forantar · 2022-01-18T18:20:07Z

Based on 3 approves and successful testing this fix can be integrated (we are approaching RDP2 for JDK18).

openjdk · 2022-01-18T18:21:37Z

@forantar Only the author (@VeselovAlex) is allowed to issue the integrate command.

VeselovAlex · 2022-01-18T18:28:41Z

/integrate

openjdk · 2022-01-18T18:30:01Z

@VeselovAlex
Your change (at version b0c2037) is now ready to be sponsored by a Committer.

forantar · 2022-01-18T18:50:46Z

/sponsor

openjdk · 2022-01-18T18:52:10Z

Going to push as commit 20ef954.
Since your change was applied there have been 41 commits pushed to the master branch:

bd35f97: 8278628: jdk/jfr/jmx/streaming/TestMaxSize.java Expected only one or two chunks
e38df21: 8256291: RunThese30M fails "assert(_class_unload ? true : ((((JfrTraceIdBits::load(class_loader_klass)) & ((1 << 4) << 8)) != 0))) failed: invariant"
4d9b3f4: 8279998: PPC64 debug builds fail with "untested: RangeCheckStub: predicate_failed_trap_id"
09d61b6: 8280034: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64
c809d34: 8279924: [PPC64, s390] implement frame::is_interpreted_frame_valid checks
4b520f0: 8279702: [macosx] ignore xcodebuild warnings on M1
c6b0275: 8279930: Synthetic cast causes generation of store barriers when using heap segments
45f2063: 8279597: [TESTBUG] ReturnBlobToWrongHeapTest.java fails with -XX:TieredStopAtLevel=1 on machines with many cores
064ee6a: 8278434: timeouts in test java/time/test/java/time/format/TestZoneTextPrinterParser.java
ff85659: 8279833: Loop optimization issue in String.encodeUTF8_UTF16
... and 31 more: https://git.openjdk.java.net/jdk18/compare/6588bedc19ab42cec9e5bb6f13be14fb4dc5a655...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-01-18T18:52:32Z

@forantar @VeselovAlex Pushed as commit 20ef954.

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

openjdk Bot added the rfr Pull request is ready for review label Dec 24, 2021

openjdk Bot added the client client-libs-dev@openjdk.java.net label Dec 24, 2021

Fix missing import

4ad508f

forantar approved these changes Dec 24, 2021

View reviewed changes

openjdk Bot added the ready Pull request is ready to be integrated label Dec 24, 2021

mrserb reviewed Dec 24, 2021

View reviewed changes

mrserb reviewed Dec 25, 2021

View reviewed changes

Pick graphics configuration from env instead of using FRC

5232794

forantar reviewed Dec 27, 2021

View reviewed changes

Fix converting coordinates when getting GC at point

b0c2037

Also move GraphicsConfiguration search into AccessibilityGraphicsEnvironment

azuev-java approved these changes Jan 18, 2022

View reviewed changes

aivanov-jdk approved these changes Jan 18, 2022

View reviewed changes

openjdk Bot added the sponsor Pull request is ready to be sponsored label Jan 18, 2022

openjdk Bot added the integrated Pull request has been integrated label Jan 18, 2022

openjdk Bot closed this Jan 18, 2022

openjdk Bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jan 18, 2022

AlexeyTsvetkov mentioned this pull request Jun 18, 2022

Unsupported Java 8? JetBrains/compose-multiplatform#2112

Closed

mrserb mentioned this pull request Dec 12, 2022

8280359: Try to remove duplicate code for coordinate calculation openjdk/jdk#11626

Closed

3 tasks

Conversation

VeselovAlex commented Dec 24, 2021 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Dec 24, 2021

Uh oh!

openjdk Bot commented Dec 24, 2021

Uh oh!

mlbridge Bot commented Dec 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

openjdk Bot commented Dec 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aivanov-jdk commented Dec 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forantar Dec 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeselovAlex commented Dec 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeselovAlex commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

forantar commented Jan 17, 2022

Uh oh!

forantar commented Jan 17, 2022

Uh oh!

azuev-java left a comment

Choose a reason for hiding this comment

Uh oh!

forantar commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk Bot commented Jan 18, 2022

Uh oh!

VeselovAlex commented Jan 18, 2022

Uh oh!

openjdk Bot commented Jan 18, 2022

Uh oh!

forantar commented Jan 18, 2022

Uh oh!

openjdk Bot commented Jan 18, 2022

Uh oh!

openjdk Bot commented Jan 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

VeselovAlex commented Dec 24, 2021 •

edited by openjdk Bot

Loading

mlbridge Bot commented Dec 24, 2021 •

edited

Loading

openjdk Bot commented Dec 24, 2021 •

edited

Loading

forantar Dec 27, 2021 •

edited

Loading

VeselovAlex commented Jan 14, 2022 •

edited

Loading

forantar commented Jan 18, 2022 •

edited

Loading