Fix and test TextBuffer::MoveToPreviousWord() #7770

carlos-zamora · 2020-09-28T22:44:14Z

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 · 2020-09-28T22:54:39Z

Carlos, did this break further with #7677?

DHowett · 2020-09-28T22:56:40Z

Since this change has to make its way back into windows, I would prefer if you do not make unrelated performance changes at the same time unless they double or triple the performance during a11y. If it's so bad that it's noticeable, let's chat.

DHowett · 2020-09-28T22:58:36Z

This would also benefit from an explanation as to what the bug ius.

codeofdusk · 2020-09-28T23:03:46Z

If it's so bad that it's noticeable, let's chat.

There are noticeable performance issues with word nav, see #5243 (which might be fixed by this PR as the NVDA review cursor does expand by word).

codeofdusk · 2020-09-28T23:04:28Z

Carlos, did this break further with #7677?

Not Carlos, but I'm pretty sure that #7677 did introduce this bug.

carlos-zamora · 2020-09-28T23:09:24Z

Carlos, did this break further with #7677?

No it did not. #7677 had to do with working with a degenerate range at EndExclusive. This one has to do with moving by word.

Since this change has to make its way back into windows, I would prefer if you do not make unrelated performance changes at the same time unless they double or triple the performance during a11y. If it's so bad that it's noticeable, let's chat.

