Ensure documents always scroll to exact browse mode position by michaelDCurran · Pull Request #12803 · nvaccess/nvda

michaelDCurran · 2021-09-03T00:29:36Z

Link to issue number:

Summary of the issue:

When reading / navigating with browse mode in Microsoft Word via UI Automation, the document does not always scroll so that the current browse mode position is visible on screen, nor does the focus mode caret position stay in sync with the browse mode caret, especially when moving across pages in browse mode.
Browse mode currently only knows how to scroll to objects in the document, not specific text range positions. Thus, it is only able to scroll to lists, tables and graphics, and not any arbitrary text.
Also, setting the caret in Microsoft Word to a range that is on a page that is currently off screen seems to fail and or set the caret to a random point on the current page.

Description of how this pull request fixes the issue:

Add a scrollIntoView method on textInfos.TextInfo which should croll the document containing that range such that the range is visible on screen. The base implementation has no implementation and does nothing.
Implement ScrollIntoView for IAccessible2 textInfos, UIA textInfos, Microsoft Word object model textInfos, and virtualBuffer textInfos. The VirtualBuffer textInfo implementation comes straight from the old BrowseMode logic which is to call ScrollIntoView on the deepest object at the position in the document.
Change BrowseModeDocument's _set_selection to call ScrollIntoView on the given textInfo, rather than the deepest object at the position of that textInfo. This is much more accurate and will ensure that any arbitrary textRange is scrolled to. It is specifically this change, plus the implementation of ScrollIntoView for UIA TextInfo that solves the Microsoft Word with Browse mode via UI Automation scrolling issue.

Testing strategy:

Try build: https://ci.appveyor.com/api/buildjobs/bsjf2a8c6t9879wf/artifacts/output%2Fnvda_snapshot_try-i9611-23673%2C38a7b5ac.exe
@Qchristensen has confirmed this pr fixes #9611 and that it does not cause regressions in any other browse mode implementation (Firefox, Chrome etc).

Known issues with pull request:

None known.

Change log entries:

Bug fixes:

Reading / navigating with browse mode in Microsoft Word via UI automation now ensures the document is always scrolled so that current browse mode position is visible, and that the caret position in focus mode correctly reflects the browse mode position. (Focus movement changing between focus and browse mode with UIA in Word 365 #9611)

Code Review Checklist:

Pull Request description is up to date.
Unit tests.
System (end to end) tests.
Manual testing.
User Documentation.
Change log entry.
Context sensitive help for GUI changes.
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers

lukaszgo1

There are some unrelated changes in this PR. Can it be rebased on master?

lukaszgo1 · 2021-09-04T12:59:19Z

+			self.obj.WinwordWindowObject.ScrollIntoView(self._rangeObj, True)
+		except COMError:
+			log.exception("Can't scroll")
+			pass


This line is unnecessary.

… possible. Specifically: * TextInfos.TextInfo: Added ScrollIntoView method (no default implementation) * Implemented ScrollIntoView for UIA, IAccessible2, MS Word (object model), base VirtualBuffer class. * treeInterceptorHandler.RootProxyTextInfo: expose ScrollIntoView method which forwards to the innerTextInfo's implementation. * BrowseModeDocument._set_selection: Call ScrollIntoView on the given textInfo, rather than its deepest object. this is more accurate. # Please enter the commit message for your changes. Lines starting

…ther than error as this could happen a lot in Outline view apparently.

AppVeyorBot · 2021-09-05T23:48:54Z

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/8f645bfilqnxlqmp/artifacts/output/nvda_snapshot_pr12803-23683,3fbcb020.exe

See test results for failed build of commit 3fbcb02018

seanbudd · 2021-09-09T06:02:53Z

+		if not obj:
+			return
+		if obj == self.obj.rootNVDAObject:
+			# However never scroll to the  document itself
+			return


Should these cases be logged? if not obj used to be

…og a debug warning if there is an invalid NVDAObject at start.

michaelDCurran · 2021-09-09T06:10:57Z

I added back the logging for `not obj` but, the equal to rootNVDAObject should not be logged as this is very common and deliberately should be ignored.

LeonarddeR · 2021-09-09T07:23:12Z

Ah, a pity I missed this. There seems to be some overlap with #9919 which I abandoned. Some thoughts:

This pr does not implement scrolling for MSHTML. MSHTML does support scrolling at the text range level, though.
I implemented an allignToTop parameter in Improve scrolling experience in browse mode #9919 since most API's support specifying this. It was used in browse mode because otherwise, some implementations (I don't remember which) tent to scroll all the time, even when not necessary.
IAccessible2 ScrollSubstringTo sometimes fails when the object is entirely out of view.

@michaelDCurran I assume you didn't know about #9919. May be it could be helpful to have a look at it and see what can be incorporated?

michaelDCurran · 2021-09-09T07:34:17Z

Ah sorry, I totally missed #9919. It seems to be more complete. I'll take a proper look and decide what to do.

michaelDCurran · 2021-09-09T08:41:01Z

@LeonarddeR Although #9919 is much more complete, I believe this pr is enough of a subset for what is required for MS Word with UIA to scroll, without breaking anything else.
True that MSHTML is not implemented, but currently nothing would call it anyway. Any browse mode implementation we currently have for MSHTML is a virtualBuffer.
I'd be happy to take a look at what can be taken from #9919 at a later date. But for now I'd only be interested in real-world regressions caused by this PR.
I think the only one that might exist is scrolling in Amazon Kindle for PC -- this is the only browse mode implementation that uses IAccessible2 directly, and it is possible scrollToSubstring may have the limitation you noted in #9919. Though that would need to be tested.

…12803)" This reverts commit f5e5514.

