Stop review position errors from crashing the magnifier

[ChristosBouronikos]

Link to issue number:

Fixes #20860

Follow-up to #20897.

Note

This PR has been updated following review feedback. I have only done limited testing so far: unit tests and some quick manual checks. It has not been tested extensively on Windows, especially the Windows Terminal and Firefox cases below. Testing and feedback are very welcome.

Relationship to #20897:

#20897 was merged as a partial fix for this issue. It changed _MAX_RECOVERY_ATTEMPTS in fullscreenMagnifier.py from 3 to 1, which reduces the number of screen flashes during recovery. It does not touch the error that starts the recovery.

This PR addresses the error itself. A failure to get a tracked position no longer reaches the recovery path at all, so there is no recovery, no flash, and Magnifier stays active. Genuine Magnification API failures still go through the recovery path, still limited to one attempt by #20897.

Summary of the issue:

Magnifier flickers and then stops itself in situations unrelated to the Magnification API, for example when opening some browseable messages (ui.browseableMessage("<p>𝔓 power set</p>", isHtml=True)), when closing a Firefox tab, or when switching to Windows Terminal.

In the logs on the issue, getting the review position raises a COMError. Magnifier._updateMagnifier treated this as a Magnification API failure, and after three such errors attempted recovery (the screen flash) and then stopped.

The logs show two different underlying retrieval failures:

1. MSHTML (browseable message with "𝔓"): MSHTMLTextInfo.__init__ fails in MoveToBookmark when moving a new BODY text range to the bookmark of the current selection (COMError: Paramètre incorrect). The same error also breaks the caret event for that object.
2. UIA (Windows Terminal): UIATextInfo.__init__ fails in collapse() when collapsing the selection to the caret (COMError: Erreur non spécifiée). This happened inside api.setNavigatorObject → review.setCurrentMode, so it also aborted the gainFocus event.

There is no log for the Firefox tab case.

Description of user facing changes:

• Magnifier no longer flickers or stops when NVDA cannot get the review cursor position. The magnified view and colour filter stay active, and the view stays at its current position until tracking works again.
• The review cursor should now work in HTML messages containing characters such as "𝔓".
• NVDA should no longer fail to handle the focus when switching to Windows Terminal in the situation from the log.

Description of developer facing changes:

• New exceptions.LocationUnavailableError, a subclass of LookupError, raised when a cursor position or its screen location cannot currently be retrieved. The underlying error is available as __cause__.
• API breaking: api.getReviewPosition now raises LocationUnavailableError when the review position cannot be retrieved, instead of the underlying error (COMError, RuntimeError, etc.). A failed retrieval is not cached, so a later call retries. This is documented in its docstring.
• API breaking: vision.util.getContextRect now raises LocationUnavailableError for retrieval failures instead of the underlying error.
• API breaking: review.setCurrentMode(..., updateReviewPosition=False) no longer fetches a review position, so the mode is set without checking that a position is available. Internally this is only used during focus and caret handling, where the fetched position was discarded anyway.

Existing callers that catch LookupError around getReviewPosition (for example in NVDAObjects/__init__.py and globalCommands.py) keep working, since the new exception is a LookupError.

Description of development approach:

Following @CyrilleB79's suggestion, review position errors are now reported with a single error type, and both visual consumers handle only that type:

• api.getReviewPosition: known retrieval errors (COMError, LookupError, OSError, RuntimeError) are raised as LocationUnavailableError with the original error as the cause. Programming errors and CallCancelled still propagate unchanged.
• Magnifier:
  • FocusManager._getReviewPosition now only catches LocationUnavailableError. _getPointAtStart also raises it when no screen point can be found.
  • In Magnifier._updateMagnifier, tracking is now separated from _doUpdate. Only Magnification API errors from _doUpdate count towards recovery, so no tracking error can trigger the recovery flash, including ones we don't know about yet (such as the Firefox case).
  • The update timer is always rescheduled. Previously, an unexpected exception (for example a CallCancelled during an NVDA freeze) stopped the timer and tracking never resumed.
• NVDA Highlighter: getContextRect raises LocationUnavailableError for retrieval failures, and updateContextRect catches it instead of LookupError/RuntimeError. It still also catches ValueError/TypeError, which come from locationHelper rejecting invalid rectangles rather than from retrieval. Previously a COMError from a caret or review lookup was not handled at all.

The two retrieval failures from the logs are fixed at their source:

• MSHTML: for a non-editable BODY, the selection range is used directly for the caret and selection positions. The BODY range covers the whole document, so moving it to the selection's bookmark is unnecessary, and that is the call that fails. Editable bodies already used the selection range directly. Other elements, such as inputs, still use the bookmark, since their range is limited to the element.
• UIA: a COMError from collapse() is now raised as RuntimeError, the same as the existing handling for a failing GetSelection(). Callers already handle that, for example object review falls back to the first position.
  • Trade-off: if the caret is unavailable when the object gains focus, the review cursor starts at the beginning of the text until the caret moves. This matches existing behaviour when GetSelection() fails. I preferred this to a new exception type, because many existing callers only catch RuntimeError here (focus speech, caret movement detection, several global commands), and would otherwise start raising errors.
• Focus handling: review.setCurrentMode no longer fetches a position it discards, and api.setNavigatorObject no longer aborts the focus event if getting the caret for the review position fails. The review position is then fetched again when needed.

Testing strategy:

