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.

Sign up for GitHub

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

Jump to bottom

Fix Screen Curtain for Windows 10/11 insiders #12684

Merged
merged 11 commits into from
Jul 29, 2021
Merged

Fix Screen Curtain for Windows 10/11 insiders #12684

merged 11 commits into from
Jul 29, 2021

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jul 26, 2021 •
edited
Loading

Link to issue number:

Fix for #12491

Summary of the issue:

Screen curtain does not work as intended for new Windows builds. This is due to an API change. Previously, NVDA used a fully transparent colour transformation to create the screen curtain, which now no longer results in a black screen.

In the future, the API is unstable: "The Magnification API is not supported under WOW64; that is, a 32-bit magnifier application will not run correctly on 64-bit Windows.".

Description of how this pull request fixes the issue:

Implements the screen curtain with a fully solid opacity.
Adds a warning to test newer Windows builds when using screen curtain.

Testing strategy:

  • Test starting screencurtain using this PR build on a Windows 10 insiders build
  • Test starting screencurtain using this PR build on a Windows 11 insiders build
  • Test starting screencurtain using this PR build on a Windows 10 stable build
  • Test starting screencurtain using this PR build on a Windows 8 stable build

Known issues with pull request:

This may occur again in the future due to the API not being fully supported, or another undefined behaviour change like this one. It would be good to add a test that confirms the screen is black using an independent tool, and/or move towards a different API.

Change log entries:

See PR

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual testing.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers

@seanbudd seanbudd requested a review from a team as a code owner July 26, 2021 03:05
@seanbudd seanbudd requested review from michaelDCurran and removed request for a team July 26, 2021 03:05
@seanbudd seanbudd self-assigned this Jul 26, 2021
@seanbudd seanbudd marked this pull request as draft July 26, 2021 03:05
@seanbudd seanbudd requested a review from Qchristensen July 26, 2021 03:06
michaelDCurran
michaelDCurran requested changes Jul 26, 2021
View reviewed changes
Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

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

A similar note to the one in changes.t2t should be added to the user Guide in the Screen Curtain section.

user_docs/en/changes.t2t Outdated Show resolved Hide resolved
@LeonarddeR
Copy link
Collaborator

"The Magnification API is not supported under WOW64; that is, a 32-bit magnifier application will not run correctly on 64-bit Windows.".

It might be interesting to prove this point by testing the magnification API on X64 Python.

Also I wonder whether we can system test this. If the screen curtain is active, the screen is black, so taking a screen bitmap would only reveal blackness.

@seanbudd seanbudd force-pushed the disableScreenCurtainOnUntestedWindows branch from 04677d2 to fe657a0 Compare July 26, 2021 05:40
@seanbudd seanbudd marked this pull request as ready for review July 26, 2021 05:42
@seanbudd seanbudd requested a review from a team as a code owner July 26, 2021 05:42
@seanbudd seanbudd requested a review from michaelDCurran July 26, 2021 05:42
@seanbudd
Copy link
Member Author

It might be interesting to prove this point by testing the magnification API on X64 Python.

Currently working on creating minimal examples of this in Python and C++ to test as WOW64 vs 64bit interprets and compliers. It's possible that the issue is being introduced elsewhere or for different reasons.

Also I wonder whether we can system test this. If the screen curtain is active, the screen is black, so taking a screen bitmap would only reveal blackness.

Possibly, though AppVeyor would only be testing a certain Windows build of course. I would be looking to include something to automate testing this with an accompanying fix.

@seanbudd seanbudd changed the title Disable Screen Curtain for untested version of Windows Disable Screen Curtain for untested versions of Windows Jul 26, 2021
michaelDCurran
michaelDCurran previously approved these changes Jul 26, 2021
View reviewed changes
seanbudd
seanbudd commented Jul 26, 2021
View reviewed changes
user_docs/en/changes.t2t Outdated Show resolved Hide resolved
user_docs/en/userGuide.t2t Outdated Show resolved Hide resolved
@Brian1Gaff
Copy link

Brian1Gaff commented Jul 26, 2021 via email •
edited by feerrenrut
Loading

lukaszgo1
lukaszgo1 reviewed Jul 26, 2021
View reviewed changes
source/winVersion.py Outdated Show resolved Hide resolved
@seanbudd seanbudd marked this pull request as draft July 27, 2021 01:39
@seanbudd seanbudd changed the title Disable Screen Curtain for untested versions of Windows Fix Screen Curtain for Windows 10/11 insiders Jul 27, 2021
@seanbudd seanbudd force-pushed the disableScreenCurtainOnUntestedWindows branch from 8328487 to 01d9be5 Compare July 27, 2021 05:37
@seanbudd seanbudd marked this pull request as ready for review July 27, 2021 05:41
@AppVeyorBot
Copy link

See test results for failed build of commit 282d875965

@seanbudd seanbudd linked an issue Jul 28, 2021 that may be closed by this pull request
Screen curtain is not working on newer windows insider builds #12491
Closed
@seanbudd seanbudd requested a review from michaelDCurran July 28, 2021 00:06
@seanbudd seanbudd force-pushed the disableScreenCurtainOnUntestedWindows branch from 43dc90e to b3336d1 Compare July 28, 2021 04:36
michaelDCurran
michaelDCurran previously approved these changes Jul 28, 2021
View reviewed changes
feerrenrut
feerrenrut reviewed Jul 28, 2021
View reviewed changes
source/winVersion.py Outdated Show resolved Hide resolved
feerrenrut
feerrenrut reviewed Jul 28, 2021
View reviewed changes
source/winVersion.py Outdated Show resolved Hide resolved
@seanbudd seanbudd requested a review from feerrenrut July 29, 2021 06:04
@seanbudd seanbudd force-pushed the disableScreenCurtainOnUntestedWindows branch from 7230804 to e72295f Compare July 29, 2021 06:11
@seanbudd seanbudd force-pushed the disableScreenCurtainOnUntestedWindows branch from e72295f to 085bbd6 Compare July 29, 2021 06:12
@seanbudd seanbudd merged commit bcfd398 into beta Jul 29, 2021
@seanbudd seanbudd deleted the disableScreenCurtainOnUntestedWindows branch July 29, 2021 07:50
@nvaccessAuto nvaccessAuto added this to the 2021.3 milestone Jul 29, 2021
@seanbudd seanbudd modified the milestones: 2021.3, 2021.2 Jul 29, 2021
@feerrenrut feerrenrut mentioned this pull request Aug 2, 2021
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukaszgo1 lukaszgo1 lukaszgo1 left review comments

@feerrenrut feerrenrut feerrenrut approved these changes

@michaelDCurran michaelDCurran michaelDCurran left review comments

@Qchristensen Qchristensen Awaiting requested review from Qchristensen

Assignees

@seanbudd seanbudd

Labels
None yet
Projects
None yet
Milestone
2021.2
Development

Successfully merging this pull request may close these issues.

Screen curtain is not working on newer windows insider builds
8 participants
@seanbudd @LeonarddeR @Brian1Gaff @AppVeyorBot @feerrenrut @lukaszgo1 @michaelDCurran @nvaccessAuto