…12803)" (#12828) This reverts commit f5e5514.

lukaszgo1 reviewed Sep 4, 2021

View reviewed changes

michaelDCurran force-pushed the i9611 branch from 38a7b5a to 113af59 Compare September 5, 2021 22:15

Remove unneeded pass. and log scrollIntoView error at debugWarning ra…

9b121c8

…ther than error as this could happen a lot in Outline view apparently.

michaelDCurran force-pushed the i9611 branch from 60a120b to 9b121c8 Compare September 5, 2021 23:08

michaelDCurran added 2 commits September 9, 2021 13:05

Merge branch 'master' into i9611

16c2956

Update what's new

bccee00

michaelDCurran marked this pull request as ready for review September 9, 2021 05:53

michaelDCurran requested a review from a team as a code owner September 9, 2021 05:53

michaelDCurran requested a review from seanbudd September 9, 2021 05:53

seanbudd approved these changes Sep 9, 2021

View reviewed changes

seanbudd requested changes Sep 9, 2021

View reviewed changes

virtualBuffers.VirtualBufferTextInfo's scrollIntoView method: again l…

551ed2a

…og a debug warning if there is an invalid NVDAObject at start.

seanbudd approved these changes Sep 9, 2021

View reviewed changes

michaelDCurran merged commit f5e5514 into master Sep 9, 2021

michaelDCurran deleted the i9611 branch September 9, 2021 06:53

nvaccessAuto added this to the 2021.3 milestone Sep 9, 2021

michaelDCurran mentioned this pull request Sep 9, 2021

Allow the Focus Mode cursor and Brows Mode Cursor to sync in HTML editable applications #10610

Closed

michaelDCurran added a commit that referenced this pull request Sep 9, 2021

Revert "Ensure documents always scroll to exact browse mode position (#…

f78ca00

…12803)" This reverts commit f5e5514.

michaelDCurran mentioned this pull request Sep 9, 2021

Revert "Ensure documents always scroll to exact browse mode position" #12828

Merged

michaelDCurran added a commit that referenced this pull request Sep 9, 2021

Revert "Ensure documents always scroll to exact browse mode position (#…

4a8593d

…12803)" (#12828) This reverts commit f5e5514.

michaelDCurran mentioned this pull request Sep 9, 2021

Browse mode in MS Word with UIA: ensure scrolling and carret position are always synched #12829

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Ensure documents always scroll to exact browse mode position#12803

Ensure documents always scroll to exact browse mode position#12803
michaelDCurran merged 5 commits into
masterfrom
i9611

michaelDCurran commented Sep 3, 2021 •

edited

Loading

Uh oh!

lukaszgo1 left a comment

Uh oh!

lukaszgo1 Sep 4, 2021

Uh oh!

AppVeyorBot commented Sep 5, 2021

Uh oh!

seanbudd Sep 9, 2021

Uh oh!

michaelDCurran commented Sep 9, 2021 via email

Uh oh!

LeonarddeR commented Sep 9, 2021 •

edited

Loading

Uh oh!

michaelDCurran commented Sep 9, 2021 via email

Uh oh!

michaelDCurran commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

michaelDCurran commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

lukaszgo1 left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 Sep 4, 2021

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Sep 5, 2021

Uh oh!

seanbudd Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

michaelDCurran commented Sep 9, 2021 via email

Uh oh!

LeonarddeR commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelDCurran commented Sep 9, 2021 via email

Uh oh!

michaelDCurran commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

michaelDCurran commented Sep 3, 2021 •

edited

Loading

LeonarddeR commented Sep 9, 2021 •

edited

Loading