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

Fix mouse tracking in Electron. #17225

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hwf1324
Copy link
Contributor

@hwf1324 hwf1324 commented Sep 27, 2024

Link to issue number:

Fixed #17108, #17190

Summary of the issue:

A fix was introduced in recent Electron applications that caused the mouse tracking to stay on a pane control that covered the entire window.
Unfortunately, since we can't fix it on Electron's part, we have to fix it in NVDA.

Description of user facing changes

Mouse tracking works in Electron applications.

Description of development approach

Use objectFromPointRedirect to redirect the object under the mouse to the correct object.

Testing strategy:

Manual testing, e.g. mouse tracking works correctly in VS COde 1.92 and higher.

Known issues with pull request:

None

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@coderabbitai summary

@hwf1324 hwf1324 requested a review from a team as a code owner September 27, 2024 04:44
@hwf1324
Copy link
Contributor Author

hwf1324 commented Sep 27, 2024

For code organization, is it necessary to create a separate Electron module?

Are there any optimization suggestions for determining object hits?

source/NVDAObjects/IAccessible/chromium.py Outdated Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
source/NVDAObjects/IAccessible/chromium.py Outdated Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

See test results for failed build of commit 54863ba794

Comment on lines +211 to +214
obj.role == controlTypes.Role.PANE
and obj.previous
and obj.previous.lastChild
and obj.previous.lastChild.windowClassName == "Chrome_RenderWidgetHostHWND"
Copy link
Member

Choose a reason for hiding this comment

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

this seems fragile and may include other documents unexpectedly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any suggestions?
Adding more constraints like before? Does this add more overhead?

I haven't encountered any problems with it in practice, at least so far.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally this would have an ID we could query to positively identify it.
Or even better, would just get fixed on the Electron side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#17108 (comment)

Unfortunately, I'm afraid that won't work.
In the comment pointed to by the link above, I mentioned that there are differences in the mouse objects that prevent the use of 'Chrome_WidgetWin_1' as a judgment condition.

@seanbudd seanbudd added the merge-early Merge Early in a developer cycle label Oct 4, 2024
@cary-rowen
Copy link
Contributor

BTW, this PR will also fix the issue of not being able to find the status bar in VS Code.

@cary-rowen
Copy link
Contributor

Hi @SaschaCowley @seanbudd
Is action required from the author before the next review?

@seanbudd
Copy link
Member

Is action required from the author before the next review?

We won't accept the current approach. We may come back later with suggested alternatives if we can think of any, but the current approach is too fragile and risky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-early Merge Early in a developer cycle
Projects
None yet
6 participants