Skip to content

Conversation

rob-aph
Copy link
Contributor

@rob-aph rob-aph commented Jun 14, 2022

Link to issue number:

Closes #13797

Summary of the issue:

Not all applications support navigating by paragraph (control + up/down arrow) natively.
A specific example is web browsers.

Description of how this pull request fixes the issue:

Add "Document Navigation" settings panel allowing explicit choice of strategy for navigating by paragraph:

  • Handled by application (default): No change in behavior. The application is responsible for responding to the key press.
  • Single line break: Use a single end of line character to define a paragraph boundary.
  • Multi line break: Use two or more end of line characters (I.E. at least one blank line) to define a paragraph boundary.

Fallback to "Handled by application" when single line break or double line break fails.

Testing strategy:

Tested with the following applications:

  • Notepad
  • Wordpad
  • Visual Studio 2022
  • Visual Studio Code
  • Notepad++
  • Bookworm
  • TextPad

Known issues with pull request:

Microsoft Word and Outlook are only supported when using UIA. Only searches forward or backward a maximum of 250 lines to keep performance reasonable.

  • TextPad (TextPad is not supported and is safely ignored when active.)
  • Visual Studio 2022
    • Causes errors but works some of the time
    • These errors also occur when not using improved paragraph navigation, indicating that the problem lies with retrieving text from the TextInfo used with this app, and not with this feature.
  • Visual Studio Code
    • Move to next paragraph when at the bottom of the document can wrap back to the top of the document. This appears to be an issue with the specific TextInfo used in this app.

Change log entries:

New features

* Adds support for both single line break (normal) and multi line break (block) paragraph navigation for use with editors that do not support such navigation natively.

Description of changes for the user:

Paragraph navigation is now supported in editors, such as Notepad and Notepad++, which do not support this feature natively. For editors which already support paragraph navigation, such as WordPad, block style paragraphs (those separated by one or more blank lines) may now be navigated in addition to single line break paragraphs.
When selecting the default option, Handled by application, paragraph navigation performs exactly as it did before this feature was added.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@seanbudd seanbudd linked an issue Jun 16, 2022 that may be closed by this pull request
@seanbudd

This comment was marked as resolved.

@cary-rowen
Copy link
Contributor

I tested this and it looks good,
For 'Block Style': If in an editor with a lot of text NVDA takes some time to retrieve empty lines, NVDA will hang during the process.

@rob-aph

This comment was marked as resolved.

@rob-aph
Copy link
Contributor Author

rob-aph commented Jun 21, 2022 via email

@cary-rowen
Copy link
Contributor

@rob-aph
Hi, I opened an epub book using bookworm, The following is the repo of bookworm:
https://github.com/blindpandas/bookworm

Here is the book file used for testing(You need to unzip the file to get the epub file):
test.zip

* Fix functions wich speak paragraphs to give up after searching 250 lines

* stop collapsing TextInfo objects where appears unnecessary to save time
@rob-aph
Copy link
Contributor Author

rob-aph commented Jun 21, 2022 via email

@cary-rowen
Copy link
Contributor

Hi, @rob-aph

I tested the latest commit and there is a noticeable improvement in NVDA hang time, but there is still a lag in "block navigation".
I would also like to hear other people's opinions, Is this acceptable?

Thanks

@rob-aph rob-aph marked this pull request as ready for review June 23, 2022 15:56
@rob-aph rob-aph requested a review from a team as a code owner June 23, 2022 15:56
@rob-aph rob-aph requested a review from seanbudd June 23, 2022 15:56
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Hi @rob-aph,

I think this is a helpful and acceptable feature.
I would also like to hear from users though, you may want to advertise #13797 and the AppVeyor PR build in the user groups to seek more feedback.

Which text editors did you test with?

Is there anything blocking the Word implementation?

@seanbudd

This comment was marked as resolved.

@seanbudd seanbudd marked this pull request as draft June 27, 2022 03:20
@cary-rowen

This comment was marked as resolved.

rob-aph and others added 2 commits June 27, 2022 14:07
Default for paragraphStyle is now "auto"

Co-authored-by: Sean Budd <[email protected]>
* Move paragraphHelper.py to utils

* Add various annotations

* Change bool next to nextParagraph
@rob-aph
Copy link
Contributor Author

rob-aph commented Jun 27, 2022 via email

@rob-aph

This comment was marked as resolved.

