-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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
alert screen reader users when a full copilot response has come in #182800
Conversation
@@ -348,6 +348,7 @@ export class InteractiveListItemRenderer extends Disposable implements ITreeRend | |||
const renderValue = this.getWordsForProgressiveRender(element); | |||
isFullyRendered = !!element.renderData?.isFullyRendered; | |||
if (isFullyRendered) { | |||
alert(element.response.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might not want this tied to the rendering state, a better place for this might be in the chat service where the response is handled from the provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code pointer? I like putting it here because I know it's not a partial response - it's the final one. IE we don't want the screen reader to read "Thinking..." and putting it here avoids that @roblourens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is where we have received the full response content: https://github.com/soulshined/vscode/blob/9e9cb2adffea59fcc86c17237bab42d4fe934554/src/vs/workbench/contrib/chat/common/chatServiceImpl.ts#L401
That is when the response is received by vscode, not when it is rendered by the view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, but I'm not allowed to access import { alert } from 'vs/base/browser/ui/aria/aria';
there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also cannot be accessed by the chatModel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uggh, I don't want to move the whole world to /browser
for this... let me think about this. Just to check, what do you want to happen if the user submits a query then closes the sidebar? Or scrolls away from the response so it's not rendered?
Also should it fire when the user cancels the response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should happen independent of view state. If the user cancels it and we've already triggered the aria announcement, the user can press escape but at that pt, it's not in our control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need some new handling to do this properly, I will hack on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did #182983 which returns a new promise from the service and alerts from the widget
added
feature-request
so I remember to add this to a TPI