nvda icon indicating copy to clipboard operation
nvda copied to clipboard

Improve Paragraph Navigation -- Add Normal and Block Style

Open rob-aph opened this issue 2 years ago • 28 comments

Link to issue number:

Closes #13797

Summary of the issue:

New feature: add support for both normal and block style paragraph navigation for editors which do not support such navigation natively. Also allows paragraph navigation in web browsers.

Description of how this pull request fixes the issue:

Offers improved paragraph navigation, as well as the option to use an editor's native paragraph navigation as before.

Testing strategy:

You'll find settings to control this feature in a new settings panel titled Document Navigation. The first option, Handled by application, causes NVDA to perform exactly as it does without this patch. The second option, Normal style, adds normal paragraph navigation (paragraphs delimited by Enter) to editors which do not support this functionality. The third option, Block style, is similar to the second option, but moves to paragraphs delimited by at least one blank line. The default is the first option, Handled by application, which acts as before, preventing users from experiencing unexpected changes in functionality. If it is impossible to find or move to the next or previous paragraph when either of the latter two options is selected, NVDA acts as if the Handled by application option is selected, and performs as before.

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.

Change log entries:

New features

  • Adds support for both normal and block style paragraph navigation for use with editors that do not support paragraph navigation natively.

Changes Bug fixes For Developers

Code Review Checklist:

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

rob-aph avatar Jun 14 '22 17:06 rob-aph

@rob-aph - what feedback are you seeking specifically?

seanbudd avatar Jun 20 '22 05:06 seanbudd

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.

cary-rowen avatar Jun 20 '22 05:06 cary-rowen

Is this an acceptable feature? Is it ready to go? These kinds of things.

rob-aph avatar Jun 21 '22 10:06 rob-aph

Both normal and block styles give up after searching 250 lines. Can you tell me which editor you were using which made NVDA hang?

rob-aph avatar Jun 21 '22 10:06 rob-aph

@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

cary-rowen avatar Jun 21 '22 10:06 cary-rowen

Can you please try again with my latest commit? I optimized a bit, and stopped trying to read more than the first 250 lines of paragraphs. I believe the paragraphs I detected in your book were much larger than I expected, and this caused the problem.

rob-aph avatar Jun 21 '22 17:06 rob-aph

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

cary-rowen avatar Jun 22 '22 09:06 cary-rowen

This PR will also need to include a description of the option in the user guide. https://www.nvaccess.org/files/nvda/releases/2022.1/documentation/userGuide.html#NVDASettings

seanbudd avatar Jun 27 '22 03:06 seanbudd

@rob-aph Can you add an unassigned gesture to this feature to toggle these styles?

cary-rowen avatar Jun 27 '22 04:06 cary-rowen

Sean,

I have tested with Notepad, Wordpad, Visual Studio Code, Notepad++, and TextPad. It doesn’t work with TextPad, but NVDA doesn’t work so well with TextPad anyway. There is an issue with VSCode where if using normal paragraph style, VSCode will wrap around to the top of the file if you attempt to move twice from the second to the last paragraph in the file to the next paragraph. I suspect this has something to do with the TextInfo used by VSCode, but I plan to see if I can improve the situation.

As I recall, the issue with Word was a sluggishness issue – something would take too long, even in small files, and NVDA would eventually fail the call. Actually, it was more Outlook’s reading pane I was trying to get to work, but I believe Outlook and Word use the same TextInfo. There may be more I can do with Word when I become more knowledgeable on the peculiarities of it’s TextInfo – I haven’t entirely given up.

I also recall this PR working in Visual Studio 2019 in the past, but I see now it doesn’t. I believe the issue may be that I require State.MULTILINE, and this value is not set in the focus object for VS 2019. Basically, I’m trying to avoid these commands getting run in single line edit fields, etc.

I would be happy to advertise this PR and let users test it, but I have no idea how to get the URL for it’s AppVeyor build, or how long the build remains available. Can you point me in the right direction?

rob-aph avatar Jun 27 '22 19:06 rob-aph

Sure, no problem. Good idea.

rob-aph avatar Jun 28 '22 11:06 rob-aph

I would be happy to advertise this PR and let users test it, but I have no idea how to get the URL for it’s AppVeyor build, or how long the build remains available. Can you point me in the right direction?

You can check the AppVeyor CI/CD build from the NVDA AppVeyor history or from the Status Checks at the bottom of the PR.

You will find the exe build in the artefacts of the build for d2eb59d.

https://ci.appveyor.com/api/buildjobs/aln761e4f3adcufl/artifacts/output%2Fnvda_snapshot_pr13798-25707%2C489f8c97.exe

seanbudd avatar Jun 29 '22 02:06 seanbudd

@seanbudd Thanks for the AppVeyor info. I'll advertize this after a few more itterations -- attempt at paragraph support in Outlook, etc.

rob-aph avatar Jun 29 '22 11:06 rob-aph

@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.

rob-aph avatar Jul 13 '22 11:07 rob-aph

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • FAIL: Lint check. See test results for more information.
  • FAIL: System tests. See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/tas67ulu006hbbf6/artifacts/output/nvda_snapshot_pr13798-25846,f3ab6400.exe

See test results for failed build of commit f3ab640057

AppVeyorBot avatar Jul 13 '22 12:07 AppVeyorBot

@seanbudd Thanks. I think I know how to proceed now.