Unit tests:

• test_api.py: getReviewPosition raises LocationUnavailableError with the original cause, does not cache a failure, and recovers on the next call. Unexpected errors and CallCancelled still propagate. setNavigatorObject still completes when getting the caret fails.
• test_review.py: setCurrentMode(..., updateReviewPosition=False) does not fetch a position. The default still checks availability.
• test_NVDAObjects_IAccessible_MSHTML.py: the BODY caret and selection use the selection range without MoveToBookmark. Inactive elements are still rejected, and other elements still use the bookmark.
• test_NVDAObjects_UIA.py: a COMError from collapse() raises RuntimeError, and object review falls back to the first position.
• test_visionUtil.py and test_visionEnhancementProviders/test_NVDAHighlighter.py: retrieval errors from getContextRect are raised as LocationUnavailableError, and the highlighter clears the highlight instead of raising.
• test_magnifier/test_magnifier.py and test_magnifier/test_focusManager.py: an unavailable tracked location keeps the current view without counting as an error. Tracking errors never trigger recovery. The timer keeps running.

The new tests for each fix fail without it.

Manual testing has been limited so far. Not yet tested extensively. Suggested manual tests:

1. Enable Magnifier with inverted colours. In the NVDA Python console, run import ui;ui.browseableMessage("<p>𝔓 power set</p>", isHtml=True).
   • Magnifier should stay active without flickering.
   • The log should no longer contain "Error updating magnifier", "Attempting full-screen magnifier recovery" or an error for the caret event.
   • Review cursor commands should read the text.
2. Switch to Windows Terminal several times with Alt+Tab. There should be no "error executing event: gainFocus" in the log, and Magnifier should keep tracking.
3. Close Firefox tabs repeatedly with Magnifier enabled. Magnifier should not flicker or stop.
4. Check that Visual Highlight still follows focus, the navigator object and the browse mode cursor.

Known issues with pull request:

• Not tested extensively yet, see above.
• There is no log for the Firefox tab case, so its underlying cause is not known. Magnifier no longer flickers or stops in this case either way. If an error still appears in the log, it should probably be tracked in a new issue.
• vision.util.getRectFromTextInfo expands the passed TextInfo in place, which can modify the cached review position. This is a pre-existing issue, left for a separate PR.

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.

[seanbudd]

Can this please be retargetted to master?

[ChristosBouronikos]

Can this please be retargetted to master?

@seanbudd changed target to master branch.

[CyrilleB79]

According to its description, this PR definitely seems to bring an improvement with regards to situations where review cursor retrieval raises errors: instead of crashing the magnifier, tracking is just temporarily not honoured. So if it works as advertised, I have not tested myself, it definitely deserves to be merged.

Though, as I understand it, the root tracking issue still stands, for magnifier, but maybe also for visual highlighter of browse mode cursor. _getReviewPosition still raises an error which remains unexpected (and undocumented in its docstring). IMO, even if lower priority, such caret / focus / review cursor retrieval errors should still have their issues remaining open and ideally be fixed. So if this PR closes #20860, the remaining tracking issue, if applicable, should be documented in a new issue.

[CyrilleB79]

Maybe ideally, all possible / known errors when retrieving review cursor should be catched in getReviewPosition and _getReviewPosition should raise a single error type when it fails so that Magnifier and visual highlighter only catch this error type.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[ChristosBouronikos]

@CyrilleB79 thanks for the review. I've updated the PR to address both of your comments. The description has the full details, but in short:

A single error type

• api.getReviewPosition now raises a new exceptions.LocationUnavailableError (a LookupError subclass) when the review position can't be retrieved. The original error is kept as the cause, and this is documented in the docstring.
• vision.util.getContextRect does the same for highlight locations.
• Magnifier and the NVDA Highlighter now only catch this error type for retrieval failures.
• In Magnifier._updateMagnifier, tracking is now separated from the Magnification API update. Only Magnification API errors count towards recovery, so a tracking error can no longer cause the flash and shutdown, including errors we don't know about yet.

The underlying tracking errors

I tried to fix the two failures from the logs at their source:

• Browseable message with "𝔓" (MSHTML): the error came from MoveToBookmark, when moving a new BODY range to the selection's bookmark. For a non-editable BODY, the selection range is now used directly, so that call is no longer made.
• Windows Terminal (UIA): collapse() failed with a COMError inside setNavigatorObject → setCurrentMode, which also aborted the gainFocus event. That error is now raised as RuntimeError, the same as an existing GetSelection() failure, so callers fall back as they already do. Also, setCurrentMode(..., updateReviewPosition=False) no longer fetches a position it would discard.

There's no log for the Firefox tab case, so I don't know its underlying cause. Magnifier should no longer flicker or stop there either way. If you still see an error in the log when closing tabs, a log would be very helpful, and it could be tracked in a new issue.

Please note: I haven't tested this extensively yet, only unit tests and some quick manual checks. In particular, I haven't properly tested the Windows Terminal and Firefox cases. If you could try your original steps to reproduce, that would be much appreciated.

[CyrilleB79]

@ChristosBouronikos regarding Firefox issue or the other one when alt+tabbing, I have no reproducible STR. It only happened from time to time (or even only once.
By the way, regarding the issue which occurred when alt+tabbing, I do not even remember if I was alt+tabbing to or from Windows Terminal.

[seanbudd]

marking as draft until testing is performed