Skip to content

Fix Screen Curtain for Windows 10/11 insiders#12684

Merged
seanbudd merged 11 commits into
betafrom
disableScreenCurtainOnUntestedWindows
Jul 29, 2021
Merged

Fix Screen Curtain for Windows 10/11 insiders#12684
seanbudd merged 11 commits into
betafrom
disableScreenCurtainOnUntestedWindows

Conversation

@seanbudd

@seanbudd seanbudd commented Jul 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

@michaelDCurran michaelDCurran left a comment

Copy link
Copy Markdown
Member

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.

Comment thread user_docs/en/changes.t2t Outdated
@LeonarddeR

LeonarddeR commented Jul 26, 2021

Copy link
Copy Markdown
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

seanbudd commented Jul 26, 2021

Copy link
Copy Markdown
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
Comment thread user_docs/en/changes.t2t Outdated
Comment thread user_docs/en/userGuide.t2t Outdated
@Brian1Gaff

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

Copy link
Copy Markdown

lukaszgo1
lukaszgo1 reviewed Jul 26, 2021
View reviewed changes
Comment thread source/winVersion.py Outdated
@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

AppVeyorBot commented Jul 27, 2021

Copy link
Copy Markdown

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
michaelDCurran
michaelDCurran reviewed Jul 28, 2021
View reviewed changes
Comment thread user_docs/en/changes.t2t Outdated
@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
Comment thread source/visionEnhancementProviders/screenCurtain.py Outdated
feerrenrut
feerrenrut reviewed Jul 28, 2021
View reviewed changes
Comment thread source/winVersion.py Outdated
feerrenrut
feerrenrut reviewed Jul 28, 2021
View reviewed changes
Comment thread source/winVersion.py
@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

@michaelDCurran michaelDCurran michaelDCurran left review comments

@Qchristensen Qchristensen Awaiting requested review from Qchristensen

+2 more reviewers

@lukaszgo1 lukaszgo1 lukaszgo1 left review comments

@feerrenrut feerrenrut feerrenrut approved these changes

Reviewers whose approvals may not affect merge requirements

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