Clean up tabindex hacking to control tab behavior on settings editor #106897

src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts

@@ -42,7 +42,7 @@ import { IEditorMemento, IEditorOpenContext, IEditorPane } from 'vs/workbench/co
 import { attachSuggestEnabledInputBoxStyler, SuggestEnabledInput } from 'vs/workbench/contrib/codeEditor/browser/suggestEnabledInput/suggestEnabledInput';
 import { SettingsTarget, SettingsTargetsWidget } from 'vs/workbench/contrib/preferences/browser/preferencesWidgets';
 import { commonlyUsedData, tocData } from 'vs/workbench/contrib/preferences/browser/settingsLayout';
-import { AbstractSettingRenderer, ISettingLinkClickEvent, ISettingOverrideClickEvent, resolveExtensionsSettings, resolveSettingsTree, SettingsTree, SettingTreeRenderers, updateSettingTreeTabOrder } from 'vs/workbench/contrib/preferences/browser/settingsTree';
+import { AbstractSettingRenderer, ISettingLinkClickEvent, ISettingOverrideClickEvent, resolveExtensionsSettings, resolveSettingsTree, SettingsTree, SettingTreeRenderers } from 'vs/workbench/contrib/preferences/browser/settingsTree';
 import { ISettingsEditorViewState, parseQuery, SearchResultIdx, SearchResultModel, SettingsTreeElement, SettingsTreeGroupChild, SettingsTreeGroupElement, SettingsTreeModel, SettingsTreeSettingElement } from 'vs/workbench/contrib/preferences/browser/settingsTreeModels';
 import { settingsTextInputBorder } from 'vs/workbench/contrib/preferences/browser/settingsWidgets';
 import { createTOCIterator, TOCTree, TOCTreeModel } from 'vs/workbench/contrib/preferences/browser/tocTree';
@@ -677,7 +677,6 @@ export class SettingsEditor2 extends EditorPane {
 			}
 
 			this.settingsTreeScrollTop = this.settingsTree.scrollTop;
-			updateSettingTreeTabOrder(this.settingsTreeContainer);
 
 			// setTimeout because calling setChildren on the settingsTree can trigger onDidScroll, so it fires when
 			// setChildren has called on the settings tree but not the toc tree yet, so their rendered elements are out of sync
@@ -702,11 +701,17 @@ export class SettingsEditor2 extends EditorPane {
 				return;
 			}
 
+			if (this.treeFocusedElement) {
+				this.treeFocusedElement.tabbable = false;
+			}
+
 			this.treeFocusedElement = element;
-			this.settingsTree.setSelection(element ? [element] : []);
 
-			// Wait for rendering to complete
-			setTimeout(() => updateSettingTreeTabOrder(this.settingsTreeContainer), 0);
+			if (this.treeFocusedElement) {
+				this.treeFocusedElement.tabbable = true;
+			}
+
+			this.settingsTree.setSelection(element ? [element] : []);
 		}));
 	}
 

src/vs/workbench/contrib/preferences/browser/settingsTree.ts

@@ -481,17 +481,6 @@ function addChildrenToTabOrder(node: Element): void {
 	});
 }
 
