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

Make ctrl+right in accessibility mode to jump to beginning of the word #83450

Merged
merged 1 commit into from
Nov 27, 2019

Conversation

mltony
Copy link
Contributor

@mltony mltony commented Oct 28, 2019

This PR fixes #83449

Changes:
In cursorWordOperations.ts in moveWordRight() I changed the logic when wordNavigationType === WordNavigationType.WordAccessibility.

Testing performed: I tested this change manually on my computer with the following screenreaders:
NVDA 2019.2
Jaws 2019

How to test:
Enable screenreader mode in the settings. IN any text document press Ctrl+Right and make sure that the cursor jumps to the beginning of each word, not the end (old behavior).

@msftclas
Copy link

msftclas commented Oct 28, 2019

CLA assistant check
All CLA requirements met.

@isidorn
Copy link
Contributor

isidorn commented Nov 15, 2019

@mltony First of all sorry for the slow response, I was on vacation.
I have tested this and it behaves nicely that the ctrl + right jumps to the beginning of each word same as Notepad and other Win apps.
We have chosen the previous behavior becuase that is how Chrome does word navigation and we believed that would play best with NVDA, since NVDA would expect us to behave like Chrome (since VS Code is built on top of Chrome).

Did you make sure that this test case still works #28306 (comment)

Also can you check if this PR fixes the following issue #80223

fyi @Neurrone

@isidorn isidorn added accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues editor-wordnav Editor word navigation issues labels Nov 15, 2019
@mltony
Copy link
Contributor Author

mltony commented Nov 23, 2019

@isidorn, thanks for your response.

Regarding #28306:
This PR mostly leaves the correct behavior intact. The reason I say mostly is because how this PR handles cursor stops near the ends of the lines. By the way, please note that this PR makes control+left and control+right behaviour symmetric, e.g. the cursor would stop in the same places regardless if we're moving forward or back through the text, so I'll just mark those stops with the | symbol. Suppose we have the following file:

Hello
world

Then with this PR, here are all the cursor stops:

|Hello|
|world|

The big question is whetehr the cursor should stop in the end of Hello. Current version of this PR makes the stop and therefore the word Hello will be announced twice - that is only the final words in every line will be announced twice. I made things this way because the PR is simpler this way and because I couldn't convince myself that one way is better than the other. I can change the PR to remove the stops at the end of lines (perhaps except for the very final line in the document), but again, I'm not sure if this way would be more convenient - @Neurrone, what's your opinion on this?
In any case, having the cursor to stop in the beginning of every word is definitely more convenient than in the end, and at least for me, if only the final word of every line is announced twice - that's not a big deal.
If I change this PR to skip cursor stops in the end of the words, then a side effect of this would be that control+right and control+left would skip over empty lines completely, which might be confusing to some. It would skip over all the empty lines if there are multiple. So we have to choose between these two options: either have final words of every line to be announced twice, or skip over empty lines.

Regarding #80223:
The first example:

a, b

Here are the cursor stops with this PR:

|a, |b|

So If we're at the very beginning and start pressing control+right, we'd hear:

b
blank

So we would hear the word b announced, which fixes part of the problem. However, we won't hear the word a, announced - but I would argue that this is rather expected behavior,contrary to what is stated in #80223, since in the very beginning the cursor is positioned at a, and when pressing control+right we shouldn't hear this word being announced again, even though this is what's happening in the current version, since the cursor would stop in the end of this word.

Another test case in this issue:

a, /b

With my PR, cursor stops are going to be like:

|a, /|b|

According to @Neurrone, the / should rather be a part of /b word, which maybe makes some sense from logical point of view, but we are constrained here by the definition of word within NVDA, which probably depends on the definition of word in chromium. So we cannot change here what are the boundaries of NVDA words, the best thing we can do is to make sure VSCode words (in terms of accessibility control+left and control+right) are exactly the same as NVDA words, which is exactly what my PR does.

@Neurrone
Copy link

CC @derekriemer @jcsteh I wonder how feasible it would be to change NVDA's definition of a word, because this can't be the only application where it is a problem. And having to replicate NVDA's definition of a word seems to make this more complicated.

@mltony thanks a lot for thoroughly investigating this. I feel that the slight tradeoff of having the last word read twice when its at the end of the line shouldn't be a big deal, especially if the alternative is having it skip blank lines completely.

And I also agree that in the test case for a, b if the cursor is at a it should say b, which is how it works in other programs.

@jcsteh
Copy link

jcsteh commented Nov 24, 2019 via email

@isidorn
Copy link
Contributor

isidorn commented Nov 25, 2019

Thanks everyone for jumping in, great discussion.
@jcsteh the alternative to what you suggest is exactly what I tried to do - VS Code changes the word movement commands such that we navigate words as Chrome does. This should work since NVDA expects VS Code to behave like Chrome.
I believe this PR should fix the VS Code word navigation to be closed to what Chrome is doing and then NVDA should announce everythign correctly.

Let me know what you think.

@mltony if what I am saying makes sense to you can you check if your word navigation is similar to how chrome is navigating words?

@mltony
Copy link
Contributor Author

mltony commented Nov 27, 2019 via email

@isidorn
Copy link
Contributor

isidorn commented Nov 27, 2019

This looks good to me. Thanks a lot for this, really appreciate the help. Let's merge it in so we get feedback from users asap.

@isidorn isidorn merged commit 9342a27 into microsoft:master Nov 27, 2019
@isidorn isidorn added this to the November 2019 milestone Nov 27, 2019
@derekriemer
Copy link

derekriemer commented Nov 30, 2019 via email

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues editor-wordnav Editor word navigation issues
Projects
None yet
6 participants