Fix bug to move braille to next line in UIA documents - Githubissues

nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows

https://www.nvaccess.org/

Other

2.12k stars 638 forks source link

Fix bug to move braille to next line in UIA documents #17401

Closed nvdaes closed 1 day ago

nvdaes commented 1 week ago

Link to issue number:

Fixes #17251

Summary of the issue:

In some UIA documents which end with an empty line, braille cannot be moved to the last line.

Description of user facing changes

Braille can be moved to the last line in all UIA documents. If the braille move to next line command is run from the last line, the cursor will be moved to the last character. In this case, if the review cursor follows system cursor, the review cursor will also be moved.

Description of development approach

A shouldCollapseToEnd variable has been added to the method to move braille to the next line, to determine if text info should be collapsed to end. When braille is not moved to the next line using other procedures, shouldCollapseToEnd is set to True and text info is collapsed to end.

Testing strategy:

Tested manually in Notepad.

Known issues with pull request:

None.

Code Review Checklist:

[x] Documentation:
- Change log entry
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
[x] Testing:
- Unit tests
- System (end to end) tests
- Manual testing
[x] UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
[x] API is compatible with existing add-ons.
[x] Security precautions taken.

@coderabbitai summary

burmancomp commented 1 week ago

Works for me, thank you! I think that currently when braille is scrolled forward or back within current line, review cursor does not move. Maybe this behavior should be kept for consistency.

burmancomp commented 1 week ago

I just found another issue. I mention it here because I found it with notepad in windows 11.

Open notepad and write some text so that last line contains only one character (no blank line after it). Try then when caret is at that line to press home and end and see what happens when you try to move review cursor. You should not be able to move review cursor to that last line after pressing end.

codeofdusk commented 6 days ago

I suspect you're working with UIA providers that mistakenly set end inclusive (@carlos-zamora and I went back and forth on this for a while in the terminal/console)...

Does this break Word? Its UIATextInfo generally works as expected.

CC @jcsteh, @michaelDCurran.

SaschaCowley commented 3 days ago

@nvdaes this doesn't seem to work quite as expected in Scintilla (Notepad++): the caret isn't being moved to the end of the last line when attempting to pan past the end of the document. Nevertheless, I'm happy with this PR. Could you please respond to @burmancomp's and @codeofdusk's comments though?

nvdaes commented 3 days ago

@codeofdusk commented

Does this break Word?

No, I haven't seen any issue in Word with this PR.

nvdaes commented 3 days ago

@burmancomp commented:

I think that currently when braille is scrolled forward or back within current line, review cursor does not move. Maybe this behavior should be kept for consistency.

This shouldn't affect the review cursor behavior.

About other issues related to Notepad, we can discuss them separately since this PR is been accepted.

nvdaes commented 3 days ago

@SaschaCowley , I've answered comments from other reviewers. About Notepad++, I'll install it and try to investigate and, if needed, I'll create an issue triaging it as p3, like this.

nvdaes commented 3 days ago

Thanks all reviewers. I think that all your suggestions are applied.

burmancomp commented 3 days ago

I am not sure how it was when I wrote about review position but currently when I have one line text in notepad which is not followed by empty line, and when I scroll forward in the situation where last portion of line is already shown in display, review cursor is moved to end of line. This does not happen with alpha version I have.

nvdaes commented 3 days ago

@burmancomp , now I think that I understand your point with review cursor. This is a change to fix this bug, described in the first comment of this PR:

If the braille move to next line command is run from the last line, the cursor will be moved to the last character.

If you have set the review cursor to follow the system cursor, it will be moved and I don't know if we can fix this in a different way in all cases. I don't know if you think that this should block this PR or not.

burmancomp commented 2 days ago

I think dest.move should not return 0 when movement is possible but it seems to do that (I noticed when I added locally log.debug line after if not moved: line. Then I tested with notepad where there was one text line followed by empty line.

So after all I think that it could be useful to fix move function first. Maybe fixing could have some positive effect on other notepad/uia issue. As said current behavior is - as far as I understand - that review position only moves when next/previous line is moved to, not when panning within current line. Finally I agree that this side effect (that review position moves within current line) has very limited effects. I think that at least this could be mentioned in known issues, and bug as to move function should be opened. But if/when bug is some day fixed in move function then it may have effect on this pr.

nvdaes commented 2 days ago

@burmancomp , I've added the description of the side effect with review cursor to the description of changes in this PR. I think that if Microsoft fixes this for UIA documents, this side effect shouldn't happen. Please see the description of this PR. I think that reflecting this side effect in the changes section is more prominent than doing it in known issues. Also, really I don't mind that the cursor is moved to the end when we reach the last line. I prefer this to know that we don't have more lines below. Sometimes the braille display may not work for any reason and this is a way to be more sure that the problem is not in the display.

burmancomp commented 2 days ago

@nvdaes just doing curiosity: how can you be sure that this uia problem is due to microsoft and not related to move function.

Yes sorry you have mentioned review cursor movement in description. There is however minor inconsistency because review cursor is not moved to start of first line if you span left although you are in start of first line.

If this is microsoft issue and if they fix it some day, do you think this pr is needed after microsoft fix.

nvdaes commented 2 days ago

@burmancomp , I'm not completely sure. I thought that this is a Microsoft bug since issue #17402 was labelled with the external fix label. I'll wayt for @SaschaCowley comments and in the meantime I'll work in a PR about profiles. When this PR is clarified I may work in Notepad++.

SaschaCowley commented 1 day ago

@nvdaes I don't think the review cursor issue is a showstopper here.

burmancomp commented 1 day ago

Actually I feel that now behavior is more consistent because in word without uia, caret is moved to end of last line when trying to move to next line. I did not understand earlier that caret is moved (due to my limited understanding and/or hopefully partially because I do not tether braille to focus so often) although this cursor movement is told in description.

I think that in both cases (in word without uia and in notepad after this pr) review cursor is moved to end of last line although it would not follow caret (same seems to be with wordpad). And behavior is consistent in these applications.

So: thanks @nvdaes!