Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Word Navigation in UiaTextRange #3659

Merged
merged 30 commits into from
Dec 12, 2019
Merged

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Nov 21, 2019

Summary of the Pull Request

Enables support for word navigation when using an automation client (i.e.: Narrator, etc...). Specifically, adds this functionality to the UiaTextRange class. The only delimiter used is whitespace because that's how words are separated in English.

PR Checklist

  • Closes UIA Word Navigation #3161
  • CLA signed.
  • Tests added/passed
    • General Word Navigation
    • Empty Buffer Navigation
  • Requires documentation to be updated (I think not!)

Detailed Description of the Pull Request / Additional comments

General "Word Movement" Expectations

the resulting text range should include any word break characters that are present at the end of the word, but before the start of the next word. (Source)

  • If you already are on a word, getting the "next word" means you skip the word you are on, and highlight the upcoming word appropriately. (similar idea when moving backwards)

Reusing our old code

Word Expansion

Since word selection is supposed to detect word delimiters already, I figured I'd reuse that code. I moved it from TerminalCore to the TextBuffer.

Then I built on top of it by adding an optional additional parameter that decides if you want to include...

  • the delimiter run when moving forward
  • the character run when moving backwards

It defaults to false so that we don't have to care when using it in selection. But we change it to true when using it in our UiaTextRange

UiaTextRange

The code is based on character movement. This allows us to actually work with boundary conditions.

The main thing to remember here is that each text range is recorded as a MoveState. The text range is most easily defined when you think about the start Endpoint and the end Endpoint. An Endpoint is just a linear 1-dimensional indexing of the text buffer. Examples:

  • Endpoint 0 --> (0,0)

  • Endpoint 79 --> (79,0) (when the buffer width is 80)

  • Endpoint 80 -->(0,1) (when the buffer width is 80)

  • When moving forward, the strategy is to focus on moving the end Endpoint. That way, we properly get the indexing for the "next" word (this also fixes a wrapping issue). Then, we update the start Endpoint. (This is reversed for moving backwards).

  • When moving a specific Endpoint, we just have a few extra if statements to properly adjust for moving start vs end.

Hooking it up

All we really had to do is add an enum. This part was super easy :)

I originally wanted the delimiters to be able to be defined. I'm not so sure about that anymore. Either way, I hardcoded our delimiter into a variable so if we ever want to expand on it or make that customizable, we just modify that variable.

Validation Steps Performed

I'll be adding more tests for the kind of things that I was doing. If you really want to test it, I recommend you use Accessibility Insights. Click on the Terminal Control. On the bottom right, click on "Text Pattern Explore...". Then just mess around with any options that say "word" on it.

Concerns/Discussion Points

  1. How does this model expand to non-english languages? How is word navigation expected to work on CJK characters? As of now, we keep going until we hit a boundary or whitespace. Is that functionally correct?
  2. A lot of this code is repeated, unfortunately. It's repeated just enough where I want to do a refactor. HOWEVER, the same issues exist in the other code in this doc (I particularly noticed it in MoveByCharacter(). I think it might be better to do all the refactoring in one big sweep, but I'm open to discussion on this.

@ZoeyR
Copy link
Contributor

ZoeyR commented Nov 22, 2019

Wasn't there a built in word delimiter list that was going to be used?

@carlos-zamora carlos-zamora changed the base branch from dev/cazamor/acc/wil-upgrade to master November 22, 2019 17:02
@carlos-zamora carlos-zamora changed the base branch from master to dev/cazamor/acc/wil-upgrade November 22, 2019 17:02
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/acc/word-nav branch from a1e5394 to 4b72ca4 Compare November 26, 2019 03:17
@carlos-zamora carlos-zamora changed the base branch from dev/cazamor/acc/wil-upgrade to master November 26, 2019 03:17
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

so I think these are nits but maybe they're not so

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/types/UiaTextRangeBase.hpp Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 27, 2019
@ghost ghost requested a review from DHowett-MSFT December 3, 2019 14:19
@carlos-zamora
Copy link
Member Author

Yea I'm much happier about this. You mention in the PR body you're working on more tests, so I'll approve contingent on those getting added 😜

Tests got added in so we're good. Now we have...

  • moveByWord (empty buffer)
  • moveByWord (non-empty buffer)
  • moveEndpointByWord

@miniksa
Copy link
Member

miniksa commented Dec 6, 2019

Yea I'm much happier about this. You mention in the PR body you're working on more tests, so I'll approve contingent on those getting added 😜

Tests got added in so we're good. Now we have...

  • moveByWord (empty buffer)
  • moveByWord (non-empty buffer)
  • moveEndpointByWord

Update your original PR comment at the top when you add things, please.

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Show resolved Hide resolved
src/types/UiaTextRangeBase.cpp Outdated Show resolved Hide resolved
src/types/UiaTextRangeBase.cpp Show resolved Hide resolved
src/types/UiaTextRangeBase.cpp Show resolved Hide resolved
src/types/UiaTextRangeBase.cpp Outdated Show resolved Hide resolved
src/types/UiaTextRangeBase.cpp Outdated Show resolved Hide resolved
src/types/UiaTextRangeBase.cpp Show resolved Hide resolved
src/types/UiaTextRangeBase.cpp Outdated Show resolved Hide resolved
src/types/UiaTextRangeBase.hpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 6, 2019
carlos-zamora and others added 2 commits December 10, 2019 10:30
* UIA Word Navigation: allow importing custom delimiters
* Created constexpr for default word delimiters
* switched to wstring_view
- add documentation
- minor cleanup
src/buffer/out/textBuffer.cpp Show resolved Hide resolved
src/buffer/out/textBuffer.cpp Show resolved Hide resolved
src/inc/unicode.hpp Outdated Show resolved Hide resolved
src/types/UiaTextRangeBase.cpp Show resolved Hide resolved
src/types/UiaTextRangeBase.cpp Outdated Show resolved Hide resolved
src/types/UiaTextRangeBase.cpp Show resolved Hide resolved
src/types/UiaTextRangeBase.hpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 10, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 10, 2019
src/types/UiaTextRangeBase.cpp Show resolved Hide resolved
src/types/UiaTextRangeBase.hpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 11, 2019
* make member wstring (not view)
* add comment
* pascal case for DefaultWordDelimiters
@DHowett-MSFT
Copy link
Contributor

I've closed and reopened the pull request to kick the CLA bot into gear.

@carlos-zamora carlos-zamora merged commit 4b48f74 into master Dec 12, 2019
@carlos-zamora carlos-zamora deleted the dev/cazamor/acc/word-nav branch December 12, 2019 23:22
@ghost
Copy link

ghost commented Jan 14, 2020

🎉Windows Terminal Preview v0.8.10091.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UIA Word Navigation
5 participants