@seanbudd

This comment was marked as resolved.

@rob-aph

This comment was marked as resolved.

* This is currently too slow, because it depends on moving by line, which appears to be the most undesirable way to manipulate a TextInfo for a Word document.

* To test in Word, please switch to Browse mode, as Focus mode support has not yet been added.

* Also works in Outlook message views.
@rob-aph
Copy link
Contributor Author

rob-aph commented Jul 13, 2022

@seanbudd I have tried harder to get this to work in Word / Outlook. You asked if anything was blocking this, and I can now respond yes. It appears very inefficient to move by line in the TextInfo used by Word / Outlook. In fact, it can sometimes hang for reasons I currently do not understand. I worked around one issue, the fact that attempting to move to the next line when there is not another line returns 1 indicating that movement occurred, when in fact, nothing changed -- it should return 0.

I should add that I only have access to Windows 10, so I have no idea of the
performance on Windows 11. Perhaps it is better.

I have updated my branch to add a demo of Word / Outlook support. I find that even when it doesn't hang, navigating by block paragraphs is extremely slow, even in very short messages.

If you wish to try this in Word, you will have to switch to Browse mode, as I have not hooked up scripts for Focus mode yet, as this is really a test / demo. It works right out of the box in read-only Outlook messages.

@AppVeyorBot

This comment was marked as resolved.

@rob-aph

This comment was marked as resolved.

@cary-rowen
Copy link
Contributor

cary-rowen commented Dec 16, 2022

@rob-aph
You wrote: I feel it's more clear if a message is given, rather than repeating the current paragraph. In the case of the last paragraph with a blank line at the end of the document, "blank" would be repeated, which I feel is much more ambiguous than the message. In Notepad, for example, pressing down arrow in an empty document says "blank." You really have no idea if you are moving through blank lines, or you are at the end of the document.


I think the situation you mentioned belongs to another issue, and the relationship with this PR is not strongly related.
In the current situation, if I navigate to the last paragraph/first paragraph, I want to listen to the complete content of the last paragraph/first paragraph again, which cannot be completed. I can only go back to the adjacent paragraph and repeat the above action.
But for Windows' default behavior, I can press ctrl+Up/Down again to accomplish this.

@CyrilleB79
Copy link
Contributor

CyrilleB79 commented Dec 16, 2022

@seanbudd, I do not know on which point you were expecting an input from me. So I have had a look on last comments of this PR. I have found new point to discuss or clarifys, sorry...

Here is my feedback:

Replace _findEndOfSentence by _findNextEndOfSentence

@rob-aph, this should be implemented as confirmed by @seanbudd in #13798 (comment).

What happens when we press ctrl+up/downArrow and document boundary is reached?

There are 3 options:

  1. Report "No next/previous paragraph"
  2. Report again the current paragraph
  3. Report "No next/previous paragraph" and then report again the current paragraph

For now you have choosen solution 1. Personally, I would prefer solution 3.

However NVDA does not seem fully consistent between all related usages (e.g. upArrow, ctrl+upArrow, PavNum8). It would be interesting to standardize what NVDA reports in all these cases. However, I would recommend not to hold off this PR for this since it could lead to long discussions; rather open a new specific issue.

Behaviour in browse mode (browsers or Word)

What is/should be the behaviour in browse mode? @rob-aph, could you update the initial description to indicate if a specific paragraph navigation is supported or not? And if it is, what is the expected result?

When the cursor is not at the beginning of the paragraph

(edited: This point is not to take into account anymore, see further discussion.)
Where should the cursor jump when pressing ctrl+upArrow?

  1. Go before the first character of the current paragraph
  2. Go before the first character of the previous paragraph

It seems that you have choosen option 2. However, in the following case, option 1. happens:

  • Set paragraph style on "Multi line break"
  • Open Notepad
  • Type the 4 following lines where <emptyLine> is just an empty line:
    abc
    <emptyLine>
    def
    <emptyLine>
    
* Put the cursor on the 4th line
* Press ctrl+upArrow

Result: the cursor jump to the first character of the current paragraph (solution 1.) which is in contradiction with other usages.

Copy link
Contributor

@CyrilleB79 CyrilleB79 left a comment

Choose a reason for hiding this comment

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

@rob-aph, please also consider the slight changes suggested in this review. Thanks.

@seanbudd
Copy link
Member

