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

When Windows 7 start menu closes and focus lands on a WorkerW pane redirect focus to the focused Desktop item. #10567

Merged
merged 13 commits into from Sep 16, 2020

Conversation

lukaszgo1
Copy link
Contributor

@lukaszgo1 lukaszgo1 commented Dec 2, 2019

Link to issue number:

none

Summary of the issue:

In #6672 we started ignoring focus events emitted by WorkerW to avoid useless "pane" announcements when moving to desktop. However on Windows 7 if start menu is opened from desktop and then closed focus ends up on this pane instead of focused desktop item.
For users this causes silence after start menu closes, it is also not very clear how to move somewhere else in this state - arrows are not functional on this strange pane.

Description of how this pull request fixes the issue:

Handling of WorkerW has been moved to a separate class in which we are ignoring focus events if there is another pending, and moving focus to the children (in this case desktop) if not.

Testing performed:

  1. Ensured that "Pane" is not announced when moving to desktop with Windows+d or Windows+m.
  2. Ensured that focus is properly moved to desktop in cases in which current Alpha reports nothing after start menu is closed.

Known issues with pull request:

None known

Change log entry:

None needed

@AppVeyorBot
Copy link

PR introduces Flake8 errors 😲

See test results for Failed build of commit 08177c10d3

@lukaszgo1
Copy link
Contributor Author

@michaelDCurran This is ready for a review when you have time.

@feerrenrut feerrenrut changed the title When Windows 7 start menu closes and focus lands on a WorkewW pane redirect focus to the focused Desktop item. When Windows 7 start menu closes and focus lands on a WorkerW pane redirect focus to the focused Desktop item. Apr 8, 2020
@feerrenrut
Copy link
Contributor

Please update the description to describe the user visible problem clearly. This reduces the overhead for a reviewer, it makes it faster to get up to speed on what this change intends to do. This is especially important when there is no issue describing the problem.

@lukaszgo1
Copy link
Contributor Author

@feerrenrut I hope the updated commend clears things up.

@feerrenrut feerrenrut added the app/windows-interface Interactions between NVDA and the default Windows GUI label Apr 8, 2020
@lukaszgo1
Copy link
Contributor Author

@feerrenrut I've updated the initial description of this PR to describe the issue as you've requested above. Is there something else which should be done to move this PR forward? The change is quite small - reviewing this should not take too long.

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Generally this looks good to me. I'd like @josephsl to review this since he developed the fix for #6671. @josephsl please let us know if you do not have time, and we'll take another approach.

Given the amount of variation in how the start menu behaves in different windows versions, some more details about the windows versions tested would help us to gain certainty.

Specifically, have you tested this on Windows 10?

If you are unable to test this yourself, then an alternative is to ask the community to test on different versions of windows. We just need to add a link to the build and add clear instructions on what to test
E.G.

"Minimize all apps with Win+d, press win key to open start menu, press win key to close start menu. NVDA should announce X not Y."

Be sure to cover your case and the steps to test #6671

@lukaszgo1
Copy link
Contributor Author

Hi @feerrenrut. I've tested this on Windows 10 2004 and while the initial approach required some modification to avoid reintroducing #6671 I've succeeded in fixing this. Then I've tested on my Server 2012 Vm which I have lying around and unfortunately #6671 is back under that Windows version. I've spend about an hour trying to fix this but without success. Given that Windows 8 and Windows Server 2012 aren't very widely used I don't want to spend more time trying to fix regressions from this PR under them. I've restricted the fix from this PR to Windows 7 where it works correctly. I've ensured that #6671 is not reintroduced on Windows 7, Server 2012 and Windows 10 2004. I've also ensured that after opening and closing start menu from the desktop focus returns to the focused desktop icon under all these Windows versions. I hope this is an acceptable solution.

josephsl
josephsl previously approved these changes Sep 15, 2020
Copy link
Collaborator

@josephsl josephsl left a comment

Choose a reason for hiding this comment

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

Tested and works correctly on Windows 7 (as described).

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Thanks for doing that testing @lukaszgo1 and for persisting with this PR. I think this is almost ready to go!

source/appModules/explorer.py Outdated Show resolved Hide resolved
source/appModules/explorer.py Outdated Show resolved Hide resolved
@lukaszgo1
Copy link
Contributor Author

@feerrenrut This is ready for another review.

@AppVeyorBot

This comment has been minimized.

feerrenrut
feerrenrut previously approved these changes Sep 16, 2020
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Thanks @lukaszgo1

@feerrenrut feerrenrut merged commit 0baf0ce into nvaccess:master Sep 16, 2020
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2020.4 milestone Sep 16, 2020
@lukaszgo1 lukaszgo1 deleted the WorkerWRedirectFocus branch September 16, 2020 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app/windows-interface Interactions between NVDA and the default Windows GUI enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants