From 870ab1e647e55976eb5bb5f94faab4766d9a184f Mon Sep 17 00:00:00 2001 From: Raymond Zhao Date: Thu, 3 Feb 2022 08:40:14 -0800 Subject: [PATCH] Merge Settings editor labels into a single group (#142045) Refactor Settings editor labels to be under a single group. Also fix VoiceOver support on macOS for those labels. Fixes #107748 Fixes #141960 --- .../browser/media/settingsEditor2.css | 16 +- .../preferences/browser/settingsTree.ts | 276 ++++++++++++------ 2 files changed, 184 insertions(+), 108 deletions(-) diff --git a/src/vs/workbench/contrib/preferences/browser/media/settingsEditor2.css b/src/vs/workbench/contrib/preferences/browser/media/settingsEditor2.css index 7b326c6d0fd33..8e559cd2ae368 100644 --- a/src/vs/workbench/contrib/preferences/browser/media/settingsEditor2.css +++ b/src/vs/workbench/contrib/preferences/browser/media/settingsEditor2.css @@ -314,6 +314,10 @@ display: inline-block; /* size to contents for hover to show context button */ } +.settings-editor > .settings-body > .settings-tree-container .setting-item-contents .setting-item-title .comma { + padding-right: 4px; +} + .settings-editor > .settings-body > .settings-tree-container .setting-item-contents .setting-item-modified-indicator { display: none; } @@ -335,18 +339,8 @@ bottom: 23px; } -.settings-editor > .settings-body > .settings-tree-container .setting-item-contents .setting-item-title .setting-item-overrides { +.settings-editor > .settings-body > .settings-tree-container .setting-item-contents .setting-item-title > .misc-label { font-style: italic; - margin-right: 4px; -} - -.settings-editor > .settings-body > .settings-tree-container .setting-item-contents .setting-item-title .setting-item-ignored, -.settings-editor > .settings-body > .settings-tree-container .setting-item-contents .setting-item-title .setting-item-default-overridden { - font-style: italic; -} - -.settings-editor > .settings-body > .settings-tree-container .setting-item-contents .setting-item-title .setting-item-default-overridden { - margin-right: 4px; } .settings-editor > .settings-body > .settings-tree-container .setting-item-contents .setting-item-title .setting-item-ignored .codicon, diff --git a/src/vs/workbench/contrib/preferences/browser/settingsTree.ts b/src/vs/workbench/contrib/preferences/browser/settingsTree.ts index 799c845783347..04ee5e37a1099 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsTree.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsTree.ts @@ -586,10 +586,7 @@ interface ISettingItemTemplate extends IDisposableTemplate { descriptionElement: HTMLElement; controlElement: HTMLElement; deprecationWarningElement: HTMLElement; - otherOverridesElement: HTMLElement; - syncIgnoredElement: HTMLElement; - defaultOverrideIndicator: HTMLElement; - defaultOverrideLabel: SimpleIconLabel; + miscLabel: SettingsTreeMiscLabel; toolbar: ToolBar; elementDisposables: DisposableStore; } @@ -759,10 +756,8 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre this.ignoredSettings = getIgnoredSettings(getDefaultIgnoredSettings(), this._configService); this._register(this._configService.onDidChangeConfiguration(e => { - if (e.affectedKeys.includes('settingsSync.ignoredSettings')) { - this.ignoredSettings = getIgnoredSettings(getDefaultIgnoredSettings(), this._configService); - this._onDidChangeIgnoredSettings.fire(); - } + this.ignoredSettings = getIgnoredSettings(getDefaultIgnoredSettings(), this._configService); + this._onDidChangeIgnoredSettings.fire(); })); } @@ -770,21 +765,6 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre abstract renderElement(element: ITreeNode, index: number, templateData: any): void; - protected createSyncIgnoredElement(container: HTMLElement): HTMLElement { - const syncIgnoredElement = DOM.append(container, $('span.setting-item-ignored')); - const syncIgnoredLabel = new SimpleIconLabel(syncIgnoredElement); - syncIgnoredLabel.text = `($(sync-ignored) ${localize('extensionSyncIgnoredLabel', 'Sync: Ignored')})`; - syncIgnoredLabel.title = localize('syncIgnoredTitle', "Settings sync does not sync this setting"); - - return syncIgnoredElement; - } - - protected createDefaultOverrideIndicator(container: HTMLElement): { element: HTMLElement; label: SimpleIconLabel } { - const defaultOverrideIndicator = DOM.append(container, $('span.setting-item-default-overridden')); - const defaultOverrideLabel = new SimpleIconLabel(defaultOverrideIndicator); - return { element: defaultOverrideIndicator, label: defaultOverrideLabel }; - } - protected renderCommonTemplate(tree: any, _container: HTMLElement, typeClass: string): ISettingItemTemplate { _container.classList.add('setting-item'); _container.classList.add('setting-item-' + typeClass); @@ -795,9 +775,8 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre const labelCategoryContainer = DOM.append(titleElement, $('.setting-item-cat-label-container')); const categoryElement = DOM.append(labelCategoryContainer, $('span.setting-item-category')); const labelElement = DOM.append(labelCategoryContainer, $('span.setting-item-label')); - const otherOverridesElement = DOM.append(titleElement, $('span.setting-item-overrides')); - const { element: defaultOverrideIndicator, label: defaultOverrideLabel } = this.createDefaultOverrideIndicator(titleElement); - const syncIgnoredElement = this.createSyncIgnoredElement(titleElement); + + const miscLabel = new SettingsTreeMiscLabel(titleElement); const descriptionElement = DOM.append(container, $('.setting-item-description')); const modifiedIndicatorElement = DOM.append(container, $('.setting-item-modified-indicator')); @@ -823,10 +802,7 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre descriptionElement, controlElement, deprecationWarningElement, - otherOverridesElement, - syncIgnoredElement, - defaultOverrideIndicator, - defaultOverrideLabel, + miscLabel, toolbar }; @@ -903,37 +879,7 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre template.descriptionElement.innerText = element.description; } - template.otherOverridesElement.innerText = ''; - template.otherOverridesElement.style.display = 'none'; - if (element.overriddenScopeList.length) { - template.otherOverridesElement.style.display = 'inline'; - - const otherOverridesLabel = element.isConfigured ? - localize('alsoConfiguredIn', "Also modified in") : - localize('configuredIn', "Modified in"); - - DOM.append(template.otherOverridesElement, $('span', undefined, `(${otherOverridesLabel}: `)); - - for (let i = 0; i < element.overriddenScopeList.length; i++) { - const view = DOM.append(template.otherOverridesElement, $('a.modified-scope', undefined, element.overriddenScopeList[i])); - - if (i !== element.overriddenScopeList.length - 1) { - DOM.append(template.otherOverridesElement, $('span', undefined, ', ')); - } else { - DOM.append(template.otherOverridesElement, $('span', undefined, ')')); - } - - template.elementDisposables.add( - DOM.addStandardDisposableListener(view, DOM.EventType.CLICK, (e: IMouseEvent) => { - this._onDidClickOverrideElement.fire({ - targetKey: element.setting.key, - scope: element.overriddenScopeList[i] - }); - e.preventDefault(); - e.stopPropagation(); - })); - } - } + template.miscLabel.updateOtherOverrides(element, template.elementDisposables, this._onDidClickOverrideElement); const onChange = (value: any) => this._onDidChangeSetting.fire({ key: element.setting.key, value, type: template.context!.valueType }); const deprecationText = element.setting.deprecationMessage || ''; @@ -950,28 +896,10 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre this.renderValue(element, template, onChange); - const defaultValueSource = element.setting.defaultValueSource; - const updateSyncIgnoredElement = () => { - template.syncIgnoredElement.style.display = this.ignoredSettings.includes(element.setting.key) ? 'inline' : 'none'; - }; - const updateTitleElements = () => { - updateSyncIgnoredElement(); - template.defaultOverrideIndicator.style.display = 'none'; - if (defaultValueSource) { - template.defaultOverrideIndicator.style.display = 'inline'; - if (typeof defaultValueSource !== 'string' && defaultValueSource.id !== element.setting.extensionInfo?.id) { - const extensionSource = defaultValueSource.displayName ?? defaultValueSource.id; - template.defaultOverrideIndicator.title = localize('defaultOverriddenDetails', "Default value overridden by {0}", extensionSource); - template.defaultOverrideLabel.text = localize('defaultOverrideLabelText', "($(wrench) Overridden by: {0})", extensionSource); - } else if (typeof defaultValueSource === 'string') { - template.defaultOverrideIndicator.title = localize('defaultOverriddenDetails', "Default value overridden by {0}", defaultValueSource); - template.defaultOverrideLabel.text = localize('defaultOverrideLabelText', "($(wrench) Overridden by: {0})", defaultValueSource); - } - } - }; - updateTitleElements(); + template.miscLabel.updateSyncIgnored(element, this.ignoredSettings); + template.miscLabel.updateDefaultOverrideIndicator(element); template.elementDisposables.add(this.onDidChangeIgnoredSettings(() => { - updateSyncIgnoredElement(); + template.miscLabel.updateSyncIgnored(element, this.ignoredSettings); })); this.updateSettingTabbable(element, template); @@ -1828,9 +1756,7 @@ export class SettingBoolRenderer extends AbstractSettingRenderer implements ITre const titleElement = DOM.append(container, $('.setting-item-title')); const categoryElement = DOM.append(titleElement, $('span.setting-item-category')); const labelElement = DOM.append(titleElement, $('span.setting-item-label')); - const otherOverridesElement = DOM.append(titleElement, $('span.setting-item-overrides')); - const { element: defaultOverrideIndicator, label: defaultOverrideLabel } = this.createDefaultOverrideIndicator(titleElement); - const syncIgnoredElement = this.createSyncIgnoredElement(titleElement); + const miscLabel = new SettingsTreeMiscLabel(titleElement); const descriptionAndValueElement = DOM.append(container, $('.setting-item-value-description')); const controlElement = DOM.append(descriptionAndValueElement, $('.setting-item-bool-control')); @@ -1879,10 +1805,7 @@ export class SettingBoolRenderer extends AbstractSettingRenderer implements ITre checkbox, descriptionElement, deprecationWarningElement, - otherOverridesElement, - syncIgnoredElement, - defaultOverrideIndicator, - defaultOverrideLabel, + miscLabel, toolbar }; @@ -2146,6 +2069,121 @@ function escapeInvisibleChars(enumValue: string): string { .replace(/\r/g, '\\r'); } +/** + * Controls logic and rendering of the label next to each setting header. + * For example, the "Modified by" and "Overridden by" labels go here. + */ +class SettingsTreeMiscLabel { + private labelElement: HTMLElement; + private otherOverridesElement: HTMLElement; + private syncIgnoredElement: HTMLElement; + private defaultOverrideIndicatorElement: HTMLElement; + private defaultOverrideIndicatorLabel: SimpleIconLabel; + + constructor(container: HTMLElement) { + this.labelElement = DOM.append(container, $('.misc-label')); + this.labelElement.style.display = 'inline'; + + this.otherOverridesElement = this.createOtherOverridesElement(); + this.syncIgnoredElement = this.createSyncIgnoredElement(); + const { element, label } = this.createDefaultOverrideIndicator(); + this.defaultOverrideIndicatorElement = element; + this.defaultOverrideIndicatorLabel = label; + } + + private createOtherOverridesElement(): HTMLElement { + const otherOverridesElement = $('span.setting-item-overrides'); + return otherOverridesElement; + } + + private createSyncIgnoredElement(): HTMLElement { + const syncIgnoredElement = $('span.setting-item-ignored'); + const syncIgnoredLabel = new SimpleIconLabel(syncIgnoredElement); + syncIgnoredLabel.text = `$(sync-ignored) ${localize('extensionSyncIgnoredLabel', 'Sync: Ignored')}`; + syncIgnoredLabel.title = localize('syncIgnoredTitle', "Settings sync does not sync this setting"); + return syncIgnoredElement; + } + + private createDefaultOverrideIndicator(): { element: HTMLElement; label: SimpleIconLabel } { + const defaultOverrideIndicator = $('span.setting-item-default-overridden'); + const defaultOverrideLabel = new SimpleIconLabel(defaultOverrideIndicator); + return { element: defaultOverrideIndicator, label: defaultOverrideLabel }; + } + + private render() { + const elementsToShow = [this.otherOverridesElement, this.syncIgnoredElement, this.defaultOverrideIndicatorElement].filter(element => { + return element.style.display !== 'none'; + }); + + this.labelElement.innerText = ''; + this.labelElement.style.display = 'none'; + if (elementsToShow.length) { + this.labelElement.style.display = 'inline'; + DOM.append(this.labelElement, $('span', undefined, '(')); + for (let i = 0; i < elementsToShow.length - 1; i++) { + DOM.append(this.labelElement, elementsToShow[i]); + DOM.append(this.labelElement, $('span.comma', undefined, ',')); + } + DOM.append(this.labelElement, elementsToShow[elementsToShow.length - 1]); + DOM.append(this.labelElement, $('span', undefined, ')')); + } + } + + updateSyncIgnored(element: SettingsTreeSettingElement, ignoredSettings: string[]) { + this.syncIgnoredElement.style.display = ignoredSettings.includes(element.setting.key) ? 'inline' : 'none'; + this.render(); + } + + updateOtherOverrides(element: SettingsTreeSettingElement, elementDisposables: DisposableStore, onDidClickOverrideElement: Emitter) { + this.otherOverridesElement.innerText = ''; + this.otherOverridesElement.style.display = 'none'; + if (element.overriddenScopeList.length) { + this.otherOverridesElement.style.display = 'inline'; + const otherOverridesLabel = element.isConfigured ? + localize('alsoConfiguredIn', "Also modified in") : + localize('configuredIn', "Modified in"); + + DOM.append(this.otherOverridesElement, $('span', undefined, `${otherOverridesLabel}: `)); + + for (let i = 0; i < element.overriddenScopeList.length; i++) { + const view = DOM.append(this.otherOverridesElement, $('a.modified-scope', undefined, element.overriddenScopeList[i])); + + if (i !== element.overriddenScopeList.length - 1) { + DOM.append(this.otherOverridesElement, $('span', undefined, ', ')); + } + + elementDisposables.add( + DOM.addStandardDisposableListener(view, DOM.EventType.CLICK, (e: IMouseEvent) => { + onDidClickOverrideElement.fire({ + targetKey: element.setting.key, + scope: element.overriddenScopeList[i] + }); + e.preventDefault(); + e.stopPropagation(); + })); + } + } + this.render(); + } + + updateDefaultOverrideIndicator(element: SettingsTreeSettingElement) { + this.defaultOverrideIndicatorElement.style.display = 'none'; + if (element.setting.defaultValueSource) { + this.defaultOverrideIndicatorElement.style.display = 'inline'; + const defaultValueSource = element.setting.defaultValueSource; + if (typeof defaultValueSource !== 'string' && defaultValueSource.id !== element.setting.extensionInfo?.id) { + const extensionSource = defaultValueSource.displayName ?? defaultValueSource.id; + this.defaultOverrideIndicatorLabel.title = localize('defaultOverriddenDetails', "Default value overridden by {0}", extensionSource); + this.defaultOverrideIndicatorLabel.text = localize('defaultOverrideLabelText', "$(wrench) Overridden by: {0}", extensionSource); + } else if (typeof defaultValueSource === 'string') { + this.defaultOverrideIndicatorLabel.title = localize('defaultOverriddenDetails', "Default value overridden by {0}", defaultValueSource); + this.defaultOverrideIndicatorLabel.text = localize('defaultOverrideLabelText', "$(wrench) Overridden by: {0}", defaultValueSource); + } + } + this.render(); + } +} + export class SettingsTreeFilter implements ITreeFilter { constructor( private viewState: ISettingsEditorViewState, @@ -2298,18 +2336,29 @@ export class NonCollapsibleObjectTreeModel extends ObjectTreeModel { } class SettingsTreeAccessibilityProvider implements IListAccessibilityProvider { + constructor(private readonly configurationService: IConfigurationService) { + } + getAriaLabel(element: SettingsTreeElement) { if (element instanceof SettingsTreeSettingElement) { - const modifiedText = element.isConfigured ? localize('settings.Modified', 'Modified.') : ''; + const ariaLabelSections: string[] = []; + ariaLabelSections.push(`${element.displayCategory} ${element.displayLabel}.`); - const otherOverridesStart = element.isConfigured ? - localize('alsoConfiguredIn', "Also modified in") : - localize('configuredIn', "Modified in"); - const otherOverridesList = element.overriddenScopeList.join(', '); - const otherOverridesLabel = element.overriddenScopeList.length ? `${otherOverridesStart} ${otherOverridesList}. ` : ''; + if (element.isConfigured) { + const modifiedText = localize('settings.Modified', 'Modified.'); + ariaLabelSections.push(modifiedText); + } + + const miscLabelAriaLabel = this.getMiscLabelAriaLabel(element); + if (miscLabelAriaLabel.length) { + ariaLabelSections.push(`${miscLabelAriaLabel}.`); + } const descriptionWithoutSettingLinks = fixSettingLinks(element.description, false); - return `${element.displayCategory} ${element.displayLabel}. ${descriptionWithoutSettingLinks}. ${modifiedText} ${otherOverridesLabel}`; + if (descriptionWithoutSettingLinks.length) { + ariaLabelSections.push(descriptionWithoutSettingLinks); + } + return ariaLabelSections.join(' '); } else if (element instanceof SettingsTreeGroupElement) { return element.label; } else { @@ -2320,6 +2369,39 @@ class SettingsTreeAccessibilityProvider implements IListAccessibilityProvider { @@ -2346,7 +2428,7 @@ export class SettingsTree extends WorkbenchObjectTree { return e.id; } }, - accessibilityProvider: new SettingsTreeAccessibilityProvider(), + accessibilityProvider: new SettingsTreeAccessibilityProvider(configurationService), styleController: id => new DefaultStyleController(DOM.createStyleSheet(container), id), filter: instantiationService.createInstance(SettingsTreeFilter, viewState), smoothScrolling: configurationService.getValue('workbench.list.smoothScrolling'),