Fix OffsetTextInfo.collapse() and OffsetTextInfo.move() by mltony · Pull Request #18348 · nvaccess/nvda

mltony · 2025-06-26T22:35:12Z

Link to issue number:

This is my second attempt to fix it without breaking #17430. For the reference, the first attempt was #18338, which is abandoned, and which didn't fix the root cause, but just decoupled @nvdaes's commit from TextInfo API. This PR however fixes the root cause of #17430.

Summary of the issue:

Commit 70cd311 by @nvdaes introduced a bug in TextInfo API: TextInfo.collapse() now unnecessarily jumps to the next paragraph in Notepad++. This bug also made its way into 2025.1 release.

I investigated the braille issue #17430 further and it boiled down to an issue with OffsetTextInfo.move() implementation: it turns out that unless it moves by character, it is unable to move to the very last position in the document. Fixing OffsetTextInfo.move() implementation.

Description of user facing changes:

N/A

Description of developer facing changes:

OffsetTextInfo.collapse() now works correctly in Notepad++.
OffsetTextInfo.move() can now move to the very last position even when unit is set to line or paragraph.

Description of development approach:

In OffsetTextInfo.move() modified the condition when self.allowMoveToOffsetPastEnd to increase highLimit regardless of current unit - previously this would only allow when unit is character.
Removed ScintillaTextInfo.expand() override.

Testing strategy:

Tested with use case provided in TextInfo.collapse(end=True) moves textInfo to the next paragraph in Notepad++ #18320.
Tested that OffsetTextInfo.move('line', 1) can now successfully go to the very last line in Notepad++ even if the last line is empty. This should be the proper fix for Notepad++: braille doesn't move to the next line if last line is empty #17430.

Known issues with pull request:

N/A

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

AppVeyorBot · 2025-06-26T23:27:07Z

PASS: Translation comments check.
PASS: License check.
PASS: Unit tests.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/u6s5een6qgp32qok/artifacts/output/nvda_snapshot_pr18348-37028,e48645fa.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.5,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 20.3,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 20.7,
FINISH_END 0.1

See test results for failed build of commit e48645facd

mltony · 2025-06-27T04:43:47Z

@nvdaes, could you test that this doesn't break #17430?
Also, cc: @LeonarddeR

nvdaes · 2025-06-27T04:54:27Z

Thanks @mltony . This works for me as expected in Notepad++ with braille when moving to the next line, if last line is empty.

mltony · 2025-06-27T05:11:38Z

Thanks @nvdaes!
Also regarding fixing the unit test. I haven't quite figured out why chromeTests.test_pr11606 started to fail. For some reason this PR changes speech after one of the keystrokes.
However the new speech makes more sense to me. In this test when end key is pressed it seems to me that the speech should be "blank" rather then "link" since this keystroke takes the cursor past the last link in the line. So the original behavior seems to be incorrect - even though I don't fully understand how did this PR fix it.
So at this point I just changed expected speech in the test and now it passes.

LeonarddeR · 2025-06-27T06:02:36Z

This is an interesting find. I'm afraid though that this implementation might report nonexisting blank lines when moving by line with the review cursor. I guess this clarifies why the chrome test behavior differs now.
A fix should be a bit less generalised.

Also note that the comment is no longer accurate now. It says: #: move with unit_character can move 1 past story length to allow braille routing to end insertion point. (#2096).

I agree though that what you said in an earlier comment in the previous pr is true, namely that the last empty line of a document has a length of 0. Treating that as 1 won't work due to the highLimit in the move method.

Note also that The story length of an empty notepad classic document (i.e. a default EditTextInfo) is 1, not 0. It uses a + 1 trick, though there is an ambiguous comment saying that an off by one error is created by the + 1. I think this means no more than that this trick causes the story length to be reported one higher than the actual length. It seems that EditTextInfo doesn't treat line breaks as actual characters.

A quick fix would be doing _getStoryLength + 1, but this introduces the same ambiguity as in EditTextInfo.

mltony · 2025-06-27T06:14:31Z

@LeonarddeR,

I'm afraid though that this implementation might report nonexisting blank lines when moving by line with the review cursor.

I don't think so. If I read this code correctly, allowMoveToOffsetPastEnd indicates whether we are allowed to move the cursor past the last character. This is for example allowed in Notepad++ but not allowed in virtual buffers. But the code in OffsetTextInfo.move() only respects this when moving by character. This part doesn't make any sense to me - hence this PR. If you can move to a certain position by character - why we can't move to it by line.

However having said that, I agree that any change in this low-level TextInfo implementation might cause undesired behavior somewhere else, so I'll keep an eye on any bug reports if there are any.

Also updated the comment.

LeonarddeR · 2025-06-27T09:56:24Z

I can't find any strange behavior with the review cursor in neither classic notepad nor NP++. NP++ improves in such a way that the review cursor can now also reach the last line, which is great.

Co-authored-by: Leonard de Ruijter <3049216+LeonarddeR@users.noreply.github.com>

LeonarddeR · 2025-06-30T16:25:16Z

I just added the merge early label since I think this needs some broader testing in Alpha.

seanbudd

Would we consider this an API breaking change?

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

mltony · 2025-07-10T22:09:22Z

@seanbudd, regarding API breaking, I just wanted to mention yet again that this is also a bug. As I mentioned before, without this PR the entire textInfo API is unusable as there is no reliable way to collapse textInfo and this breaks a few of my add-ons. It would be unfortunate if we have to wait until next major release to fix a bug like that.

seanbudd · 2025-08-13T05:16:12Z

@SaschaCowley @michaelDCurran - do you mind reviewing this so we can merge it early into 2026.1?

@mltony - could you merge in master and add an entry for API breaking changes?

mltony · 2025-08-15T16:24:15Z

@seanbudd, done!

seanbudd · 2025-08-18T06:32:01Z

Thanks

Link to issue number: Related to #18744 Follow up of #18348 Summary of the issue: When working on #18744, I again stumbled upon the fact that the story length reported by EditTextInfo is too high. This was discovered after system test failures caused by #18348, i.e. reporting of an extraneous blank at the end of the document when using NVDA review cursor. Note, this is not a bug in #18348 and was accounted for in #18744 after the test failure. To reproduce 1. Open notepad classic 2. type `a` 3. Position the caret at the end of the line 4. Press nvda+delete or nvda+numpad delete Observe that NVDA reports 50%, not 100% Observe that `nav.makeTextInfo("caret")._getStoryLength()` reports 2, not 1 Description of user facing changes: None Description of developer facing changes: None Description of development approach: Fixed the off by one errors by removing the `+ 1` cases. Testing strategy: Tested str from this pr. Known issues with pull request: None

…cess#18348)" This reverts commit 8742b3a.

Fixes #19152 Fixup for #18348, Supersedes #19203, #19171 Summary of the issue: In LibreOffice, it is not possible to use braille panning since #18348. Description of user facing changes: Panning works again in LibreOffice Description of developer facing changes: removed OffsetsTextInfo.allowMoveToOffsetPastEnd. Description of development approach: Rather than having one allowMoveToOffsetPastEnd class attribute, introduce a allowMoveToUnitOffsetPastEnd method that takes a unit.

Fix OffsetTextInfo.collapse() and OffsetTextInfo.move()

6e75803

mltony force-pushed the fixTIMove branch from e6f8d67 to 6e75803 Compare June 26, 2025 22:41

mltony marked this pull request as ready for review June 27, 2025 00:33

mltony requested a review from a team as a code owner June 27, 2025 00:33

mltony requested a review from seanbudd June 27, 2025 00:33

mltony marked this pull request as draft June 27, 2025 00:37

Fix system test

012f6aa

mltony marked this pull request as ready for review June 27, 2025 04:43

Address comment

a8575c3

Pre-commit auto-fix

bae89c0

LeonarddeR approved these changes Jun 27, 2025

View reviewed changes

Comment thread source/textInfos/offsets.py Outdated

Update source/textInfos/offsets.py

8247486

Co-authored-by: Leonard de Ruijter <3049216+LeonarddeR@users.noreply.github.com>

LeonarddeR added the merge-early Merge Early in a developer cycle label Jun 30, 2025

seanbudd added the blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. label Jul 1, 2025

SaschaCowley reviewed Jul 8, 2025

View reviewed changes

Comment thread source/textInfos/offsets.py

seanbudd reviewed Jul 10, 2025

View reviewed changes

Comment thread user_docs/en/changes.md Outdated

Update user_docs/en/changes.md

08bb02c

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

seanbudd added this to the 2026.1 milestone Jul 22, 2025

seanbudd requested a review from SaschaCowley July 31, 2025 05:57

seanbudd added api-breaking-change and removed blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. labels Jul 31, 2025

seanbudd approved these changes Jul 31, 2025

View reviewed changes

SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Aug 4, 2025

seanbudd requested a review from michaelDCurran August 13, 2025 05:15

mltony added 3 commits August 15, 2025 09:16

Merge branch 'master' into fixTIMove

9b4c1f6

Merge branch 'fixTIMove' of github.com:mltony/nvda into fixTIMove

da9596b

Update changes

417b493

SaschaCowley approved these changes Aug 19, 2025

View reviewed changes

SaschaCowley merged commit 8742b3a into nvaccess:master Aug 19, 2025
21 of 22 checks passed

LeonarddeR mentioned this pull request Aug 20, 2025

Fix off by one text length in EditTextInfo #18767

Merged

5 tasks

LeonarddeR mentioned this pull request Sep 14, 2025

Using text review, when navigating by word, unexpected blank item encountered at the end #18912

Closed

Emil-18 mentioned this pull request Sep 29, 2025

remove ScintillaTextInfo.collapse, and find another way to solv #17430 #18900

Closed

This was referenced Oct 29, 2025

LibreOffice Writer: Cannot move braille to next paragraph #19152

Closed

Fix regression: Restore ability to move braille to next paragraph #19163

Closed

Fix regression: restore ability to move braille to next paragraph in LibreOffice Writer #19171

Closed

nvdaes added a commit to nvdaes/nvda that referenced this pull request Nov 11, 2025

Revert "Fix OffsetTextInfo.collapse() and OffsetTextInfo.move() (nvac…

57809e4

…cess#18348)" This reverts commit 8742b3a.

nvdaes mentioned this pull request Nov 11, 2025

Revert "Fix OffsetTextInfo.collapse() and OffsetTextInfo.move() (#18348)" #19203

Closed

LeonarddeR mentioned this pull request Nov 11, 2025

Fix LibreOffice braille scrolling #19204

Merged

5 tasks

nvdaes mentioned this pull request Nov 15, 2025

Revert "Fix off by one text length in EditTextInfo (#18767)" #19215

Closed

Uh oh!

Conversation

mltony commented Jun 26, 2025

Link to issue number:

Summary of the issue:

Description of user facing changes:

Description of developer facing changes:

Description of development approach:

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Jun 26, 2025

Uh oh!

mltony commented Jun 27, 2025

Uh oh!

nvdaes commented Jun 27, 2025

Uh oh!

mltony commented Jun 27, 2025

Uh oh!

LeonarddeR commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mltony commented Jun 27, 2025

Uh oh!

LeonarddeR commented Jun 27, 2025

Uh oh!

Uh oh!

LeonarddeR commented Jun 30, 2025

Uh oh!

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mltony commented Jul 10, 2025

Uh oh!

seanbudd commented Aug 13, 2025

Uh oh!

mltony commented Aug 15, 2025

Uh oh!

seanbudd commented Aug 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

LeonarddeR commented Jun 27, 2025 •

edited

Loading