Suggest widget set aria active descendent #87880
Conversation
| @@ -162,6 +165,7 @@ class Renderer implements IListRenderer<CompletionItem, ISuggestionTemplateData> | |||
| const data = <ISuggestionTemplateData>templateData; | |||
| const suggestion = (<CompletionItem>element).completion; | |||
|
|
|||
| data.root.id = getAriaId(_index); | |||
There was a problem hiding this comment.
no more underscore for _index? (now that it is used...)
| getAriaLabel: (item: CompletionItem) => { | ||
| if (this.expandDocsSettingFromStorage()) { | ||
| const { documentation, detail } = item.completion; | ||
| const docs = strings.format( |
There was a problem hiding this comment.
This is tricky... Usually, detail and documentation aren't defined and the UI triggers that when an item gains focus. I don't think tho that this call is sync'd on that and what you get is likely indeterministic. You should be able to call resolve on the item but you need to know how long you can "wait" for its result...
There was a problem hiding this comment.
Yes, I missed this.
However this is just working since once the suggestion details and docs get resolved the item will get re-rendered by the splice call here.
When the item gets rerendered the list asks again for the aria label and we will return deterministicly the correct aria label since the item just got resolved.
I can not wait in the getAriaLabel call since it is not async.
Idealy the method would also check if the item is resolved, if not just return the label. However not sure how to check that.
I am open for suggestions.
There was a problem hiding this comment.
Yeah, would just be nicer if we wouldn't read it twice. To know if its resolved, you can add a boolean to the CompletionItem which knows that resolving is done.
There was a problem hiding this comment.
As you suggested I have added a isResolved boolean to the CompletionItem. So when we compute the aria label if the item is not resolved we will only read the label.
Let me know what you think.
|
Once this hits Insiders', I can give you test results for NVDA on WIndows. You only tested with VoiceOver on OS X so far, correct? |
|
@zersiax correct. If everything goes well this should hit insiders some time next week, most probably Tuesday or Wednesday. Thanks a lot! |
| @@ -319,6 +319,13 @@ export interface IOverviewRuler { | |||
| setLayout(position: OverviewRulerPosition): void; | |||
| } | |||
|
|
|||
| /** | |||
| * Editor aria options. | |||
| */ | |||
There was a problem hiding this comment.
Please add @internal also here so this unused interface disappears from monaco.d.ts
| @@ -424,6 +425,18 @@ export class TextAreaHandler extends ViewPart { | |||
| return this._lastRenderPosition; | |||
| } | |||
|
|
|||
| public setAria(options: IEditorAriaOptions): void { | |||
| if (options.activeDescendent) { | |||
There was a problem hiding this comment.
The DOM spelling appears to be activedescendant vs activeDescendent. Not sure which one is correct, probably the DOM one, so we should use activeDescendant (with an a)
| * Sets the editor aria options, primarily the active descendent. | ||
| * @internal | ||
| */ | ||
| setAria(options: IEditorAriaOptions): void; |
There was a problem hiding this comment.
I'm not a huge fain of the name setAria, it feels "unfinished", maybe setAriaOptions would be better?
|
Thanks a lot for the review. |
|
@isidorn confirmed, will let you know monday or tuesday ;) |
This PR:
setAriaapi to the Editor, that gets passed to thetextAreaHandlersetAriaapi in the Suggest widget. This way the suggest widget no longer uses thearia.alertCompletionItemsand thedetails. Overall things should be simpler now in theSuggestWidgetFixes #3787
Please note that I have tested this on my macOS Catalina and it works just fine. I plan to test it on windows when I am get back to the office next week.
@alexdima let me know what you think
fyi @zersiax @jrieken
We can still fine tune what is exactly being read and if there are still issues.