rob-aph avatar Jul 15 '22 08:07 rob-aph

@cary-rowen Unassigned gesture to toggle through paragraph navigation styles has been added.

rob-aph avatar Jul 19 '22 14:07 rob-aph

I tested this feature. pretty good.

cary-rowen avatar Jul 20 '22 06:07 cary-rowen

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • FAIL: System tests. See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/t5et50j3n1maqj0u/artifacts/output/nvda_snapshot_pr13798-26047,2466e5d4.exe

See test results for failed build of commit 2466e5d4b9

AppVeyorBot avatar Jul 29 '22 11:07 AppVeyorBot

@seanbudd Let me ask, if I rebase/squash my commits from the command line, does that invalidate the pull request? I would like to clean things up before requesting a review. Similarly, does rebaseing on to the latest master branch cause issues?

rob-aph avatar Aug 01 '22 15:08 rob-aph

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • FAIL: System tests. See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/i5na2b45gcot6v7g/artifacts/output/nvda_snapshot_pr13798-26056,e9d77ca3.exe

See test results for failed build of commit e9d77ca368

AppVeyorBot avatar Aug 01 '22 15:08 AppVeyorBot

Squashing/rebasing will reset the review - currently I have the option to review the changes since my last review. If the last commit which was reviewed is lost, then I will have to re-review for scratch. If you keep the commit 34b118e and only change newer commits this shouldn't be an issue. The pull request will eventually be squash merged into master.

Merging in the latest master won't cause issues.

seanbudd avatar Aug 02 '22 00:08 seanbudd

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • FAIL: System tests. See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/55nyvng03ce5xwlk/artifacts/output/nvda_snapshot_pr13798-26101,053ac179.exe

See test results for failed build of commit 053ac179de

AppVeyorBot avatar Aug 04 '22 16:08 AppVeyorBot

@rob-aph Could you also consider this comment

cary-rowen avatar Aug 12 '22 01:08 cary-rowen

  • FAIL: System tests. See test results for more information.
  • PASS: Lint check.
  • PASS: Unit tests.
  • PASS: Translation comments check.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/u64w2prdkq9x2vfh/artifacts/output/nvda_snapshot_pr13798-26392,a82b905e.exe

See test results for failed build of commit a82b905e6d

AppVeyorBot avatar Aug 29 '22 12:08 AppVeyorBot

@seanbudd Continuing, it appears that _validateConfig_featureFlag is not expecting anything besides a BoolFlag. I get this error when attempting to update the config setting, and when NVDA switches app profiles. Error follows: Traceback (most recent call last):

  File "scriptHandler.py", line 289, in executeScript
    script(gesture)
  File "globalCommands.py", line 3687, in script_toggleParagraphStyle
    config.conf["documentNavigation"]["paragraphStyle"] = newFlag
  File "config\__init__.py", line 1237, in __setitem__
    val = self.manager.validator.check(spec, val)
  File "c:\src\nvda\.venv\lib\site-packages\configobj\validate.py", line 615, in check
    return self._check_value(value, fun_name, fun_args, fun_kwargs)
  File "c:\src\nvda\.venv\lib\site-packages\configobj\validate.py", line 647, in _check_value
    return fun(value, *fun_args, **fun_kwargs)
  File "config\featureFlag.py", line 127, in _validateConfig_featureFlag
    'Expected a featureFlag value in the form of a string. EG "disabled", "enabled", or "default".'
configobj.validate.ValidateError: Expected a featureFlag value in the form of a string. EG "disabled", "enabled", or "default". Got <enum 'ParagraphNavigationFlag'> with value: ParagraphNavigationFlag.DEFAULT instead.

rob-aph avatar Sep 02 '22 00:09 rob-aph

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • FAIL: System tests. See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/tnap1x83ar5qt6p4/artifacts/output/nvda_snapshot_pr13798-26429,7612a6e6.exe

See test results for failed build of commit 7612a6e6e5

AppVeyorBot avatar Sep 02 '22 01:09 AppVeyorBot

This still needs a section in the user guide

seanbudd avatar Sep 20 '22 04:09 seanbudd

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • FAIL: Lint check. See test results for more information.
  • FAIL: System tests (tags: installer NVDA). See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/51uf9uwi71bn9gpn/artifacts/output/nvda_snapshot_pr13798-26687,7e790efa.exe
  • CI timing (mins): INIT 0.0, INSTALL_START 0.9, INSTALL_END 0.9, BUILD_START 0.0, BUILD_END 22.8, TESTSETUP_START 0.0, TESTSETUP_END 0.3, TEST_START 0.0, TEST_END 13.8, FINISH_END 0.1

See test results for failed build of commit 7e790efa89

AppVeyorBot avatar Sep 26 '22 15:09 AppVeyorBot

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • FAIL: System tests (tags: installer NVDA). See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/a73p32bqtlvl7oui/artifacts/output/nvda_snapshot_pr13798-26692,bdf34e71.exe
  • CI timing (mins): INIT 0.0, INSTALL_START 0.8, INSTALL_END 0.8, BUILD_START 0.0, BUILD_END 22.9, TESTSETUP_START 0.0, TESTSETUP_END 0.2, TEST_START 0.0, TEST_END 12.9, FINISH_END 0.1

See test results for failed build of commit bdf34e71bd

AppVeyorBot avatar Sep 28 '22 13:09 AppVeyorBot