Style navigation QuickNav command

[mltony]

Link to issue number:

#16000

Summary of the issue:

Adding jump to same style and jump to different style QuickNav commands.

Description of user facing changes

Adding jump to same style and jump to different style QuickNav commands. They are not assigned to any keyboard gestures by default.

Description of development approach

Scanning document by paragraph in the desired direction and analyzing styles within each paragraph.

Testing strategy:

Tested manually in Chrome and Firefox. Added a system test.

Known issues with pull request:

• In addition to same style navigation I also added different style navigation command because it's implementation turned out to be very similar - I hope that's ok.
• This PR will have conflicting changes with #16031 - I will resolve conflicts whenever either of these PRs is merged.
• ~~Another quirk to be aware of: in this PR I treat paragraphs separately. For example if we have an entire document written in the same style, then jump to next same style command would iterate over paragraphs. In theory we can try to merge paragraphs and parts of paragraphs written in the same style together, but this would make code more complicated and I personally feel current approach is more intuitive, but LMK if you think otherwise.~~ Update: as requested, now I changed the code so that every chunk found by this pR can span multiple paragraphs.

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.

[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/f9pah2q8r0ivh5ce/artifacts/output/nvda_snapshot_pr16050-30669,3297106a.exe
• CI timing (mins): INIT 0.0, INSTALL_START 1.0, INSTALL_END 1.0, BUILD_START 0.0, BUILD_END 28.2, TESTSETUP_START 0.0, TESTSETUP_END 0.3, TEST_START 0.0, TEST_END 17.6, FINISH_END 0.2

See test results for failed build of commit 3297106adb

[mltony]

The bot says some system test with tag Installer is failing. I couldn't find any information on waht test is failing. Not sure if this is flaky test. If it's not flaky - could someone help me to find which test is failing? It wasn't very clear to me based on bot's comment.

[seanbudd]

Yes I think that test failure is unrelated

[CyrilleB79]

In an Outlook message, I was hoping to jump to "answers in red" sent by a colleague of mine.

Unfortunately, it does not work. I get errors such as the following when calling all 4 commands (here with Move to next different style text command):

ERROR - scriptHandler.executeScript (16:39:49.269) - MainThread (9984):
error executing script: <bound method BrowseModeTreeInterceptor.addQuickNav.<locals>.<lambda> of <nvdaBuiltin.appModules.outlook.MailViewerTreeInterceptor object at 0x01DBFF90>> with gesture 'j'
Traceback (most recent call last):
  File "scriptHandler.py", line 295, in executeScript
    script(gesture)
  File "browseMode.py", line 508, in <lambda>
    script = lambda self,gesture: self._quickNavScript(gesture, itemType, "next", nextError, readUnit)
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "browseMode.py", line 466, in _quickNavScript
    item = next(iterFactory(direction, info))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "browseMode.py", line 2111, in _iterTextStyle
    styles = self._extractStyles(initialTextInfo)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "browseMode.py", line 2075, in _extractStyles
    if field.field['role'] == controlTypes.Role.MARKED_CONTENT:
       ~~~~~~~~~~~^^^^^^^^
KeyError: 'role'

[CyrilleB79]

Regarding unassigned keys: IMO, if this feature works as advertised, it would be much more interesting to have default assigned keys for it and keep QuickNav Text paragraph navigation commands unassigned (#16031).

Maybe we should merge this PR and #16031 with unassigned gestures and test them during alpha stage. Then at the end of alpha stage, we can figure if one or both of these feature need to have an assigned gesture and add them.

[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/x54uvy82l5mn3lmq/artifacts/output/nvda_snapshot_pr16050-30845,d32acb65.exe
• CI timing (mins): INIT 0.0, INSTALL_START 1.0, INSTALL_END 1.0, BUILD_START 0.0, BUILD_END 29.1, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 11.0, FINISH_END 0.2

See test results for failed build of commit d32acb6519

[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/aywbwyso43qlyp75/artifacts/output/nvda_snapshot_pr16050-30906,3745ebba.exe
• CI timing (mins): INIT 0.0, INSTALL_START 1.0, INSTALL_END 1.0, BUILD_START 0.0, BUILD_END 28.7, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 10.7, FINISH_END 0.1

See test results for failed build of commit 3745ebba42

[seanbudd]

Hi @mltony , There are 5 review comments from last review unresolved. I think they were missed as they were automatically hidden due to the many review comments.

[mltony]

@seanbudd my bad, didn't see those comments. Now I addressed all of them.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/v0a8ui9clvcs177v/artifacts/output/nvda_snapshot_pr16050-31009,f8c0abda.exe
• CI timing (mins): INIT 0.0, INSTALL_START 1.0, INSTALL_END 0.9, BUILD_START 0.0, BUILD_END 26.5, TESTSETUP_START 0.0, TESTSETUP_END 0.3, TEST_START 0.0, TEST_END 2.3, FINISH_END 0.1

See test results for failed build of commit f8c0abda19

[CyrilleB79]

Hi Tony,

The feature does not work at all in Word (browse mode): With Word legacy (object model):

• move to next same style text jumps to next word, no matter the formatting.
• move to next different style text causes an error:

ERROR - scriptHandler.executeScript (22:58:32.703) - MainThread (8308):
error executing script: <bound method BrowseModeTreeInterceptor.addQuickNav.<locals>.<lambda> of <NVDAObjects.window.winword.WordDocumentTreeInterceptor object at 0x00C6C370>> with gesture '<'
Traceback (most recent call last):
  File "scriptHandler.py", line 295, in executeScript
    script(gesture)
  File "browseMode.py", line 559, in <lambda>
    script = lambda self,gesture: self._quickNavScript(gesture, itemType, "next", nextError, readUnit)
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "browseMode.py", line 517, in _quickNavScript
    item = next(iterFactory(direction, info))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "browseMode.py", line 2235, in _iterTextStyle
    paragraph.collapse(end=True)
  File "treeInterceptorHandler.py", line 234, in collapse
    return self.innerTextInfo.collapse(end=end)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "NVDAObjects\window\winword.py", line 1048, in collapse
    raise RuntimeError

If instead you use UIA in Word:

• move to next same style text jumps to next different style text instead
• move to next different style text reports "No next different style text", even when there are various formatting.

Could you double check all the Word/Outlook use cases? Thanks.

[mltony]

@CyrilleB79 , I investigated a bit into MSWord. Pushed one more commit to address one issue with MSWord - now I am merging identical styles together. This is especially important for MSWord since textInfos there cchange their format every word - but - they mark all the words with a detected language, while spaces in between words have no assigned language. Since we filter out language from the list of styles, the remaining styles are identical and should be merged. Also I filed two issues - these appear to be problems with MSWOrd-specific textInfo implementation: #16171 and #16172.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/hc01obd2u4vu6mxq/artifacts/output/nvda_snapshot_pr16050-31028,b1e5153e.exe
• CI timing (mins): INIT 0.0, INSTALL_START 1.0, INSTALL_END 0.9, BUILD_START 0.0, BUILD_END 27.7, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 2.2, FINISH_END 0.1

See test results for failed build of commit b1e5153e48

[CyrilleB79]

@mltony the feature does not seem to work with Word legacy; I did not re-test with UIA.

Could you double-check that all command types work in Word with and without UIA? Let me know when you have re-tested this successfully. Thanks.

[mltony]

@CyrilleB79 - as I mentioned #16171 and #16172 are word specific problems that prevent style navigation to work correctly in MSWord. Both issues affect both legacy and UIA word support. Until both are fixed, style navigation won't work correctly in MSWord.

[CyrilleB79]

@mltony, sorry to come a bit late with it, but I had not paid attention to the following known issue:

Another quirk to be aware of: in this PR I treat paragraphs separately. For example if we have an entire document written in the same style, then jump to next same style command would iterate over paragraphs. In theory we can try to merge paragraphs and parts of paragraphs written in the same style together, but this would make code more complicated and I personally feel current approach is more intuitive, but LMK if you think otherwise.wouldn't seem very logic

My personal opinion is that merging same style across paragraph boundaries is more than desirable. For me, the main use case of this PR is : "See my comments in red the message below". In a message with many bullet paragraphs (e.g. meeting minutes), with only few comments in red, I do not expect the jump commands to jump to each bullet.

[CyrilleB79]

@CyrilleB79 - as I mentioned #16171 and #16172 are word specific problems that prevent style navigation to work correctly in MSWord. Both issues affect both legacy and UIA word support. Until both are fixed, style navigation won't work correctly in MSWord.

Re #16171: as mentioned there, word is able to read text with formatting information if NVDA is configured for this. Thus you should probably use the same technique for style navigation.

Re #16172: I have not reviewed the code. But it seems strange to me that you cannot work it around, e.g. catching the exception to indicate the end of the document. Actually Word has a browse mode with a working quick nav capability. Why is style navigation impacted and other quick nav commands not?

I hope you can fix #16171 and #16172 in this PR. Especially because Outlook uses Word browse mode on by default, and because the "See my comments in red in the message below" is a very common use case.

Though, if you confirm that you cannot fix them, you should:

• mention it in known issue section
• disable these commands in Word/Outlook so that we do not expect them to work

[XLTechie]

What @CyrilleB79 describes is the behavior I would expect also. I'm not sure how this feature is useful if it doesn't act that way.

[mltony]

@CyrilleB79, @XLTechie, I addressed your comments. Now same style chunks can span across multiple paragraphs. This made the code significantly more complicated as I warned before. Took me quite some time to debug that. Also I made this PR compatible with MS Word. I figured out how to retrieve formatting information there. Also I still think #16172 is a bug - as it shouldn't throw an error. But for now I am catching this error in my code, so this PR works as expected and is not blocked on #16172.

[CyrilleB79]

Thanks @mltony. This actually works much better in Word.

There still seems to be little issues with links of e-mails and URLs. Try to type the following: My e-mail is [email protected] and I’ll provide a link on https://github.com/nvaccess/nvda/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc. Is it OK.

Depending on where you have initially put the cursor, you get unexpected results. E.g. cursor before the link, or after its first chararacter, or in the middle, or on the penultimate character, or on the ultimate character, or just after the last character of the link, etc. I wonder if it's related to punctuation symbols or to some word strange behaviour.

[mltony]

@CyrilleB79 , I debugged this and drilled it down to #16196 which happnes to be a duplicate of #11427, which according to Sean is blocked on an external fix. It seems that the issue is only applicable to links in MSWord. So I worked around this by ignoring color of links in MSWord.

[mltony]

I updated this to use moveToCodepointOffset function - this is now ready for review. I also fixed a small bug in moveToCodepointOffsets and added more unit tests to cover that bug.