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

Injected text accessibility #129677

Closed
isidorn opened this issue Jul 28, 2021 · 13 comments · Fixed by #129809
Closed

Injected text accessibility #129677

isidorn opened this issue Jul 28, 2021 · 13 comments · Fixed by #129809
Assignees
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues inline-completions
Milestone

Comments

@isidorn
Copy link
Contributor

isidorn commented Jul 28, 2021

Testing #129383

Here's how I think the injected text accessibility should work.
Reading the injected text every time it shows up would be very aggressive and spammy. So I suggest to have an explicit command which the users could trigger that would help them here. Here's how the command would behave:

  • once the user triggers the command the injected text is read out
  • focus is moved to the injected text hover so the user can navigate / accept a suggestion
  • once the user accepted or closed the hover, focus is back in the editor
  • command would have some keybinding. Something with a key chord should be empty

I see that Cmd + K, Cmd + I already triggers the hover. However the problem here is:

  • focus is not moved into the hover
  • the suggested insertion is not being read out

More feedback can be found here #125294

@isidorn isidorn added the accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues label Jul 28, 2021
@hediet
Copy link
Member

hediet commented Jul 28, 2021

Thanks for testing it!

focus is not moved into the hover

Hmm, are you sure?
Ctrl K, Ctrl seems to move the focus to the hover for me on windows (focus is indicated by the blue border):

recording

@isidorn
Copy link
Contributor Author

isidorn commented Jul 28, 2021

@hediet I mean for your copilot hover, not in general. Previous and next buttons are disabled in my case, maybe that is the cause of why the focus is not moved?

Screenshot 2021-07-28 at 13 52 24

@isidorn
Copy link
Contributor Author

isidorn commented Jul 29, 2021

@hediet just to clarify my idea, since you mentioned in slack
"by showing the inline suggestion in the inline suggestion hover when the screen reader mode is enabled"

I think a nicer implementation would be to always have the aria-label set, even if the screen reader is not there. Since the aria label will only be visible to screen reader users. So I would not change the actual content of the widget.

Though what you mentioned will also work, and feel free to try that approach out first.

@hediet
Copy link
Member

hediet commented Jul 29, 2021

This is my idea:
When this.accessibilityService.isScreenReaderOptimized() returns true, the inline suggestion is added to the hover:

image

  • Screen reader users are informed that an inline suggestion follows when the hover is triggered
  • When clicking on next, the hover is updated & the screenreader automatically reads the inline suggestions
  • This does not affect non-screenreader users
  • The text fixes the focus problem

What do you think?

@isidorn
Copy link
Contributor Author

isidorn commented Jul 30, 2021

I think this is a lovely solution. I really like it. Though I would still have to try it out.
The only remaining piece of the puzzle is: how to improve that the screen reader users are informed that there is a hover at all. But we can tackle that afterwards.

However I checked your PR and there are a lot of code changes for last day of endgame, and since we never supported this I suggest that we simply do this first week of debt. That way you can merge it in insiders and I can test it out.
Due to that I am moving this to August. Hope that makes sense, thanks!

@isidorn isidorn modified the milestones: July 2021, August 2021 Jul 30, 2021
@hediet
Copy link
Member

hediet commented Jul 30, 2021

Thanks! Makes a lot of sense!

@hediet
Copy link
Member

hediet commented Aug 2, 2021

Reopening, as it is unclear if the PR fully addresses this issue.

@hediet hediet reopened this Aug 2, 2021
@isidorn
Copy link
Contributor Author

isidorn commented Aug 2, 2021

@hediet I can give this a test spin. Are you happy with the current state of things so I try it out?

@hediet
Copy link
Member

hediet commented Aug 2, 2021

Yes, you can try it!

@isidorn
Copy link
Contributor Author

isidorn commented Aug 2, 2021

Oh the insiders is not yet out with your changes since we freeze insiders this week.
I will give this a spin once we release in a couple of days.

@hediet
Copy link
Member

hediet commented Aug 11, 2021

This is the outcome:

When screenreader mode is enabled and the cursor is before an inline suggestion, hitting Ctrl + K, Ctrl + I will open a hover and the screenreader will read the suggestion.
If "Tab moves focus" is set, "Tab" will then cycle through various commands regarding inline suggestions (next/previous/accept/...). "Enter" will invoke the currently selected command.

I think this addresses all points mentioned in this issue.

@hediet hediet closed this as completed Aug 11, 2021
@zersiax
Copy link

zersiax commented Aug 11, 2021

All, but one, which was already brought up by @isidorn . We are going to have to come up with a way to indicate the hover is there to begin with. It's great that the hover is accessible, but it's not going to do much good if people don't know it should be invoked, given this is a manual process.
@isidorn was any thought given yet to other line-based suggestions like lightbulbs, error indicators etc. yet in other issues? It feels to me like the best solution here would be to tackle all these at once and give CoPilot that framework to just hook into?

@hediet
Copy link
Member

hediet commented Aug 11, 2021

I opened #130565 to track how to notify screenreader users when there are inline suggestions available.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2021
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 inline-completions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@isidorn @hediet @zersiax and others