-export function updateSettingTreeTabOrder(container: Element): void {
-	const allRows = [...container.querySelectorAll(AbstractSettingRenderer.ALL_ROWS_SELECTOR)];
-	const focusedRow = allRows.find(row => row.classList.contains('focused'));
-
-	allRows.forEach(removeChildrenFromTabOrder);
-
-	if (isDefined(focusedRow)) {
-		addChildrenToTabOrder(focusedRow);
-	}
-}
-
 export abstract class AbstractSettingRenderer extends Disposable implements ITreeRenderer<SettingsTreeElement, never, any> {
 	/** To override */
 	abstract get templateId(): string;
@@ -525,7 +514,6 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre
 	private readonly _onDidChangeIgnoredSettings = this._register(new Emitter<void>());
 	readonly onDidChangeIgnoredSettings: Event<void> = this._onDidChangeIgnoredSettings.event;
 
-	// Put common injections back here
 	constructor(
 		private readonly settingActions: IAction[],
 		private readonly disposableActionFactory: (setting: ISetting) => IAction[],
@@ -744,6 +732,19 @@ export abstract class AbstractSettingRenderer extends Disposable implements ITre
 		template.elementDisposables.add(this.onDidChangeIgnoredSettings(() => {
 			update();
 		}));
+
+		this.updateSettingTabbable(element, template);
+		template.elementDisposables.add(element.onDidChangeTabbable(() => {
+			this.updateSettingTabbable(element, template);
+		}));
+	}
+
+	private updateSettingTabbable(element: SettingsTreeSettingElement, template: ISettingItemTemplate | ISettingBoolItemTemplate): void {
+		if (element.tabbable) {
+			addChildrenToTabOrder(template.containerElement);
+		} else {
+			removeChildrenFromTabOrder(template.containerElement);
+		}
 	}
 
 	private renderSettingMarkdown(element: SettingsTreeSettingElement, text: string, disposeables: DisposableStore): HTMLElement {

src/vs/workbench/contrib/preferences/browser/settingsTreeModels.ts

@@ -16,6 +16,8 @@ import { IExtensionSetting, ISearchResult, ISetting, SettingValueType } from 'vs
 import { IWorkbenchEnvironmentService } from 'vs/workbench/services/environment/common/environmentService';
 import { FOLDER_SCOPES, WORKSPACE_SCOPES, REMOTE_MACHINE_SCOPES, LOCAL_MACHINE_SCOPES } from 'vs/workbench/services/configuration/common/configuration';
 import { IJSONSchema } from 'vs/base/common/jsonSchema';
+import { Disposable } from 'vs/base/common/lifecycle';
+import { Emitter } from 'vs/base/common/event';
 
 export const ONLINE_SERVICES_SETTING_TAG = 'usesOnlineServices';
 
@@ -26,13 +28,27 @@ export interface ISettingsEditorViewState {
 	filterToCategory?: SettingsTreeGroupElement;
 }
 
-export abstract class SettingsTreeElement {
+export abstract class SettingsTreeElement extends Disposable {
 	id: string;
 	parent?: SettingsTreeGroupElement;
 
+	private _tabbable = false;
+	protected readonly _onDidChangeTabbable = new Emitter<void>();
+	readonly onDidChangeTabbable = this._onDidChangeTabbable.event;
+
 	constructor(_id: string) {
+		super();
 		this.id = _id;
 	}
+
+	get tabbable(): boolean {
+		return this._tabbable;
+	}
+
+	set tabbable(value: boolean) {
+		this._tabbable = value;
+		this._onDidChangeTabbable.fire();
+	}
 }
 
 export type SettingsTreeGroupChild = (SettingsTreeGroupElement | SettingsTreeSettingElement | SettingsTreeNewExtensionsElement);
@@ -293,16 +309,31 @@ export class SettingsTreeModel {
 
 		const newRoot = this.createSettingsTreeGroupElement(newTocRoot);
 		if (newRoot.children[0] instanceof SettingsTreeGroupElement) {
-			(<SettingsTreeGroupElement>newRoot.children[0]).isFirstGroup = true; // TODO
+			(<SettingsTreeGroupElement>newRoot.children[0]).isFirstGroup = true;
 		}
 
 		if (this._root) {
+			this.disposeChildren(this._root.children);
 			this._root.children = newRoot.children;
 		} else {
 			this._root = newRoot;
 		}
 	}
 
+	private disposeChildren(children: SettingsTreeGroupChild[]) {
+		for (let child of children) {
+			this.recursiveDispose(child);
+		}
+	}
+
+	private recursiveDispose(element: SettingsTreeElement) {
+		if (element instanceof SettingsTreeGroupElement) {
+			this.disposeChildren(element.children);
+		}
+
+		element.dispose();
+	}
+
 	getElementsByName(name: string): SettingsTreeSettingElement[] | null {
 		return withUndefinedAsNull(this._treeElementsBySettingName.get(name));
 	}