Skip to content
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

8291087: Wrong position of focus of screen reader on Windows with screen scale > 1 #853

Closed

Conversation

kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jul 28, 2022

When using the Windows Narrator screen reader on a Hi-DPI screen, the visible focus is drawn in the wrong location and with the wrong size. This happens because the the JavaFX Windows accessibility code that returns the screen bounds of the requested UI element does not take the screen scale into account.

The fix is to adjust the bounds by the screen scale before returning it.

You can test it on a Windows machine with scaling set to >= 125% as follows:

  1. Turn on Windows Narrator
  2. Run any JavaFX program with at least UI controls, for example, hello.HelloTextField in apps/toys/Hello
  3. TAB between the controls

Without the fix, Narrator shows the focus indicator in the wrong position and is too small. With the fix, it is correct. While testing this, I discovered an unrelated (and preexisting) bug where the focus indicator for a TextField or TextArea whose content is larger that the control (and is displayed with scroll bars) is not clipped to the visible area. This happens regardless of screen scale. I will file a follow-up bug for this.

Note that this bug is specific to Windows. It does not occur on macOS, which works correctly on a retina or non-retina display.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8291087: Wrong position of focus of screen reader on Windows with screen scale > 1

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 853

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

Using diff file

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

@kevinrushforth
Copy link
Member Author

/reviewers 2

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 28, 2022

👋 Welcome back kcr! 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 openjdk bot added the rfr Ready for review label Jul 28, 2022
@openjdk
Copy link

openjdk bot commented Jul 28, 2022

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

@mlbridge
Copy link

mlbridge bot commented Jul 28, 2022

Webrevs

@andy-goryachev-oracle
Copy link
Contributor

just curious, does it work when the control spans two monitors with different scales?
when dragging the enclosing window from one monitor to another?

@kevinrushforth
Copy link
Member Author

I did some limited testing yesterday with two screens each of which had a different scale, and what I tested works as expected.

Copy link
Collaborator

@aghaisas aghaisas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on Windows with 100%, 125%. 150% and 175% scales. I confirm that this fixes the focus rendering problem seen with the screen reader being ON.

Copy link
Member

@arapte arapte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Tested with different scale factors on one screen only.

@openjdk
Copy link

openjdk bot commented Jul 29, 2022

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

8291087: Wrong position of focus of screen reader on Windows with screen scale > 1

Reviewed-by: aghaisas, arapte

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

  • b4d86bd: 8218826: TableRowSkinBase: horizontal layout broken if row has padding
  • 4c1f4c2: Merge
  • dd30402: 8291589: Update copyright header for files modified in 2022
  • 779365f: Merge
  • 5febaca: 8291502: Mouse or touch presses on a non-focusable region don't clear the focusVisible flag of the current focus owner
  • 8fa8919: 8291588: Update boot JDK to 18.0.2
  • 08ec9c8: 8289605: Robot capture tests fail intermittently on Mac M1
  • 4b4deae: 8289397: Fix warnings: Possible accidental assignment in place of a comparison. A condition expression should not be reduced to an assignment
  • 89d1cf2: 8290530: Bump minimum JDK version for JavaFX to JDK 17
  • 0b74ee8: 8290990: Clear .root style class from a root node that is removed from a Scene/SubScene
  • ... and 1 more: https://git.openjdk.org/jfx/compare/49a2efbbfba017101e5f57b590bf820307863bc3...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.

@openjdk openjdk bot added the ready Ready to be integrated label Jul 29, 2022
@kevinrushforth
Copy link
Member Author

While testing this, I discovered an unrelated (and preexisting) bug where the focus indicator for a TextField or TextArea whose content is larger that the control (and is displayed with scroll bars) is not clipped to the visible area. This happens regardless of screen scale. I will file a follow-up bug for this.

I filed JDK-8291590 to track this.

@kevinrushforth
Copy link
Member Author

I did a little more multi-screen testing with different layouts, and saw some odd behavior on the secondary screen that I want to track down and understand better. It look like a different problem, but I want to be sure before I integrate this. If it is a different problem, I'll file another follow-on bug.

@kevinrushforth
Copy link
Member Author

Getting back to this, I discovered what the problem was with some multi-screens layouts when two screens have a different scale. Computing the platform x and y by simply multiplying by the scale is not right, and only work for a screen that touches the origin of the virtual screen space (in both X and Y). When the secondary screen is to the left or above the primary screen, the simple (but incorrect) calculation gives the same result as the correct calculation. For a secondary screen below or to the right of the primary screen, it doesn't. Simply scaling the coordinate about the origin produces a value that is either too small or too large when the two screens have a different scale.

The right fix is to use the Screen.toPlatformX and Screen.toPlatformY methods, which take this into account, and translates the coordinate to the right origin, scales it, and then translates it back. In order to do that, I needed to get the Window from which I could get the Screen, which required making the quantum WindowStage class public along with its getWindow method. This is an internal implementation class, so is not a concern.

I've tested this updated version in various combinations of layout and screen scales, and everything now works as expected.

@openjdk openjdk bot removed the ready Ready to be integrated label Aug 5, 2022
@andy-goryachev-oracle
Copy link
Contributor

@kevinrushforth :
on windows, a window can be positioned to span two monitors. you can't do this on Mac - once dropped, only a part of the window remains (on the monitor which contains the title bar, it seems). but on windows, the app continues to work on both monitors. what happens then?

@kevinrushforth
Copy link
Member Author

Much of my testing was done with a window straddling two screens. It works as expected in all cases I checked. If a single control spans both screens, the part that is on each screen is correctly highlighted by the Windows Narrator visible focus.

Note that in this mode, the window is considered to be "on" one of the screens, and all scales are relative to that screen (not just for accessibility, this is how HiDPI works on Windows if you are multi-monitor HiDPI aware).

Copy link
Collaborator

@aghaisas aghaisas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks good!

@openjdk openjdk bot added the ready Ready to be integrated label Aug 9, 2022
@kevinrushforth
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Aug 9, 2022

Going to push as commit 38324a7.
Since your change was applied there have been 11 commits pushed to the master branch:

  • b4d86bd: 8218826: TableRowSkinBase: horizontal layout broken if row has padding
  • 4c1f4c2: Merge
  • dd30402: 8291589: Update copyright header for files modified in 2022
  • 779365f: Merge
  • 5febaca: 8291502: Mouse or touch presses on a non-focusable region don't clear the focusVisible flag of the current focus owner
  • 8fa8919: 8291588: Update boot JDK to 18.0.2
  • 08ec9c8: 8289605: Robot capture tests fail intermittently on Mac M1
  • 4b4deae: 8289397: Fix warnings: Possible accidental assignment in place of a comparison. A condition expression should not be reduced to an assignment
  • 89d1cf2: 8290530: Bump minimum JDK version for JavaFX to JDK 17
  • 0b74ee8: 8290990: Clear .root style class from a root node that is removed from a Scene/SubScene
  • ... and 1 more: https://git.openjdk.org/jfx/compare/49a2efbbfba017101e5f57b590bf820307863bc3...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Aug 9, 2022
@openjdk openjdk bot closed this Aug 9, 2022
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Aug 9, 2022
@openjdk
Copy link

openjdk bot commented Aug 9, 2022

@kevinrushforth Pushed as commit 38324a7.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants