Skip to content

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

Closed
nvdaes wants to merge 2 commits into
nvaccess:masterfrom
nvdaes:revert18348
Closed

Revert "Fix OffsetTextInfo.collapse() and OffsetTextInfo.move() (#18348)"#19203
nvdaes wants to merge 2 commits into
nvaccess:masterfrom
nvdaes:revert18348

Conversation

@nvdaes

@nvdaes nvdaes commented Nov 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This reverts commit 8742b3a.

name: Revert PR
about: Revert an existing pull request

Reverts PR

Partially reverts #18348

Issues fixed

Fixes #19152

Issues reopened

Reopens #18320

Reason for revert

This causes a regression in LibreOffice Writer documents, where braille cannot be moved to the next paragraph by scrolling forward. (issue #19152).

Can this PR be reimplemented? If so, what is required for the next attempt

@LeonarddeR proposed a solution in #19171 PR which would likely fix #17430 and #19152 properly.

@nvdaes nvdaes requested a review from a team as a code owner November 11, 2025 07:14
@nvdaes nvdaes requested a review from SaschaCowley November 11, 2025 07:14
@nvdaes nvdaes mentioned this pull request Nov 11, 2025
Closed
5 tasks
@LeonarddeR LeonarddeR mentioned this pull request Nov 11, 2025
Merged
5 tasks
seanbudd
seanbudd reviewed Nov 12, 2025
View reviewed changes
Comment thread source/textInfos/offsets.py
Comment thread source/NVDAObjects/window/scintilla.py Outdated
@nvdaes

nvdaes commented Nov 12, 2025

Copy link
Copy Markdown
Collaborator Author

@seanbudd , I've applied your suggestions and updated the PR description.

@seanbudd

seanbudd commented Nov 12, 2025

Copy link
Copy Markdown
Member

Can you investigate the test failures?

@nvdaes

nvdaes commented Nov 12, 2025

Copy link
Copy Markdown
Collaborator Author

@seanbudd wrote:

Can you investigate the test failures?

Failures will be fixed when #19204 is merged.

@seanbudd

seanbudd commented Nov 12, 2025

Copy link
Copy Markdown
Member

I'm somewhat confused though, weren't tests passing before #18348? why are they failing after reverting it?

@nvdaes

nvdaes commented Nov 13, 2025

Copy link
Copy Markdown
Collaborator Author

@seanbudd wrote:

weren't tests passing before #18348?

After #18348, #18767 introduced more related changes, and in #19171, I reverted them so that test pass. See #19171 description.:

Revert "Fix off by one text length in EditTextInfo (#18767) (otherwise system test fail.

@SaschaCowley

SaschaCowley commented Nov 14, 2025

Copy link
Copy Markdown
Member

@nvdaes we can't merge this while tests are failing like this. I think the easiest thing to do is going to be to revert #18348, #18767 and #17431, which should take us back to a good state. We can then re-implement any fixes we've lost with the information we now have.

@SaschaCowley SaschaCowley marked this pull request as draft November 14, 2025 01:47
@nvdaes

nvdaes commented Nov 14, 2025

Copy link
Copy Markdown
Collaborator Author

@SaschaCowley , I'll use the revert template to revert the mentioned PRs, so that you can merge this and move forward.

@nvdaes

nvdaes commented Nov 15, 2025

Copy link
Copy Markdown
Collaborator Author

I think that we should revert #17431 in this same PR, since #18348 reverted these changes and they cannot be reverted from further PRs. Perhaps in this way #18767 doesn't need to be reverted. Let's see if test pass now.

@LeonarddeR

LeonarddeR commented Nov 15, 2025

Copy link
Copy Markdown
Collaborator

@nvdaes To be honest, I don't think neither this pr nor #19215 would be necessary when #19204 is accepted.

@LeonarddeR LeonarddeR mentioned this pull request Nov 15, 2025
Closed
@nvdaes

nvdaes commented Nov 15, 2025

Copy link
Copy Markdown
Collaborator Author

@LeonarddeR , perhaps this is needed to revert the updated changelog, i.e., so our fixes not applied anymore are removed, and for the maintenance of the project, to track better pull request created and not applied anymore, not in terms of functionality, I gess. Anyway, I'll follow Sascha suggestion, and will merge all reverted pull request into a branch on my fork to see if all test pass as expected.

@LeonarddeR

LeonarddeR commented Nov 15, 2025

Copy link
Copy Markdown
Collaborator

Again, these pull requests would only be necessary if there wouldn't be a suitable alternative to fix #19152. #19204 fixes #19152 in a way that is incompatible with your reverts.

@nvdaes

nvdaes commented Nov 15, 2025

Copy link
Copy Markdown
Collaborator Author

Then perhaps you should update the changelog, if you think that it's needed. Also, I would like you to include that now the cursor will move to the end of the document, since this is a change or better a bug fix, though an issue is not created for this. What do you think?

@nvdaes

nvdaes commented Nov 15, 2025

Copy link
Copy Markdown
Collaborator Author

@seanbudd and @SaschaCowley , do you still think that the mentioned PRs should be reverted? Otherwise we can close this.

@LeonarddeR

LeonarddeR commented Nov 15, 2025

Copy link
Copy Markdown
Collaborator

Then perhaps you should update the changelog, if you think that it's needed. Also, I would like you to include that now the cursor will move to the end of the document, since this is a change or better a bug fix, though an issue is not created for this. What do you think?

Fair point, I'll do that

@nvdaes

nvdaes commented Nov 15, 2025

Copy link
Copy Markdown
Collaborator Author

Thanks so much, @LeonarddeR

@seanbudd seanbudd mentioned this pull request Nov 17, 2025
Closed
@seanbudd

seanbudd commented Nov 17, 2025

Copy link
Copy Markdown
Member

Closing in favour of #19204

@seanbudd seanbudd closed this Nov 17, 2025
seanbudd pushed a commit that referenced this pull request Nov 17, 2025
@LeonarddeR
Fix LibreOffice braille scrolling (#19204)
125915b 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd left review comments

@SaschaCowley SaschaCowley Awaiting requested review from SaschaCowley SaschaCowley is a code owner automatically assigned from nvaccess/developers

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LibreOffice Writer: Cannot move braille to next paragraph Notepad++: braille doesn't move to the next line if last line is empty

4 participants

@nvdaes @seanbudd @SaschaCowley @LeonarddeR