From 41a7b46b35e0a174193ab20024654a2ea5ccd6b8 Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Wed, 23 Sep 2020 12:20:19 -0500 Subject: [PATCH] Clean up settings editor aria label code #106897 --- .../preferences/browser/settingsTree.ts | 101 +++--------------- .../preferences/browser/settingsWidgets.ts | 2 + 2 files changed, 16 insertions(+), 87 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/settingsTree.ts b/src/vs/workbench/contrib/preferences/browser/settingsTree.ts index df568849718..53d1fcc26a1 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsTree.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsTree.ts @@ -690,9 +690,6 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre template.descriptionElement.innerText = element.description; } - const baseId = (element.displayCategory + '_' + element.displayLabel).replace(/ /g, '_').toLowerCase(); - template.descriptionElement.id = baseId + '_setting_description'; - template.otherOverridesElement.innerText = ''; template.otherOverridesElement.style.display = 'none'; if (element.overriddenScopeList.length) { @@ -776,68 +773,6 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre protected abstract renderValue(dataElement: SettingsTreeSettingElement, template: ISettingItemTemplate, onChange: (value: any) => void): void; - protected setElementAriaLabels(dataElement: SettingsTreeSettingElement, templateId: string, template: ISettingItemTemplate): string { - // Create base Id for element references - const baseId = (dataElement.displayCategory + '_' + dataElement.displayLabel).replace(/ /g, '_').toLowerCase(); - - const modifiedText = template.otherOverridesElement.textContent ? - template.otherOverridesElement.textContent : (dataElement.isConfigured ? localize('settings.Modified', ' Modified. ') : ''); - - let itemElement: HTMLElement | null = null; - - // Use '.' as reader pause - let label = dataElement.displayCategory + ' ' + dataElement.displayLabel + '. '; - - // Setup and add ARIA attributes - // Create id and label for control/input element - parent is wrapper div - - if (templateId === SETTINGS_TEXT_TEMPLATE_ID) { - if (itemElement = (template).inputBox.inputElement) { - itemElement.setAttribute('role', 'textbox'); - label += modifiedText; - } - } else if (templateId === SETTINGS_NUMBER_TEMPLATE_ID) { - if (itemElement = (template).inputBox.inputElement) { - itemElement.setAttribute('role', 'textbox'); - label += ' number. ' + modifiedText; - } - } else if (templateId === SETTINGS_BOOL_TEMPLATE_ID) { - if (itemElement = (template).checkbox.domNode) { - itemElement.setAttribute('role', 'checkbox'); - label += modifiedText; - // Add checkbox target to description clickable and able to toggle checkbox - template.descriptionElement.setAttribute('checkbox_label_target_id', baseId + '_setting_item'); - } - } else if (templateId === SETTINGS_ENUM_TEMPLATE_ID) { - if (itemElement = template.controlElement.firstElementChild) { - itemElement.setAttribute('role', 'combobox'); - label += modifiedText; - } - } else if (templateId === SETTINGS_OBJECT_TEMPLATE_ID) { - if (itemElement = (template).objectWidget.domNode) { - itemElement.setAttribute('role', 'list'); - label += modifiedText; - } - } else { - // Don't change attributes if we don't know what we areFunctions - return ''; - } - - // We don't have control element, return empty label - if (!itemElement) { - return ''; - } - - // Labels will not be read on descendent input elements of the parent treeitem - // unless defined as roles for input items - // voiceover does not seem to use labeledby correctly, set labels directly on input elements - itemElement.id = baseId + '_setting_item'; - itemElement.setAttribute('aria-label', label); - itemElement.setAttribute('aria-describedby', baseId + '_setting_description settings_aria_more_actions_shortcut_label'); - - return label; - } - disposeTemplate(template: IDisposableTemplate): void { dispose(template.toDispose); } @@ -1187,8 +1122,6 @@ export class SettingObjectRenderer extends AbstractSettingRenderer implements IT valueSuggester: createObjectValueSuggester(dataElement), }); - this.setElementAriaLabels(dataElement, this.templateId, template); - template.context = dataElement; } } @@ -1317,17 +1250,15 @@ export class SettingTextRenderer extends AbstractSettingRenderer implements ITre } protected renderValue(dataElement: SettingsTreeSettingElement, template: ISettingTextItemTemplate, onChange: (value: string) => void): void { - const label = this.setElementAriaLabels(dataElement, SETTINGS_TEXT_TEMPLATE_ID, template); - template.onChange = undefined; template.inputBox.value = dataElement.value; template.onChange = value => { - if (!renderValidations(dataElement, template, false, label)) { + if (!renderValidations(dataElement, template, false)) { onChange(value); } }; - renderValidations(dataElement, template, true, label); + renderValidations(dataElement, template, true); } } @@ -1404,9 +1335,6 @@ export class SettingEnumRenderer extends AbstractSettingRenderer implements ITre template.selectBox.setOptions(displayOptions); - const label = this.setElementAriaLabels(dataElement, SETTINGS_ENUM_TEMPLATE_ID, template); - template.selectBox.setAriaLabel(label); - let idx = dataElement.setting.enum!.indexOf(dataElement.value); if (idx === -1) { idx = dataElement.setting.enum!.indexOf(dataElement.defaultValue); @@ -1469,17 +1397,15 @@ export class SettingNumberRenderer extends AbstractSettingRenderer implements IT const nullNumParseFn = (dataElement.valueType === 'nullable-integer' || dataElement.valueType === 'nullable-number') ? ((v: string) => v === '' ? null : numParseFn(v)) : numParseFn; - const label = this.setElementAriaLabels(dataElement, SETTINGS_NUMBER_TEMPLATE_ID, template); - template.onChange = undefined; template.inputBox.value = dataElement.value; template.onChange = value => { - if (!renderValidations(dataElement, template, false, label)) { + if (!renderValidations(dataElement, template, false)) { onChange(nullNumParseFn(value)); } }; - renderValidations(dataElement, template, true, label); + renderValidations(dataElement, template, true); } } @@ -1572,9 +1498,6 @@ export class SettingBoolRenderer extends AbstractSettingRenderer implements ITre template.onChange = undefined; template.checkbox.checked = dataElement.value; template.onChange = onChange; - - // Setup and add ARIA attributes - this.setElementAriaLabels(dataElement, SETTINGS_BOOL_TEMPLATE_ID, template); } } @@ -1693,18 +1616,18 @@ export class SettingTreeRenderers { /** * Validate and render any error message. Returns true if the value is invalid. */ -function renderValidations(dataElement: SettingsTreeSettingElement, template: ISettingTextItemTemplate, calledOnStartup: boolean, originalAriaLabel: string): boolean { +function renderValidations(dataElement: SettingsTreeSettingElement, template: ISettingTextItemTemplate, calledOnStartup: boolean): boolean { if (dataElement.setting.validator) { const errMsg = dataElement.setting.validator(template.inputBox.value); if (errMsg) { DOM.addClass(template.containerElement, 'invalid-input'); template.validationErrorMessageElement.innerText = errMsg; const validationError = localize('validationError', "Validation Error."); - template.inputBox.inputElement.parentElement!.setAttribute('aria-label', [originalAriaLabel, validationError, errMsg].join(' ')); + template.inputBox.inputElement.parentElement!.setAttribute('aria-label', [validationError, errMsg].join(' ')); if (!calledOnStartup) { ariaAlert(validationError + ' ' + errMsg); } return true; } else { - template.inputBox.inputElement.parentElement!.setAttribute('aria-label', originalAriaLabel); + template.inputBox.inputElement.parentElement!.removeAttribute('aria-label'); } } DOM.removeClass(template.containerElement, 'invalid-input'); @@ -1923,9 +1846,13 @@ export class SettingsTree extends WorkbenchObjectTree { } }, accessibilityProvider: { - getAriaLabel() { - // TODO@roblourens https://github.com/microsoft/vscode/issues/95862 - return ''; + getAriaLabel(element: SettingsTreeElement) { + // const modifiedText = template.otherOverridesElement.textContent ? + // template.otherOverridesElement.textContent : (dataElement.isConfigured ? localize('settings.Modified', ' Modified. ') : ''); + + return (element instanceof SettingsTreeSettingElement) ? + `${element.displayCategory} ${element.displayLabel}. ${element.description}` : + element.id; }, getWidgetAriaLabel() { return localize('settings', "Settings"); diff --git a/src/vs/workbench/contrib/preferences/browser/settingsWidgets.ts b/src/vs/workbench/contrib/preferences/browser/settingsWidgets.ts index 5eaab66e56c..53c2d9b5f48 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsWidgets.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsWidgets.ts @@ -263,6 +263,7 @@ abstract class AbstractListSettingWidget extends Dispo super(); this.listElement = DOM.append(container, $('div')); + this.listElement.setAttribute('role', 'list'); this.getContainerClasses().forEach(c => this.listElement.classList.add(c)); this.listElement.setAttribute('tabindex', '0'); DOM.append(container, this.renderAddButton()); @@ -379,6 +380,7 @@ abstract class AbstractListSettingWidget extends Dispo actionBar.push(this.getActionsForItem(item, idx), { icon: true, label: true }); rowElement.title = this.getLocalizedRowTitle(item); + rowElement.setAttribute('aria-label', rowElement.title); if (item.selected && listFocused) { this.listDisposables.add(disposableTimeout(() => rowElement.focus())); -- GitLab