I feel that. I could leave MoveToNextWord() untouched since the bug only had an issue with MoveToPreviousWord()? I think the test I wrote failed in that state though (I'll double check if you want).

I did add extra tests to try and alleviate that concern though. And the code looks much cleaner this way. My earlier version of MoveToPreviousWord() required 4 while loops

move back through all whitespace of current word
move back through all text of current word
move back through all whitespace of previous word
move back through all text of previous word

DHowett

NAK for restructuring.

Since the last edits here introduced a regression and we're close to a Windows bug bar date, I'd like to get this in two separate PRs (bug fix, performance).

carlos-zamora · 2020-09-28T23:21:48Z

Carlos, did this break further with #7677?

Not Carlos, but I'm pretty sure that #7677 did introduce this bug.

Slapped the feature test on 9539ec3 (just before #7677). It failed then too.

codeofdusk

This PR does seem to fix #7742 with minor performance improvement to word nav (although could just be placebo)! There are still some noticeable performance issues though at the end of the output. I'll leave code comments to others.

DHowett · 2020-09-29T00:55:22Z

src/buffer/out/textBuffer.hpp

@@ -131,7 +131,7 @@ class TextBuffer final

    const COORD GetWordStart(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode = false) const;
    const COORD GetWordEnd(const COORD target, const std::wstring_view wordDelimiters, bool accessibilityMode = false) const;
-    bool MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, COORD lastCharPos) const;
+    bool MoveToNextWord(COORD& pos, const std::wstring_view wordDelimiters, const COORD lastCharPos) const;


surprised this compiles

derp. Just added it to the cpp too.

The correct way to fix this would be to back out all changes to MoveToNextWord

src/host/ut_host/TextBufferTests.cpp

DHowett

I believe I understand -- you cannot move back a word, so you do not change the coordinates.

DHowett

as noted

DHowett · 2020-09-29T17:25:48Z

@msftbot merge this in 5 minutes

ghost · 2020-09-29T17:25:52Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Tue, 29 Sep 2020 17:30:50 GMT, which is in 5 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

codeofdusk · 2020-09-29T17:59:48Z

@DHowett I think this PR requires a second review from someone with write access.

codeofdusk · 2020-09-29T18:00:08Z

Oh and pipelines are failing.

DHowett · 2020-09-29T18:01:08Z

@DHowett I think this PR requires a second review from someone with write access.

Yes, indeed. The team has been notified.

miniksa

Alright. I had to go dig into what GetWordStart did because you didn't change it in this PR. But this looks fine to get this fixed. And I'm glad to see the increased test coverage. I'll jump over to the perf one next.

DHowett · 2020-10-14T20:42:21Z

This fix was just released as part of Windows in Insider Build 20236!

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 · 2020-11-11T18:04:59Z

🎉Windows Terminal v1.4.3141.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2020-11-11T18:08:47Z

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

Fix and test a11y movement by word

b02e130

ghost added 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. labels Sep 28, 2020

DHowett requested changes Sep 28, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 28, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 28, 2020

DHowett mentioned this pull request Sep 28, 2020

UIA: crash at very end of buffer #7771

Closed

remove optimization for MoveToNextWord()

3102da7

codeofdusk approved these changes Sep 29, 2020

View reviewed changes

carlos-zamora changed the title ~~Fix and test a11y movement by word~~ Fix and test TextBuffer::MoveToPreviousWord() Sep 29, 2020

DHowett reviewed Sep 29, 2020

View reviewed changes

src/host/ut_host/TextBufferTests.cpp Show resolved Hide resolved

DHowett approved these changes Sep 29, 2020

View reviewed changes

add a missing const

9b1ee57

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Sep 29, 2020

ghost requested review from zadjii-msft, miniksa, leonMSFT and PankajBhojwani September 29, 2020 17:03

DHowett requested changes Sep 29, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 29, 2020

un-const

9d07b2c

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 29, 2020

DHowett approved these changes Sep 29, 2020

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 29, 2020

DHowett mentioned this pull request Sep 30, 2020

Add optimization to get a11y next word #7789

Merged

miniksa approved these changes Sep 30, 2020

View reviewed changes

ghost merged commit 9ec57a7 into master Sep 30, 2020

ghost deleted the dev/cazamor/a11y/move-by-word-bugfix branch September 30, 2020 18:13

carlos-zamora restored the dev/cazamor/a11y/move-by-word-bugfix branch September 30, 2020 18:20

carlos-zamora deleted the dev/cazamor/a11y/move-by-word-bugfix branch September 30, 2020 18:24

carlos-zamora restored the dev/cazamor/a11y/move-by-word-bugfix branch September 30, 2020 18:24

ghost mentioned this pull request Nov 11, 2020

UIA: incorrect move to previous word #7742

Closed

DHowett deleted the dev/cazamor/a11y/move-by-word-bugfix branch February 5, 2021 19:07

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and test TextBuffer::MoveToPreviousWord() #7770

Fix and test TextBuffer::MoveToPreviousWord() #7770

carlos-zamora commented Sep 28, 2020 •

edited by DHowett

Loading

DHowett commented Sep 28, 2020

DHowett commented Sep 28, 2020

DHowett commented Sep 28, 2020

codeofdusk commented Sep 28, 2020 •

edited

Loading

codeofdusk commented Sep 28, 2020 •

edited

Loading

carlos-zamora commented Sep 28, 2020

DHowett left a comment

carlos-zamora commented Sep 28, 2020

codeofdusk left a comment •

edited

Loading

DHowett Sep 29, 2020

carlos-zamora Sep 29, 2020

DHowett Sep 29, 2020

DHowett left a comment

DHowett left a comment

DHowett commented Sep 29, 2020

ghost commented Sep 29, 2020

codeofdusk commented Sep 29, 2020

codeofdusk commented Sep 29, 2020

DHowett commented Sep 29, 2020

miniksa left a comment

DHowett commented Oct 14, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

Fix and test TextBuffer::MoveToPreviousWord() #7770

Fix and test TextBuffer::MoveToPreviousWord() #7770

Conversation

carlos-zamora commented Sep 28, 2020 • edited by DHowett Loading

Validation Steps Performed

DHowett commented Sep 28, 2020

DHowett commented Sep 28, 2020

DHowett commented Sep 28, 2020

codeofdusk commented Sep 28, 2020 • edited Loading

codeofdusk commented Sep 28, 2020 • edited Loading

carlos-zamora commented Sep 28, 2020

DHowett left a comment

Choose a reason for hiding this comment

carlos-zamora commented Sep 28, 2020

codeofdusk left a comment • edited Loading

Choose a reason for hiding this comment

DHowett Sep 29, 2020

Choose a reason for hiding this comment

carlos-zamora Sep 29, 2020

Choose a reason for hiding this comment

DHowett Sep 29, 2020

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Sep 29, 2020

ghost commented Sep 29, 2020

codeofdusk commented Sep 29, 2020

codeofdusk commented Sep 29, 2020

DHowett commented Sep 29, 2020

miniksa left a comment

Choose a reason for hiding this comment

DHowett commented Oct 14, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

carlos-zamora commented Sep 28, 2020 •

edited by DHowett

Loading

codeofdusk commented Sep 28, 2020 •

edited

Loading

codeofdusk commented Sep 28, 2020 •

edited

Loading

codeofdusk left a comment •

edited

Loading