This is ready to merge now. Is the pull request description up to date?

Can you add a new section "Description of changes for the user".
Include any behaviour which has changed, e.g.

  • new applications which support paragraph navigation
  • any changed paragraph navigation behaviour when using the default navigation style

@rob-aph
Copy link
Contributor Author

rob-aph commented Dec 20, 2022

@seanbudd I think it's ready to go.

@seanbudd seanbudd merged commit dc7eb28 into nvaccess:master Dec 21, 2022
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Dec 21, 2022
@seanbudd
Copy link
Member

Thanks @rob-aph for all your work on this PR!

@cary-rowen
Copy link
Contributor

Hi, @rob-aph
This feature caused NVDA to lag when navigating output text within VS Code's terminal.
Navigating the output of VS Code requires you to be in browse mode, but perhaps NVDA is in edit mode by default, if you try to press Ctrl+up/downArrow before manually pressing NVDA+Spacebar you will find that NVDA is silent briefly, and after a few seconds you hear "No previous paragraph/No next paragraph".
Do you have any thoughts on the situation? In VS Code, especially in non-editable areas, should paragraph navigation be handled by the application?

@seanbudd
Copy link
Member

seanbudd commented Jan 8, 2023

@cary-rowen how do you know it is this pull request? could you create an issue to track this problem?

@cary-rowen
Copy link
Contributor

Hi, @seanbudd @rob-aph
See #14522

@CyrilleB79 CyrilleB79 mentioned this pull request Mar 27, 2023
6 tasks
seanbudd pushed a commit that referenced this pull request Mar 28, 2023
Summary of the issue:
PR #13798 introduced paragraph navigation in documents. Unfortunately two message added by this PR are not translatable: "No next/previous paragraph" since the gettext (_) function call has been forgotten; the translators comments are already there though.

Description of user facing changes
The message will be translatable

Description of development approach
Just added the call to the gettext function (_).
@Adriani90
Copy link
Contributor

@rob-aph first of all thanks for this huge work, really appreciated that you help us in making the world more accessible through your contribution.

I came accross an issue while using this great feature, the speech and UI messages seem not to be translatable:
Se paragraph_helper.py; line 132, 135 and 229; and globalcommands.py line 3980. The messages need to be translatable, this is a standard for all speech and UI messages in NVDA, so you need to put them in a wraper function in order to send them through Gettext to the translators. We use _("") like in this tutorial:
https://phrase.com/blog/posts/translate-python-gnu-gettext/

@seanbudd, @michaelDCurran, @nvdaes maybe this needs a broader documentation in the developer guide?

@nvdaes
Copy link
Collaborator

nvdaes commented Apr 9, 2023

Adriani90 commented

maybe this needs a broader documentation in the developer guide?

IMO, this is well documented in the contributing.
Note that the developer guide doesn't contain info about files in standard format, just about specific files used for NVDA, like character description and symbol pronounciation.
Please see developer guide.
Cheers

@Adriani90
Copy link
Contributor

Adriani90 commented Apr 10, 2023

It might make sense to integrate a gettext linter to make this process more secure.
Here is a good one for example:
https://www.technomancy.org/python/pylint-i18n-lint-checker/

Pylint is already included in flake8 so it should be relatively easy to include this plugin as well.
Ofcourse it needs a check beforehand to change any rules that should or should not be ignored.

@cary-rowen
Copy link
Contributor

@Adriani90
Doesn't #14751 already solve the untranslatable string issue you mentioned?
If you want to suggest that NV Access improve the documentation, can you open a separate Issue?

@Adriani90
Copy link
Contributor

Adriani90 commented Apr 10, 2023 via email

@seanbudd
Copy link
Member

Then only the string in globalcommands.py still needs to be made translatable.

Could you also open an issue for this, if it is still outstanding?

@CyrilleB79
Copy link
Contributor

Then only the string in globalcommands.py still needs to be made translatable.

Could you also open an issue for this, if it is still outstanding?

@Adriani90 I cannot see any untranslatable string in last alpha.
Could you confirm here that all required strings are now translatable in last alpha or give information on what should still be made translatable? Preferably in a new issue for better traceability.

@Adriani90
Copy link
Contributor

It seems now everything is translatable from here, not sure which commit made line 3980 in globalCommands translatable but it seems this is fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked/needs-code-review merge-early Merge Early in a developer cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improved Paragraph Navigation