Fix active cell update on tabbing (microsoft#18614)

* listen on focus_in of toolbar * update styles on focus_in * listen for active cell change on notebook componen * add tabbing order to textcells * remove duplicate listener * clean up * undo * remove visible check from cellToolbar * remove duplicate detectChanges on updateActiveCell * only update active cell if it's already not * add aria-label for accessibility * localize the aria label * refactor * add cellLabel property to CellModel * remove updateActiveCell from code component * regression from merge fix * set edit mode as true when focusing on cell * moce check to model * merge changes correctly * update edit mode if code cell * fixes Co-authored-by: barbaravaldez <bavaldez@microsoft.com> Co-authored-by: chgagnon <chgagnon@microsoft.com>
GazimbeIsaac · Apr 8, 2022 · 4191ef8 · 4191ef8
1 parent 41b639c
commit 4191ef8
Show file tree

Hide file tree

Showing 9 changed files with 37 additions and 16 deletions.
diff --git a/src/sql/workbench/contrib/notebook/browser/cellViews/code.component.html b/src/sql/workbench/contrib/notebook/browser/cellViews/code.component.html
@@ -14,7 +14,7 @@
 		</div>
 		<div style="display: flex; flex-flow: row; justify-content: flex-end;">
 			<collapse-component *ngIf="cellModel.cellType === 'code' && cellModel.source && cellModel.source.length > 1" [cellModel]="cellModel" [activeCellId]="activeCellId"></collapse-component>
-			<button #cellLanguage title="{{cellLanguageTitle}}" class="cellLanguage" *ngIf="cellModel.cellType === 'code' && cellModel.language" (click)="onCellLanguageClick()">
+			<button #cellLanguage title="{{cellLanguageTitle}}" class="cellLanguage" *ngIf="cellModel.cellType === 'code' && cellModel.language" (click)="onCellLanguageClick()" (focus)="onCellLanguageFocus()">
 				{{cellModel.displayLanguage}}
 			</button>
 		</div>

diff --git a/src/sql/workbench/contrib/notebook/browser/cellViews/code.component.ts b/src/sql/workbench/contrib/notebook/browser/cellViews/code.component.ts
@@ -452,8 +452,9 @@ export class CodeComponent extends CellView implements OnInit, OnChanges {
 
 	private onCellModeChanged(isEditMode: boolean): void {
 		if (this.cellModel.id === this._activeCellId || this._activeCellId === '') {
-			this._editor.getControl().focus();
-			if (!isEditMode) {
+			if (isEditMode) {
+				this._editor.getControl().focus();
+			} else {
 				(document.activeElement as HTMLElement).blur();
 			}
 		}
@@ -470,6 +471,10 @@ export class CodeComponent extends CellView implements OnInit, OnChanges {
 			.catch(err => onUnexpectedError(err));
 	}
 
+	public onCellLanguageFocus(): void {
+		this._model.updateActiveCell(this._cellModel);
+	}
+
 	private pickCellLanguage(languages: string[]): Promise<ILanguagePickInput | undefined> {
 		if (languages.length === 0) {
 			languages = [this._cellModel.language];

diff --git a/src/sql/workbench/contrib/notebook/browser/cellViews/collapse.component.html b/src/sql/workbench/contrib/notebook/browser/cellViews/collapse.component.html
@@ -5,5 +5,5 @@
  *--------------------------------------------------------------------------------------------*/
 -->
 <div style="width: 100%; height: fit-content; display: flex; flex-flow: column">
-	<button #collapseCellButton (click)="toggleCollapsed($event)" class="hide-component-button codicon"></button>
+	<button #collapseCellButton (click)="toggleCollapsed($event)" class="hide-component-button codicon" (focus)="onFocus()"></button>
 </div>
diff --git a/src/sql/workbench/contrib/notebook/browser/cellViews/collapse.component.ts b/src/sql/workbench/contrib/notebook/browser/cellViews/collapse.component.ts
@@ -68,6 +68,10 @@ export class CollapseComponent extends CellView implements OnInit, OnChanges {
 		this.cellModel.isCollapsed = !this.cellModel.isCollapsed;
 	}
 
+	public onFocus(): void {
+		this.cellModel.notebookModel.updateActiveCell(this.cellModel);
+	}
+
 	public cellGuid(): string {
 		return this.cellModel.cellGuid;
 	}

diff --git a/src/sql/workbench/contrib/notebook/browser/notebook.component.html b/src/sql/workbench/contrib/notebook/browser/notebook.component.html
@@ -9,7 +9,7 @@
 	</div>
 	<div #container class="scrollable" style="flex: 1 1 auto; position: relative; outline: none" (click)="clickOffCell($event)" (scroll)="scrollHandler($event)">
 		<div *ngFor="let cell of cells">
-			<div id="{{ cell.id }}" class="notebook-cell" (click)="clickOnCell(cell, $event)" [class.active]="cell.active">
+			<div id="{{ cell.id }}" class="notebook-cell" (click)="clickOnCell(cell, $event)" [class.active]="cell.active" (focus)="updateActiveCell(cell)" tabindex="0" attr.aria-label="{{ cell.cellLabel }}">
 				<cell-toolbar-component *ngIf="cell.active" [cellModel]="cell" [model]="model"></cell-toolbar-component>
 				<code-cell-component *ngIf="cell.cellType === 'code'" [cellModel]="cell" [model]="model" [activeCellId]="activeCellId">
 				</code-cell-component>

diff --git a/src/sql/workbench/contrib/notebook/browser/notebook.component.ts b/src/sql/workbench/contrib/notebook/browser/notebook.component.ts
@@ -297,7 +297,6 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
 	public selectCell(cell: ICellModel) {
 		if (!this.model.activeCell || this.model.activeCell.id !== cell.id) {
 			this.model.updateActiveCell(cell);
-			this.detectChanges();
 		}
 	}
 
@@ -340,7 +339,10 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
 
 	public unselectActiveCell() {
 		this.model.updateActiveCell(undefined);
-		this.detectChanges();
+	}
+
+	public updateActiveCell(cell: ICellModel) {
+		this._model.updateActiveCell(cell);
 	}
 
 	// Handles double click to edit icon change
@@ -446,6 +448,7 @@ export class NotebookComponent extends AngularDisposable implements OnInit, OnDe
 		this._register(this._model.contentChanged((change) => this.handleContentChanged(change)));
 		this._register(this._model.onProviderIdChange((provider) => this.handleProviderIdChanged(provider)));
 		this._register(this._model.kernelChanged((kernelArgs) => this.handleKernelChanged(kernelArgs)));
+		this._register(this._model.onActiveCellChanged(() => this.detectChanges()));
 		this._register(this._model.onCellTypeChanged(() => this.detectChanges()));
 		this._register(this._model.layoutChanged(() => this.detectChanges()));
 		this._register(this.model.onScroll.event(() => this._onScroll.fire()));

diff --git a/src/sql/workbench/services/notebook/browser/models/cell.ts b/src/sql/workbench/services/notebook/browser/models/cell.ts
@@ -46,6 +46,7 @@ export interface QueryResultId {
 
 export class CellModel extends Disposable implements ICellModel {
 	public id: string;
+	public cellLabel: string;
 
 	private _cellType: nb.CellType;
 	private _source: string | string[];
@@ -120,6 +121,11 @@ export class CellModel extends Disposable implements ICellModel {
 		}
 		// if the fromJson() method was already called and _cellGuid was previously set, don't generate another UUID unnecessarily
 		this._cellGuid = this._cellGuid || generateUuid();
+		if (this._cellType === 'code') {
+			this.cellLabel = localize('codeCellLabel', "Code Cell {0}", this.id);
+		} else {
+			this.cellLabel = localize('mdCellLabel', "Markdown Cell {0}", this.id);
+		}
 		this.createUri();
 		this.populatePropertiesFromSettings();
 	}

diff --git a/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts b/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts
@@ -499,6 +499,7 @@ export interface ITableUpdatedEvent {
 export interface ICellModel {
 	cellUri: URI;
 	id: string;
+	cellLabel: string;
 	readonly language: string;
 	readonly displayLanguage: string;
 	readonly cellGuid: string;

diff --git a/src/sql/workbench/services/notebook/browser/models/notebookModel.ts b/src/sql/workbench/services/notebook/browser/models/notebookModel.ts
@@ -802,16 +802,18 @@ export class NotebookModel extends Disposable implements INotebookModel {
 	}
 
 	public updateActiveCell(cell?: ICellModel, isEditMode: boolean = false): void {
-		if (this._activeCell) {
-			this._activeCell.active = false;
-			this._activeCell.isEditMode = false;
-		}
-		this._activeCell = cell;
-		if (this._activeCell) {
-			this._activeCell.active = true;
-			this._activeCell.isEditMode = isEditMode;
+		if (this._activeCell !== cell) {
+			if (this._activeCell) {
+				this._activeCell.active = false;
+				this._activeCell.isEditMode = false;
+			}
+			this._activeCell = cell;
+			if (this._activeCell) {
+				this._activeCell.active = true;
+				this._activeCell.isEditMode = isEditMode;
+			}
+			this._onActiveCellChanged.fire(cell);
 		}
-		this._onActiveCellChanged.fire(cell);
 	}
 
 	public convertCellType(cell: ICellModel, addToUndoStack: boolean = true): void {