Fix regression: restore ability to move braille to next paragraph in LibreOffice Writer#19171
Fix regression: restore ability to move braille to next paragraph in LibreOffice Writer#19171nvdaes wants to merge 10 commits intonvaccess:masterfrom
Conversation
|
Another possibility would be to revert completely #18348, but hope we find a better solution to fix this regression, keeping the proper behavior of Notepad++. |
|
@nvdaes, I would strongly discourage reverting #18348 as it would reintroduce major regressions in textInfo API. |
|
@mltony , hope we can find a solution without broken the API in an unappropriate way. |
|
With the last commit, braille can be scrolled forward in LibreOffice and in Notepad++. |
nvdaes
changed the title
|
@LeonarddeR , seems that, with this PR, we need to revert your a514710 commit, since in fact we are reverting code introduced in #18348. |
This reverts commit a514710.
|
As well as @mltony, I strongly discourage going forward the current way. This is most likely a bug in the text info implementation used in libre office. The current pr touches unrelated code, such as in edit text info which is not used in Libre Office as far as I can recall. |
|
@LeonarddeR wrote:
No, this PR is reverting changes, and, in addition, touches code just in the braille nextLine function. If textInfo issues can be fixed for buggy programs, like LibreOffice or Notepad++ (in case they contain bugs), I agree. I'll wait for feedback from @SaschaCowley , @seanbudd or @gerald-hartig , to see if I should go forward with this fixing remaining failures in tests. |
|
I just tested the oneliner in #19152 (comment) with braille. It turns out that with this change, it is still possible to route to the last character of a document. Therefore I think it is safe enough to apply it. def _getTextInfos(self):
start = self._start.copy()
start.allowMoveToOffsetPastEnd = startIsEnd = self._startObj == self._endObj
yield start
if startIsEnd:
return
obj = self._startObj.flowsTo
while obj and obj != self._endObj:
info = obj.makeTextInfo(textInfos.POSITION_ALL)
info.allowMoveToOffsetPastEnd = False
yield info
obj = obj.flowsTo
yield self._end.copy() |
|
@LeonarddeR wrote:
I noted that in LibreOffice Writer documents, with NVDA 2025.3.1rc1, if the last line is not empty, the corsor couldn't be moved with braille to the last position, but it can be moved there with this change in braille.py. I'll test your proposed changes about textInfo and, if it works as expected, I'll apply them. |
|
@LeonarddeR , as mentioned in #19152 , for now I'll leave this PR changing the nextLine function in braille.py. I may fix tests and request a review from NV Access, since this allows to move the cursor to the last position of a document in LibreOffice writer, and this is consistent with other applications. I created a different pull request closed by @seanbudd according with @gerald-hartig comment, where he mentioned that they prefer to revert changes in textInfos/offsets.py. I'll do all this now, and if further work is needed, I may continue or even test changes if you create a different PR. |
|
Please reviewers, take special care about #18767. I reverted changes, but I'm not sure if something else needs to be done. I have tested with Notepad on Windows 11 64 bits, and the story lenght seems to be right. |
I can understand why the request would be to revert the changes if they really introduce a bug that can't be solved otherwise. However, the solution I proposed in #19171 (comment) avoids such a revert. If anyone insists on reverting which I'd regret and advise against, there is a revert template that should be used. |
|
@LeonarddeR wrote:
But this is not reverting #18348 completely, since a docstring is maintained This is marked as ready for review. If NV Access prefers your proposal, I advice them to close this PR and you can create a new one. |
This reverts commit dd78ae4.
| # Py3 review: investigation with Python 2 NVDA revealed that | ||
| # adding 1 to this creates an off by one error. | ||
| # Tested using Wordpad, enforcing EditTextInfo as the textInfo implementation. | ||
| return textLen + 1 |
There was a problem hiding this comment.
@nvdaes, this change is dangerous as it might affect other textInfos. I wouldn't touch this file - especially since you're trying to fix LibreOffice problem.
| # Py3 review: investigation with Python 2 NVDA revealed that | ||
| # adding 1 to this created an off by one error. | ||
| # Tested using Notepad | ||
| return watchdog.cancellableSendMessage(self.obj.windowHandle, winUser.WM_GETTEXTLENGTH, 0, 0) + 1 |
There was a problem hiding this comment.
Same here - this might break other textInfo API clients throughout NVDA code.
| shouldCollapseToEnd = True | ||
| # This will allow to move to next paragraph In LibreOffice _Writer, | ||
| # And consistently move to the last position in different applications. | ||
| dest.expand(textInfos.UNIT_LINE) |
There was a problem hiding this comment.
Shall we use self._getReadingUnit() instead of UNIT_LINE?
There was a problem hiding this comment.
I think we should expand to line, as done in Commit 70cd311, but touching the nextLine function of braille.py, to get more consistent results between applications, even if textIfno bugs inherent to these apps should be fixed, additionally.
| lowLimit = 0 | ||
| highLimit = self._getStoryLength() | ||
| if self.allowMoveToOffsetPastEnd: | ||
| if self.allowMoveToOffsetPastEnd and unit == textInfos.UNIT_CHARACTER: |
There was a problem hiding this comment.
This effectively rolls back #18348.
I don't think this is justified. It seems to me that you are replacing #18348 with a hack. It somehow works but we don't have a clear understanding how exactly. This hacky approach will inevitably backfire in other parts and generate more bugs that need to be fixed later down the road.
I think @LeonarddeR investigated this issue and proposed a much cleaner solution to fix this bug. I would strongly suggest to focus on that cleaner solution.
There was a problem hiding this comment.
I've tested @LeonarddeR proposal, and I prefer, at least for now, the solution proposed by me in this PR. Seems that touching TreeCompoundTextInfo._getTextInfos , and setting allowMoveToOffsetPastEnd to False in the SymphonyTextInfo class of the soffice.py module, when we reach the last line of a Writer document, if this line is not empty, the cursor is not moved to the end of the line, and this is not consistent with other applications.
Both approaches fix the regression, but I don't think that I'm aplying a hack. I'm touching the function to move to next line in the braille module. This regression is related to braille. So I'm not against fixing bugs in textInfos.
Anyway, I won't close this. This is ready for review, and if the maintainers of the project think that this should be closed, and they provide reasoning about possible issues with my approach, I'll be happy if they close this.
There was a problem hiding this comment.
At this point our best understanding is that OffsetTextInfo.allowMoveToOffsetPastEnd should apply to any type of moving, regardless of whether we're moving by character or by line. It is hard for me to imagine a textInfo where a certain position can only be moved to by character, but not by line or paragraph.
Your change in textInfos\offsets.py:666 reverts my PR #18348. You are restoring behavior that is clearly wrong. That's why that's a hack.
|
Let's stop criticizing each other’s solutions. It only wastes time and leads to heated discussions. Perhaps NV Access can give a final say on which path to take? |
|
@nvdaes I am with @mltony on this one. This isn't a braille issue, but an issue with the SymphonyDocumentTextInfo, as the same issue occurs when moving the review cursor to the next line. Your Approach would almost be the same as rewriting the speakObject function to speak the name fetched directly from an accessibillity API (or other source) if some condition is met, instead of exposing the name through an NVDAObject and allowing the speakObject function to fetch the name from the object. To be honest, I would rather have the issue, and wait until it is fixed in the text info, than fixing it just for braille users, and create inconsistent behavior between speech and braille |
|
@Emil-18 wrote:
No. I'll update the description of this PR to mention that this doesn't break it, and that the only difference with Leonard proposal that I find is that this PR makes the cursor to be moved at the end of the document when the las line is reached scrolling with braille, on consistency with behavior of other applications. With this PR the review cursor can be also moved across lines. |
|
@nvdaes I tested it, and it reintroduces the issue with not being able to move to the last line in notepad ++ if the last line is empty |
|
I'm going to close this pr for now, due to the following reasons:
Closing this pull request also creates responsibilities. I hereby promise that I will submit a new pull request that at least has the necessary unit tests to validate the behavior of compound documents, so that we can then strive for a fix with a minimal footprint. |
|
@LeonarddeR wrote:
Not here. In Notepad++ on my system, on Windows 11 64 bits, there`s not difference between the behavior of braille in NVDA 2025.3.1 and this PR. Perhaps steps to reproduce the issue should be provided. @LeonarddeR wrote:
Just 4 participants. I think we should give more time to NV Access people, considering that they suggested to revert changes made in #18348 . I'm happy if you want to investigate better solutions, but I don't think that you should close this without giving time to NV Access to provide feedback, and time to me in case something needs to be done from my part. |
|
@nvdaes The issue in notepad ++ in your pr isn't with braille, but with the OffsetsTextInfo. It works when moving to next line with braille, because you expand the TextInfo to line Steps to reproduce
Actual behaviorNVDA says "bottom" and the review cursor doesn't move Expeccted behaviorThe review cursor moves to the blank line |
|
@LeonarddeR wrote:
Not here. In Notepad++ on my system, on Windows 11 64 bits, there`s not difference between the behavior of braille in NVDA 2025.3.1 and this PR. Perhaps steps to reproduce the issue should be provided. @LeonarddeR wrote:
Just 4 participants. I think we should give more time to NV Access people, considering that they suggested to revert changes made in #18348 . I'm happy if you want to investigate better solutions, but I don't think that you should close this without giving time to NV Access to provide feedback, and time to me in case something needs to be done from my part. @Emil-18 , in fact the described issue with Notepad++ was fixed in master, where the p2 regression with LibreOffice was introduced. |
|
@nvdaes I might have been too quick here, I'm just seeing to much difficulties to continue with this. I've done several work on this today and I can just say with certainty that the work you are doing on this issue, though greatly appreciated, does not do justice to the underlying problem. I'm happy to reopen this as long as you leave this a draft until NV Access kicks in with further triage. |
|
Hi all - per our release process, unless a clean full fix is immediately available, we are for reverting PRs that decrease the overall quality of NVDA. Further work to properly fix this issue and reintroduce changes from #18348 is welcome. @nvdaes - please use the revert PR template and minimize these changes to the partial revert required to restore behaviour before #18348. Any other fixes should be in a separate PR, i.e. what @LeonarddeR is proposing to fix the issue fully. |
seanbudd
added
the
conceptApproved
|
@mltony that is exactly why we have asked for a partial revert. |
|
Please see #19203 |
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.
6 participants
Link to issue number:
Fixes #19152
Summary of the issue:
Braille cannot be moved to next paragraph, due to a regression introduced in #18348
Description of user facing changes:
Braille can be moved to the next paragraph again.
Description of developer facing changes:
None.
Description of development approach:
Restore changes introduced for the
movemethod intextInfos/offsets.py.In
nextLineof braille.py, if the cursor is not moved, expand to line before collapsing.Revert "Fix off by one text length in EditTextInfo (#18767) (otherwise system test fail.
Testing strategy:
Tested locally with a Focus 40 braille display, in LibreOffice Writer and Notepad++:
Checked that, when scrolling forward with braille, the cursor is moved to the end of the document from the last line, also in LibreOffice Writer (this is a documented change since this was not happening in NVDA 2025.3.1).
Checked that the behavior is consistent regardless of the Read by paragraph option (when this is turned on or off).
Checked that the review cursor can be moved across lines, i.e., performing the review previous and review next line commands.
Tested proposal by @LeonarddeR mentioned in #issuecomment-3506121489
The only difference found between Leonards's proposal and this pull request is that, with this PR, the cursor can be moved to the end of the documment in LibreOffice Writer from the last line, which is consistent with the behavior of other applications.
Known issues with pull request:
None.
Code Review Checklist: