Skip to content

Update magnifier Point at start#19780

Open
Boumtchack wants to merge 8 commits intonvaccess:masterfrom
France-Travail:fix/NVDAPythonConsole
Open

Update magnifier Point at start#19780
Boumtchack wants to merge 8 commits intonvaccess:masterfrom
France-Travail:fix/NVDAPythonConsole

Conversation

@Boumtchack
Copy link
Copy Markdown
Contributor

@Boumtchack Boumtchack commented Mar 11, 2026

Link to issue number:

fixes #19746

Summary of the issue:

Text caret follow would fail in some text editor (python console of nvda or notepad), if it would be the last character

Description of user facing changes:

text caret will now focus properly end character

Description of developer facing changes:

Description of development approach:

Update _get_pointAtStart now falls back to the previous character's position when the caret is at end-of-text and both _getBoundingRectFromOffset and _getPointFromOffset fail for the current offset.

Testing strategy:

Manual

Known issues with pull request:

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.

@Boumtchack Boumtchack marked this pull request as ready for review March 11, 2026 13:46
@Boumtchack Boumtchack requested a review from a team as a code owner March 11, 2026 13:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes magnifier caret-follow failures when the caret is positioned at end-of-text by improving how OffsetsTextInfo.pointAtStart is computed when the current offset can’t be mapped to screen coordinates.

Changes:

  • Extend _get_pointAtStart to handle LookupError as well as NotImplementedError when resolving a point for the current offset.
  • Add a fallback for collapsed ranges to use the previous character’s offset when the caret is at end-of-text and current-offset location lookup fails.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@CyrilleB79
Copy link
Copy Markdown
Contributor

I am not an expert of this code.

But I wonder if it would not be better, if possible, to implement the fallback in magnifier files rather than in offset.py. Indeed an offset function used elsewhere returning an erroneous information could have side effect; couldn't it?

@LeonarddeR
Copy link
Copy Markdown
Collaborator

I think I agree with @CyrilleB79 here.
Note that the end offset is exclusive in text controls. So when you have a text control containing one character, that one character is at offset 0. The story length is 1 and collapsing at end is offset 1, yet offset 1 is exclusive to the text.
What would make sense to me though is returning the top right of the rectangle at the previous offset.

@seanbudd seanbudd marked this pull request as draft March 16, 2026 05:49
@seanbudd seanbudd changed the title Update Point at start Update magnifier Point at start Mar 17, 2026
@seanbudd seanbudd added the merge-early Merge Early in a developer cycle label Mar 17, 2026
@seanbudd seanbudd self-requested a review March 24, 2026 04:25
@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 24, 2026
@seanbudd
Copy link
Copy Markdown
Member

Is this ready for re-review?

@Boumtchack Boumtchack marked this pull request as ready for review March 30, 2026 13:48
@seanbudd seanbudd marked this pull request as draft March 31, 2026 02:12
@Boumtchack Boumtchack marked this pull request as ready for review March 31, 2026 12:18
Comment on lines +194 to +195
if not (isinstance(textInfo, OffsetsTextInfo) and textInfo.isCollapsed and textInfo._startOffset > 0):
raise originalExc
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.

I think it would be cleaner to just indent this into the except block. It will make reading the logs when it's raised more obvious too

return textInfo.pointAtStart
except (NotImplementedError, LookupError, AttributeError):
pass
except (NotImplementedError, LookupError, AttributeError) as e:
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.

can we debug log this error?

Comment on lines +201 to +205
try:
return textInfo._getPointFromOffset(prevOffset)
except (NotImplementedError, LookupError, AttributeError):
pass
raise originalExc
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.

Suggested change
try:
return textInfo._getPointFromOffset(prevOffset)
except (NotImplementedError, LookupError, AttributeError):
pass
raise originalExc
log.debug(...)
try:
return textInfo._getPointFromOffset(prevOffset)
except (NotImplementedError, LookupError, AttributeError) as e:
log.debug(...)
raise e

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kept both fallbacks for better precision and added debug logging to each. The final raise re-raises the original pointAtStart error, which is the most useful for debugging.

@seanbudd seanbudd marked this pull request as draft April 1, 2026 02:57
This was referenced Apr 7, 2026
@Boumtchack Boumtchack marked this pull request as ready for review April 14, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. merge-early Merge Early in a developer cycle

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Magnifier - Text caret is not followed in NVDA Python console

6 participants