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

refine when accessibility signals play in code editor #204257

Closed
ramoncorominas opened this issue Feb 4, 2024 · 36 comments · Fixed by #204445 or #210679
Closed

refine when accessibility signals play in code editor #204257

ramoncorominas opened this issue Feb 4, 2024 · 36 comments · Fixed by #204445 or #210679
Assignees
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@ramoncorominas
Copy link
Contributor

Type: Bug

I'm grouping together in this single issue several issues related to audio cues and accessibility-related text notifications in the editor. This is because they appear to be closely related and likely share a common cause. It is suspected that the triggering mechanism for both the audio cue and the text alert is the same, leading to the observed problematic behavior.

In the Visual Studio Code editor, there are issues related to the audio cues that have been present in previous versions. Additionally, with the introduction of accessibility-related text notifications in v1.86, new problems have arisen. These notifications, intended for screen reader users, are triggered when the cursor encounters lines with errors, warnings, breakpoints, etc. The issues with these features are as follows:

  1. Both the audio cues and accessibility-related text notifications are triggered whenever the cursor moves within the line, such as when using Control+left/right to navigate to the previous/next word. This is particularly disruptive for the text notifications, as the repeated reading of the word "error", "warning", etc., often interrupts the normal verbalization of the screen reader. It would be more effective if these cues and notifications were only triggered when moving to a different line.

  2. There appears to be a slight delay before the accessibility-related notification is announced. If the user moves too quickly between lines, the notification is not read in sync with the corresponding line, or sometimes it is read multiple times. It would be more efficient if the notification for a line was cancelled when the user moves to another line, ensuring it is always synchronized with the appropriate line.

An advanced approach could be to have different audio cues for line-specific errors/warnings/breakpoints and "inline" errors/warnings/breakpoints. One suggestion would be to use the same sound but with different pitches for these two types of cues. This would not be necessary for certain audio cues that are only line-specific (such as folded code) or triggered by specific events (such as task audio cues).

VS Code version: Code 1.86.0 (0504748, 2024-01-31T10:28:19.990Z)
OS version: Windows_NT x64 10.0.19045
Modes:

@akash07k
Copy link

akash07k commented Feb 4, 2024

+1

@meganrogge
Copy link
Contributor

meganrogge commented Feb 5, 2024

Thanks for the issue.

Re:

There appears to be a slight delay before the accessibility-related notification is announced. If the user moves too quickly between lines, the notification is not read in sync with the corresponding line, or sometimes it is read multiple times. It would be more efficient if the notification for a line was cancelled when the user moves to another line, ensuring it is always synchronized with the appropriate line.

I have commented in #204258. This is a result of using status rather than alert. I don't think there's a way to cancel a status message.

Re:

Both the audio cues and accessibility-related text notifications are triggered whenever the cursor moves within the line, such as when using Control+left/right to navigate to the previous/next word. This is particularly disruptive for the text notifications, as the repeated reading of the word "error", "warning", etc., often interrupts the normal verbalization of the screen reader. It would be more effective if these cues and notifications were only triggered when moving to a different line.

Tracking the issue of playing the alert/audio cue repeatedly while within the line here: #188285

For now, given you find this disruptive, could you disable the alerts and keep the audio cues on?

Also cc @rperez030 and @jooyoungseo

@meganrogge meganrogge added accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues under-discussion Issue is under discussion for relevance, priority, approach labels Feb 5, 2024
@rperez030
Copy link
Contributor

rperez030 commented Feb 5, 2024

Another problem that someone brought to my attention is that, if one doesn't type very fast, you start getting alert as you type which can be distracting.

One possibility that comes to mind, and I don't know how feasible that would be, is to use delays. for example, when moving the cursor between lines, the status message would only be triggered if someone remains for, let's say, more than a second in a given line, which would indicate they are actually listening to the content and not just navigating. The delay while typing should be even longer, probably 5 seconds, which would indicate that the person stopped typing.

@meganrogge
Copy link
Contributor

meganrogge commented Feb 6, 2024

Thanks for this feedback @rperez030.

Seems like we should probably disable the in line audio cues (warning, error, debugger on breakpoint) until this is sorted out. I don't think it will be a quick fix. Do you think these should be disabled as a candidate @rperez030 or is disabling them in insider's sufficient?

@meganrogge meganrogge added the bug Issue identified by VS Code Team member as probable bug label Feb 6, 2024
@meganrogge
Copy link
Contributor

I am leaning toward just fixing this in insider's. Users can disable the settings if they find them disruptive.

@meganrogge
Copy link
Contributor

I think using status is somewhat of an issue here because of the inherently dicey timing component. We don't currently have a way to cancel that.

@krperry
Copy link

krperry commented Feb 6, 2024

I agree with some of the above, but the real problem is the messages don’t make since. If you are sighted and you have a red line under two characters you know there is a problem, there. If you are blind and the line says error and nothing else, how is a screen reader user supposed to know where that error is? I have been coding long enough that I can figure it out but if this is supposed to help people find errors quickly. The current method is not helping blind people find the error it is only helping find the line. I might be wrong because I am 100% blind and the last time, I could see to code was with THE Commodores 20 and 64, and the Atari 800XL But if I made this feature for python and someone typed the following:

df my_function():

Now if I move to the line, I get an Error and a warning. I have no idea what the warning is and if I am a new python programmer, I have no idea what the error is. Let’s say my cursor is on my_function and I arrow left and right. It should not keep saying errors and warning that is true because hopefully it is clear that the error is on the df. If I arrow over the part of the code that has the red underline or whatever it is then it should say error and or beep, but it shouldn’t say error unless I am arrowing on the error, or I arrow up and down on to the line with error. That way I can move around without hearing a lot of repeated warnings and errors that make no since and make it hard for a lesser skilled screen reader user to even read the code.

If you can’t tell what the error is like the whole line is redlined, then it should only say error or warning once when I arrow to that line or that line is focused by jumping to it by a goto or search. It should only beep once as well.

The error messages should also be off by default. Having the sounds on by default is fine but having the messages on by default is just too much.

There is another problem too. As I backspace it says error and warning a lot. I am sure that is a part of this same problem.

I personally think that the error beep should not be at 300 milliseconds by default when you stop typing. So many coders who are blind and use this use this to learn coding as well as code on a regular basis. For those of us who type fast enough and think in code 300 MS is fine but for many people who are learning and using code completion get more error reports because they don’t complete a line of code fast enough. I think the default should be closer to 3 seconds before it starts alerting you that you have done something wrong. This gives a blind person time to review their line of code and it will help if they are just making a correction and get rid of the beep, beep, beep, that we get right now.
If someone wants to turn the speed up to 300 MS then more power to them but as a default right now the error messages beep way too often as you try to complete a line of code.

The error beeps and messages if on should be quick when you land on a line but not while you’re typing.

Remember when it all comes down to it this is just an indicator that there is something wrong. If a person wants to know what is wrong as a blind user, they will do CTRL+SHIFT+m to get the messages. If it read out what the error was rather than just error, error, error, maybe it would help but even that could be too annoying. It is better to indicate and then let the user press a key to read what is being indicated. That way if a person needs more, they can activate more information. If you give them too much, they can’t get rid of it as easily. It is like too much cold is better than too much hot. I can put on a lot of wintry weather gear to stay warm but if I must take off all my clothes to stay cool people are going to go nuts. If a person who is not extra skilled with a screen reader hears too much, they go nuts . If they get indications that there is a problem and can ask for more help, then that makes for a powerful tool.

All these things should have settings which so far, they do. The problem is many of the default settings right now are both good and bad Speed for beep bad, number of messages bad. Having error beeps on by default good. Easy to get to errors and back good. WE want information but be as a sighted person would not want their whole screen flashing bright red for each two character error. We don’t’ want to hear to much. We do need to be able to know what is being indicated though.

@meganrogge
Copy link
Contributor

meganrogge commented Feb 6, 2024

@krperry thanks for your thoughts, I consulted with screen reader users before enabling this by default.

I'm happy to revisit that though, as I indicated above, since it seems there are some issues with the experience that will be addressed over time.

@krperry
Copy link

krperry commented Feb 6, 2024 via email

@meganrogge meganrogge added the candidate Issue identified as probable candidate for fixing in the next release label Feb 6, 2024
meganrogge added a commit that referenced this issue Feb 6, 2024
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Feb 6, 2024
@meganrogge meganrogge reopened this Feb 6, 2024
@VSCodeTriageBot VSCodeTriageBot removed the insiders-released Patch has been released in VS Code Insiders label Feb 6, 2024
@rperez030
Copy link
Contributor

Thanks for this feedback @rperez030.

Seems like we should probably disable the in line audio cues (warning, error, debugger on breakpoint) until this is sorted out. I don't think it will be a quick fix. Do you think these should be disabled as a candidate @rperez030 or is disabling them in insider's sufficient?

Given the feedback we're receiving, I would feel inclined for a candidate.

I still believe having them on by default is a good idea, but at least I could not anticipate the disruption during inline cursor navigation, and especially while typing.

@rperez030
Copy link
Contributor

I agree with some of the above, but the real problem is the messages don’t make since. If you are sighted and you have a red line under two characters you know there is a problem, there. If you are blind and the line says error and nothing else, how is a screen reader user supposed to know where that error is?

@krperry I have the same problem that you have in this case since I also don't see what is being communicated visually, but I suspect this is somewhat ambiguous to everyone. Using your example, if I arrow over the df part of the code I get a warning. If I then use the shortcut to reveal the tooltip (CTRL +K followed by CTRL +I), I get the following: ""df" is not definedPylancereportUndefinedVariable(function) df: AnyView Problem (Alt+F8)". in other words, Pylance believe df could be an undefined variable and it is triggering a warrning, not an error, so although not verry helpful, it seems like we are getting the right alert. If I then move to the name of the function, I get an error and a warning combine, and the tooltip reveals the following: ""df" is not definedPylancereportUndefinedVariableStatements must be separated by newlines or semicolonsPylance"my_function" is not definedPylancereportUndefinedVariable(function) my_function: AnyView Problem (Alt+F8)". In other words, we are still carrying the warning from the first portion of the line + what it believes is another statement in the same line, which is treated like an error. Helpful or not, this leads me to believe we are receiving the same information via the alerts that a sighted user is receiving visually.

I also can see how all of this wouldn't make any sense to someone who is getting started, whether blind or sighted, but a blind student would have to deal with the overhead of having to figure out the UI itself and the interaction with the screen reader. In that case, it probably make sense to suggest the beginner user to disable the alerts and rely on the problems pane.

To me, there are 3 problems here that need addressing:

  1. The same alert is repeated as I navigate for every character. It should only be announced when it changes, for example, when I move to the second statement in the above example.
  2. alerts shouldn't be announced for the wrong line. If I add the following code, and I move quickly from top to bottom with the down arrow key, NVDA will announce error for the second line when it shouldn't.:
df my_function():
    print ("Hello from a function")

  1. Alerts shouldn't be fired when the user is supposed to be typing or it would cause disruption.
    Point 2 could be addressed by the use of alerts, but alerts are handled in different ways by voiceover and Windows based screen readers. As we saw when we were testing the first version of this, Voiceover cancels the read out of the line when the alert is fired. Using status on macOS and alert on Windows might be a solution, but even that could potentially be disrupted for someone who relies on braille only since the alert will be triggered when the cursor reach the line anyway. I only use Braille for support and certainly know when navigating a file quickly, so it wouldn't affect me. To me, the ideal solution for point 2 could be using status, but managing the time when the alert is fired. Another possibility would be not to have alerts at all during inline navigation. I personally don't need them at all in that case, but perhaps I'm just used to not having them and figurint out what is wrong by reading the line.
    Point 3 could be addressed by waiting until the user is idle for a few seconds after typing alphanumeric keys to fire aan alert given that it is possible, or by not firing alerts at all when entering characters in the editor.

I'm sure we'll sort all of this out in time.

@krperry
Copy link

krperry commented Feb 6, 2024 via email

@krperry
Copy link

krperry commented Feb 6, 2024 via email

@meganrogge meganrogge changed the title Problematic Behavior of Audio Cues and Accessibility-Related Text Notifications in Code Editor refine when accessibility signals play in code editor Mar 27, 2024
@meganrogge
Copy link
Contributor

Meeting notes created with @hediet

if a signal is played, it means there is a signal at that position or on the line (at this moment)
- a user changes lines to one that contains a marker (after x debouncing time, per modality, has occurred)
- a user changes columns to one that contains a marker (after y debouncing time, per modality, has occcurred)
- a marker appears (after x debouncing time, per modality, has occurred)

- line changes have to be debounced by line change, not by position change
- when column changes, cancel debounced signal unless line has changed (typing)

- what happens when you change lines and land in a column with a marker? 
	- different sounds?

right now we're debouncing position change. we don't clear when the signal is no longer present
we should debounce signal being played instead

@meganrogge
Copy link
Contributor

meganrogge commented Apr 2, 2024

@jooyoungseo and @rperez030 , we are considering allowing line and column change accessibility signals from being debounced separately.

When a user navigates between lines and lands in a column with a marker (for example error, if the line and column related signals are debounced separately, this means the sound could play twice.

I'm wondering if we should have separate sounds / announcements to indicate:

  1. landing on a line, not on a column, with an error
  2. landing on a line and column with an error

@rperez030
Copy link
Contributor

a signal should not play twice, even if the user happens to land in a place where both the line and the column has a marker. That wouldn't make sense from a ux perspective in my opinion. For a user that relies on speech, when navigating by line with the up or down arrow keys, the position of the cursor within the line doesn't matter, unless the user is going to take an action on that particular line. For a screen reader user that relies on braille, it doesn't matter either because a braille display can only display one flash message at a time.

@meganrogge
Copy link
Contributor

meganrogge commented Apr 3, 2024

Cases:

x = debounce of line change signal
y = debounce of column change signal
E = marker of a certain type, like error

  • change line
    • E marker on line, not column
      • play signal E after x
    • E marker on line and column
      • x = y, only one sound will play
      • x < y, cancel debouncing of y for event E, play signal E after x
      • x > y, cancel debouncing of x for event E, play signal E after y
  • stay on same line, change column
    • E marker on line, not column
      • do nothing
    • E marker on column
      • play signal E after y

Note that inherent in this "play signal E" notion is that the position has not changed by the time that occurs. If the position has changed, we will cancel all debounced events.

@meganrogge
Copy link
Contributor

meganrogge commented Apr 3, 2024

We will need to test to see if aria.alert should be used instead of aria.status. The latter is queued and I don't think there's a way to cancel it, so the position could have changed by the time that it's announced.

The reason we switched to that was characters in the editor when typing were not being read as that announcement was getting overridden by the aria.alert.

@rperez030
Copy link
Contributor

Since the aria announcements were disruptive when typing when using aria.alert, we used aria.status as a bandaid, suboptimal fix. They were also disabled by default for line events. #204445

This is a problem because means the announcement might occur after some delay, when it's no longer relevant.

As such, once we have the separate debouncing of line/column change, we should move over to aria.alert.

aria.alert could potentially interrupt the reading of the current line in macOS. What about the use of a delay system as proposed in PR #205320? That sounds like a more definitive solution to the problem of managing signals to be, given that we could incorporate that work.

@meganrogge
Copy link
Contributor

Next, there is the difference between errors, warnings, etc. and breakpoints, folded areas, etc.

What should we call these?

Diagnostics: errors, warnings, etc.
Landmarks: breakpoints, folded areas, etc.

@meganrogge
Copy link
Contributor

@rperez030 yes, we're investigating doing something like that. However, isn't it still true that an aria.status announcement is queued, so the position could change and outside of our control, that would play erroneously?

@rperez030
Copy link
Contributor

@rperez030 yes, we're investigating doing something like that. However, isn't it still true that an aria.status announcement is queued, so the position could change and outside of our control, that would play erroneously?

status messages are queued, but if the user does something such as moving around or typing before the status message is announced, the queue is overridden. Playing audio cues when they are no longer relevant I think is more of a concern for me.

@meganrogge
Copy link
Contributor

good to know, so seems we can stick with using aria.status then.

hediet added a commit that referenced this issue Apr 18, 2024
hediet added a commit that referenced this issue Apr 18, 2024
…eatures to TextProperties. (#210679)

* Fixes #204257. Refactors AccessibilitySignalService and renames LineFeatures to TextProperties.

* Improves typings.,
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Apr 18, 2024
@hediet
Copy link
Member

hediet commented Apr 22, 2024

@meganrogge can you verify?

@hediet hediet added the verification-needed Verification of issue is requested label Apr 22, 2024
@krperry
Copy link

krperry commented Apr 23, 2024

This all seems to be working better. I stil have to tell students when they want to make the delay when the error sounds are played they have to go to settings and sett the first itme which is I think Hover delay to something like 1 to 5 seconds. They can't just find that setting because it doesn't say "this slows down how often accessibility sounds happen". At least the setting can be set though. All the other stuff in this issue works so much better. The only one that is still a bit anoying is if you have an error in a line and you are typing to fix the error while in the error. Every key press beeps like the error. I am not sure there is much you can do about that and once you fix the error it stops which is great. I would consider this issue is fixed.

@hediet
Copy link
Member

hediet commented Apr 23, 2024

Every key press beeps like the error

That is odd, it should be delayed by 1 second.

@rperez030
Copy link
Contributor

A 1-second delay, in my opinion, does not significantly improve user experience. Ideally, this delay should be configurable, with a default setting of at least 3 seconds. setting editor.hover.delay is a viable workaround; however, it falls short in addressing the issue when an error or warning state has already been triggered.

@meganrogge meganrogge added the verified Verification succeeded label Apr 24, 2024
@meganrogge
Copy link
Contributor

@rperez030 thanks for that feedback. @hediet what do you think about making this configurable?

@hediet
Copy link
Member

hediet commented Apr 25, 2024

@hediet what do you think about making this configurable?

I'm all up for making the delays configurable! Can you add the settings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
8 participants