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.

Sign up for GitHub

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

Jump to bottom

UIA: incorrect move to previous word #7742

Closed
codeofdusk opened this issue Sep 25, 2020 · 5 comments · Fixed by #7770
Closed

UIA: incorrect move to previous word #7742

codeofdusk opened this issue Sep 25, 2020 · 5 comments · Fixed by #7770
Assignees
@carlos-zamora
Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Windows vNext

Comments

@codeofdusk
Copy link
Contributor

codeofdusk commented Sep 25, 2020 •
edited by DHowett
Loading

Since #7677 was merged:

In the NVDA Python console (NVDA+control+z):

Word

>>> # In a Word document (accessed VIA UIA, positioned on the start of the text "My name is Carlos"):
>>> ti=nav.makeTextInfo("caret")
>>> ti.expand("word")
>>> ti.text
'My '
>>> ti.collapse() # Make a degenerate range (move end to start)
>>> ti.move("word", 1)
1
>>> ti.expand("word")
>>> ti.text
'name '
>>> ti.collapse()
>>> ti.move("word", -1)
-1
>>> ti.expand("character")
>>> ti.text
'M' # GOOD

conhost

>>> # Now, in Conhost
>>> ti=nav.makeTextInfo("caret")
>>> ti.expand("word")
>>> ti.text
'C:\\Users\\codeofdusk>My '
>>> ti.collapse() # Make a degenerate range (move end to start)
>>> ti.move("word", 1)
1
>>> ti.expand("word")
>>> ti.text
'name '
>>> ti.collapse()
>>> ti.move("word", -1)
-1
>>> ti.expand("character")
>>> ti.text
' ' # BAD

Cc @carlos-zamora.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Sep 25, 2020
@carlos-zamora carlos-zamora added Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. and removed Needs-Tag-Fix Doesn't match tag requirements labels Sep 25, 2020
@carlos-zamora carlos-zamora self-assigned this Sep 25, 2020
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Sep 25, 2020
@DHowett DHowett added this to the Windows vNext milestone Sep 25, 2020
@DHowett DHowett added the Priority-1 A description (P1) label Sep 25, 2020
@codeofdusk
Copy link
Contributor Author

@DHowett Any chance this could be 21H1 instead?

@DHowett
Copy link
Member

DHowett commented Sep 25, 2020

I split up your codeblock and introduced section headers because I got a bit confused when reading it.

@DHowett
Copy link
Member

DHowett commented Sep 25, 2020 •
edited
Loading

@codeofdusk I renamed the "21H1" milestone to "Windows vNext". They mean the same thing, but the new name doesn't use our internal version designators or make any guarantees about when it's going to be released. 😄

@carlos-zamora carlos-zamora mentioned this issue Sep 28, 2020
Merged
@ghost ghost added the In-PR This issue has a related PR label Sep 28, 2020
@ghost ghost closed this as completed in #7770 Sep 30, 2020
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Sep 30, 2020
ghost pushed a commit that referenced this issue Sep 30, 2020
@carlos-zamora
Fix and test TextBuffer::MoveToPreviousWord() (#7770)
9ec57a7 
This fixes a bug when moving backwards by word that resulted in #7742.

This also includes...
- a minor refactor that leverages `GetWordStart` in `MoveToPreviousWord`
- additional unit tests for movement by word
- a feature test comprised of the referenced bug report

`MoveToPreviousWord()` would...
- move backwards for each whitespace character
- then, move backwards for each regular character

This would actually result in moving to the beginning of the current "word" (as defined by a11y).

We actually need to do this process twice:
- the first time gets you to the beginning of the current word
- attempt to move back by one character
- the second time gets you to the beginning of the previous word

Rather than implementing 4 while loops, we leverage `GetWordStart()` to
attempt to move to the beginning of the previous word. We call it twice
(as described above). The logic is unchanged, but we instead reuse a
function that has already undergone more testing.

To make sure this works as expected, additional unit tests were
introduced covering "MoveByWord" in the TextBuffer.

## Validation Steps Performed
Added test for repro steps.
Added unit tests for movement by word.

Closes #7742
DHowett pushed a commit that referenced this issue Oct 19, 2020
@carlos-zamora @DHowett
Fix and test TextBuffer::MoveToPreviousWord() (#7770)
c529789 
This fixes a bug when moving backwards by word that resulted in #7742.

This also includes...
- a minor refactor that leverages `GetWordStart` in `MoveToPreviousWord`
- additional unit tests for movement by word
- a feature test comprised of the referenced bug report

`MoveToPreviousWord()` would...
- move backwards for each whitespace character
- then, move backwards for each regular character

This would actually result in moving to the beginning of the current "word" (as defined by a11y).

We actually need to do this process twice:
- the first time gets you to the beginning of the current word
- attempt to move back by one character
- the second time gets you to the beginning of the previous word

Rather than implementing 4 while loops, we leverage `GetWordStart()` to
attempt to move to the beginning of the previous word. We call it twice
(as described above). The logic is unchanged, but we instead reuse a
function that has already undergone more testing.

To make sure this works as expected, additional unit tests were
introduced covering "MoveByWord" in the TextBuffer.

## Validation Steps Performed
Added test for repro steps.
Added unit tests for movement by word.

Closes #7742

(cherry picked from commit 9ec57a7)
@ghost
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #7770, which has now been successfully released as Windows Terminal v1.4.3141.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #7770, which has now been successfully released as Windows Terminal Preview v1.5.3142.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlos-zamora carlos-zamora

Labels
Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Windows vNext
Development

Successfully merging a pull request may close this issue.

Fix and test TextBuffer::MoveToPreviousWord() microsoft/terminal
3 participants
@DHowett @codeofdusk @carlos-zamora