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 issues with script propagation in Excel as well as tree interceptor passthrough on init #15136

Merged
merged 4 commits into from Jul 17, 2023

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Jul 12, 2023

Link to issue number:

Fixes #15131
Fixes #10612

Summary of the issue:

Recent changes to Excel support caused an issue where selection in the Excel cell edit control didn't work. When fixing that, I discovered another issue where initializing the Excel tree interceptor set the passThrough auto property to True, resulting into vision and braille focus set to the previous focus object.

Description of user facing changes

  1. Selection works again in cell edit controls
  2. When leaving a cell edit control, the name of the sheet is announced again, similar when coming from the formula bar.
  3. When an excel cell gets focus from outside, focus is no longer needlessly set to the previous focus object.

Description of development approach

This is another case of an issue that was obfuscated before #14984. There is a workaround in the Excel appModule to fallback on the UIA edit control if the edit control that gets focus when pressing f2 reports as unknown. However, the parent of the UIA control was set to the excel cell, not to the parent of the Excel6 window. This means that the changeSelection script still propagated, i.e. for every selection change related gesture in the cell edit control, NVDA would check whether a new cell was focused.
I changed the app module to set the parent of the UIA cell edit to the parent of the Excel6 window. This has the side effect of the sheet name being reported after leaving the edit control because the sheet receives focus again. This is however consistent with behavior prior to the app module workaround was applied, i.e. the time when we relied on the displayText to report the contents.
After I applied this change, I discovered that leaving the edit cell raised a COMError on an UIA object. This turned out to be an issue like this:

  1. api.setFocusObject was called for the new focus object (i.e. the worksheet)
  2. A new tree interceptor was created for the work sheet
  3. In the init method of the tree interceptor, self.passThrough was set, which is an auto property calling _set_passThrough on the tree interceptor.
  4. _set_passThrough called braille.handler.handleGainFocus and vision.handler.handleGainFocus for the current focus object, which was still the previous focus object (i.e. api.setFocusObject was not yet updated to the new focus).
  5. a COMError was raised because the UIA cell edit element was no longer available.

I fixed this by setting _passThrough instead, which sets the internal _passThrough boolean without applying the logic that updates focus for vision and braille, which is really not necessary and even harmful to do in init of a tree interceptor. I therefore applied the same change to another incarnation of this issue in the MSHTML virtual buffer.

Testing strategy:

STR from #15131. I'd like to encourage people to test #14799 and #10612 as well since I have good hope that those are fixed as well.

Known issues with pull request:

When leaving a cell edit control, the name of the sheet is announced again, similar when coming from the formula bar.

Change log entries:

Bug fixes

  • When landing on an Excel cell from outside a work sheet, braille and focus highlighter are no longer needlessly updated for the object that had focus previously.

Code Review Checklist:

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

@LeonarddeR LeonarddeR requested a review from a team as a code owner July 12, 2023 19:22
@LeonarddeR LeonarddeR requested a review from seanbudd July 12, 2023 19:22
@seanbudd
Copy link
Member

This PR is currently set to close all 3 issues when merged. Would creating a try-build help confirm that this PR closes all 3 issues?

@cary-rowen
Copy link
Contributor

Hi,

After my testing #15131 can be fixed by this PR.
@Adriani90 and @aaclause can you guys test #14799 and #10612 with builds from this PR?

Thanks

@AppVeyorBot
Copy link

See test results for failed build of commit 30e005b84b

@cary-rowen
Copy link
Contributor

The current CI build is failing, can you try to rebuild a version?

@seanbudd
Copy link
Member

The failure is unrelated, the downloadable build should still work

@Adriani90
Copy link
Collaborator

Thanks @LeonarddeR for the work on this one.

  1. As for MS Excel: NVDA freezes or becomes really slow when selecting cells with arrow key while directly editing in cells is enabled #14799, NVDA does not crash anymore, but it is still slow when selecting a lot of cells as described in that issue. I think it might have to do with the focus being out of the screen when it moves to a selected cell which is not visible. I think we would need some assistance from a sighted person to explain what exactly happens with the focus in MS Excel: NVDA freezes or becomes really slow when selecting cells with arrow key while directly editing in cells is enabled #14799. When selecting many cells, after a while NVDA does not report the coodinates at all anymore.

  2. I cannot reproduce Excel navigation causing _ctypes.COMError #10612 with this build.

  3. As for Regression: Error performing some gestures in Excel cell edit control #15131, the selection in the cell works now again, however NVDA does not report what is selected when pressing shift+arrow keys or ctrl+shift+arrow keys.

@LeonarddeR
Copy link
Collaborator Author

3. As for [Regression:  Error performing some gestures in Excel cell edit control #15131](https://github.com/nvaccess/nvda/issues/15131), the selection in the cell works now again, however NVDA does not report what is selected when pressing shift+arrow keys or ctrl+shift+arrow keys.

This issue is strictly spoken unrelated. It probably has to do with UIA being forcefully disabled in Excel, causing text change events not coming through. Though I can't reproduce when selective registration is off.

@seanbudd seanbudd merged commit 08de8de into nvaccess:master Jul 17, 2023
1 check was pending
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants