Terminal:SelectBox: Terminal select box should have main ARIA label

[cleidigh]

@Tyriar
Since accessibility with terminal is a focus of October, would you like me to go ahead and add this?

[Tyriar]

@cleidigh what would this proposed change do exactly? Also does it impact elsewhere the dropdown is used?

[cleidigh]

@Tyriar
The aria-label option places the main label on the select element. This label gets red before the value.
For the debug configurations:

this.selectBox = new SelectBox([], -1, contextViewService, null, { ariaLabel: nls.localize('debugLaunchConfigurations', 'Debug Launch Configurations') });

I'm trying to get all the drop downs with main labels and if possible with the correct role.
For the terminal drop down the title might be: "Open Terminals: 1 cmd" where 1 cmd is obviously the first selected terminal name.

The change is just on the terminal client side. I'm going to do the same thing for the output dropdown

[Tyriar]

@cleidigh sounds good 👍

[skprabhanjan]

@Tyriar , Can i work on this ?
if yes then some starter points would be helpful, thanks :)

[Tyriar]

@skprabhanjan I think @cleidigh was going to work on this one. He's very familiar with this component.

[cleidigh]

@skprabhanjan
cc: @Tyriar
Absolutely. Just to be clear on a couple things:

• This will probably be just a couple lines of code (but great if you want to set up your dev environment
  if you don't already have it and go through the process..)
• Timing wise, this is intertwined with some other accessibility bugs so I have to fix those for Electron oddities, you can make this sequential from that.

But I can help you through the process on @Tyriar 's behalf.
I suggest looking at the code referenced above from -

debugActionItems.ts (around line 40 - sorry I can't select code easily in github)

@IThemeService private themeService: IThemeService,
		@IConfigurationService private configurationService: IConfigurationService,
		@ICommandService private commandService: ICommandService,
		@IWorkspaceContextService private contextService: IWorkspaceContextService,
		@IContextViewService contextViewService: IContextViewService,
	) {
		this.toDispose = [];
		this.selectBox = new SelectBox([], -1, contextViewService, null, { ariaLabel: nls.localize('debugLaunchConfigurations', 'Debug Launch Configurations') });
		this.toDispose.push(this.selectBox);
		this.toDispose.push(attachSelectBoxStyler(this.selectBox, themeService, {
			selectBackground: SIDE_BAR_BACKGROUND
		}));

		this.registerListeners();
	}

The selectBox, selectBoxCustom files contain the drop-down code (for reference these are not needed to be touched)

[skprabhanjan]

@cleidigh , Thanks for the reply, I have set up my dev environment already :)
cc: @Tyriar ,
I was able to locate the usage in debugActionItems.ts where the selectBox is this.selectBox = new SelectBox([], -1, contextViewService, null, { ariaLabel: nls.localize('debugLaunchConfigurations', 'Debug Launch Configurations') }); .
The current code has ariaLabel set, but I am not entirely clear as to what it should be changed to( As issue heading says "Terminal select box should have main ARIA label" and you have quoted saying "The aria-label option places the main label on the select element" ) , I tried searching for references in other places where this is used but did not make things clear.
Could you please me on this ?
Thanks:)

[cleidigh]

@skprabhanjan
No problem.

• I gave the debugActionItems pointer as the one example SelectBox constructor call that includes the label option within the interface.
• The terminal uses SelectActionItem within actionBar.ts
• We will have to modify this to pass through the ISelectBoxOptions interface which contains the option
  "ariaLabel" which should be localized string.
• I would recommend not passing just the label string, but rather the entire interface as we will easily add more options and we won't have to modify the call chain again
• The label is for the select, descriptive accessibility name for the SelectBox or dropdown I proposed
  "Open Terminals:" - @Tyriar may have a preference or better idea.
• For the localization ID, I look for other module example strings and use a similar format approach for consistency within the module.
• If you want to be able to test accessibility labels, you should install NVDA reader for Windows or if you are on OS X you can use built-in voice over
• I still have to make the fix for the electron bug within SelectBox, I could not do that today, will do tomorrow. Without this fix the labeling addition does not get read.
• Looks like we might be 8-10 hours apart so I can review anything you do overnight the next day if I have no conflicts

Cheers

export class SelectActionItem extends BaseActionItem {
	protected selectBox: SelectBox;

	constructor(ctx: any, action: IAction, options: string[], selected: number, contextViewProvider: IContextViewProvider, selectBoxOptions?: ISelectBoxOptions) {
		super(ctx, action);

		this.selectBox = new SelectBox(options, selected, contextViewProvider, null, selectBoxOptions);

		this._register(this.selectBox);
		this.registerListeners();
	}

[Tyriar]

"Open Terminals" sounds good to me.

[cleidigh]

FYI I cannot build anything so I'm kind of down and out, cannot post the fix for Electron or anything else...
#61274

[skprabhanjan]

@cleidigh , sorry I thought debugActionItems had to be changed and hence the confusion.
cc: @Tyriar
Adding selectBoxOptions.ariaLabel = nls.localize('openTerminals', "Open Terminals"); before https://github.com/Microsoft/vscode/blob/master/src/vs/base/browser/ui/actionbar/actionbar.ts#L790 would be good ?

[cleidigh]

@skprabhanjan
Apologies, I did a bit of "brain-fart"
Because of the issue we have with Electron 2.x (recent upgrade from 1.x version ) that stopped reading labels, I created this issue forgetting I had already added the label a while ago :-D

I think "Open Terminals." will be a better label. So you can go ahead and edit the label in terminalActions.ts around line 734
Note: '.' at end of label helps to add a pause for the reader output.

		super(null, action, terminalService.getTabLabels(), terminalService.activeTabIndex, contextViewService, { ariaLabel: nls.localize('terminals', 'Terminals') });

[cleidigh]

@skprabhanjan
Thanks for the first PR !

[chrmarti]

Noticed that VoiceOver doesn't read the ARIA label when just tabbing through. Verified the label is there.

[cleidigh]

@chrmarti
Thanks, I can reproduce us as well. Battle between NVDA/voiceover/Electron continues...
I will work on a possible workaround PR.