Skip to content

Commit 1a8331f

Browse files
roblourensroblourens
committed
Fix some notebook cell focus/tabbing issues
- Get rid of output focus sink when cell has no output, to avoid a dead spot when tabbing through cell - Fix the toolbar losing focus shortly after tabbing onto it, by avoid context key churn - Fix domFocus when focus is in a different element from the one that is selected (depends on an earlier PR in listCommands.ts) Fix microsoft#99782
1 parent f9c6fa3 commit 1a8331f

5 files changed

Lines changed: 42 additions & 15 deletions

File tree

‎src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,7 @@ export interface CodeCellRenderTemplate extends BaseCellRenderTemplate {
436436
runButtonContainer: HTMLElement;
437437
executionOrderLabel: HTMLElement;
438438
outputContainer: HTMLElement;
439+
focusSinkElement: HTMLElement;
439440
editor: ICodeEditor;
440441
progressBar: ProgressBar;
441442
timer: TimerRenderer;

‎src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -407,10 +407,13 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditor
407407
}));
408408

409409
this.localStore.add(this.list!.onDidChangeFocus(() => {
410-
this._cellContextKeyManager?.dispose();
411410
const focused = this.list!.getFocusedElements()[0];
412411
if (focused) {
413-
this._cellContextKeyManager = this.localStore.add(new CellContextKeyManager(this.contextKeyService, textModel, focused as any));
412+
if (!this._cellContextKeyManager) {
413+
this._cellContextKeyManager = this.localStore.add(new CellContextKeyManager(this.contextKeyService, textModel, focused as any));
414+
}
415+
416+
this._cellContextKeyManager.updateForElement(focused as any);
414417
}
415418
}));
416419

‎src/vs/workbench/contrib/notebook/browser/view/notebookCellList.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -525,7 +525,10 @@ export class NotebookCellList extends WorkbenchList<CellViewModel> implements ID
525525

526526
// override
527527
domFocus() {
528-
if (document.activeElement && this.view.domNode.contains(document.activeElement)) {
528+
const focused = this.getFocusedElements()[0];
529+
const focusedDomElement = this.domElementOfElement(focused);
530+
531+
if (document.activeElement && focusedDomElement && focusedDomElement.contains(document.activeElement)) {
529532
// for example, when focus goes into monaco editor, if we refocus the list view, the editor will lose focus.
530533
return;
531534
}

‎src/vs/workbench/contrib/notebook/browser/view/renderers/cellContextKeys.ts‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { BaseCellViewModel } from 'vs/workbench/contrib/notebook/browser/viewMod
99
import { NOTEBOOK_CELL_TYPE, NOTEBOOK_VIEW_TYPE, NOTEBOOK_CELL_EDITABLE, NOTEBOOK_CELL_RUNNABLE, NOTEBOOK_CELL_MARKDOWN_EDIT_MODE, NOTEBOOK_CELL_RUN_STATE, NOTEBOOK_CELL_HAS_OUTPUTS, CellViewModelStateChangeEvent, CellEditState } from 'vs/workbench/contrib/notebook/browser/notebookBrowser';
1010
import { CodeCellViewModel } from 'vs/workbench/contrib/notebook/browser/viewModel/codeCellViewModel';
1111
import { MarkdownCellViewModel } from 'vs/workbench/contrib/notebook/browser/viewModel/markdownCellViewModel';
12-
import { Disposable } from 'vs/base/common/lifecycle';
12+
import { Disposable, DisposableStore } from 'vs/base/common/lifecycle';
1313

1414
export class CellContextKeyManager extends Disposable {
1515

@@ -22,10 +22,12 @@ export class CellContextKeyManager extends Disposable {
2222

2323
private markdownEditMode: IContextKey<boolean>;
2424

25+
private elementDisposables = new DisposableStore();
26+
2527
constructor(
2628
private readonly contextKeyService: IContextKeyService,
2729
private readonly notebookTextModel: INotebookTextModel,
28-
private readonly element: BaseCellViewModel
30+
private element: BaseCellViewModel
2931
) {
3032
super();
3133

@@ -37,16 +39,18 @@ export class CellContextKeyManager extends Disposable {
3739
this.cellRunState = NOTEBOOK_CELL_RUN_STATE.bindTo(this.contextKeyService);
3840
this.cellHasOutputs = NOTEBOOK_CELL_HAS_OUTPUTS.bindTo(this.contextKeyService);
3941

40-
this._register(element.onDidChangeState(e => this.onDidChangeState(e)));
42+
this.updateForElement(element);
43+
}
44+
45+
public updateForElement(element: BaseCellViewModel) {
46+
this.elementDisposables.clear();
47+
this.elementDisposables.add(element.onDidChangeState(e => this.onDidChangeState(e)));
4148

4249
if (element instanceof CodeCellViewModel) {
43-
this._register(element.onDidChangeOutputs(() => this.updateForOutputs()));
50+
this.elementDisposables.add(element.onDidChangeOutputs(() => this.updateForOutputs()));
4451
}
4552

46-
this.initialize();
47-
}
48-
49-
private initialize() {
53+
this.element = element;
5054
if (this.element instanceof MarkdownCellViewModel) {
5155
this.cellType.set('markdown');
5256
} else if (this.element instanceof CodeCellViewModel) {

‎src/vs/workbench/contrib/notebook/browser/view/renderers/cellRenderer.ts‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,11 @@ abstract class AbstractCellRenderer {
266266
const updateActions = () => {
267267
const actions = this.getCellToolbarActions(menu);
268268

269+
const hadFocus = DOM.isAncestor(document.activeElement, templateData.toolbar.getContainer());
269270
templateData.toolbar.setActions(actions.primary, actions.secondary)();
271+
if (hadFocus) {
272+
this.notebookEditor.focus();
273+
}
270274

271275
if (templateData.focusIndicator) {
272276
if (actions.primary.length || actions.secondary.length) {
@@ -897,8 +901,8 @@ export class CodeCellRenderer extends AbstractCellRenderer implements IListRende
897901
const timer = new TimerRenderer(statusBar.durationContainer);
898902

899903
const outputContainer = DOM.append(container, $('.output'));
900-
const focusSink = DOM.append(container, $('.cell-editor-focus-sink'));
901-
focusSink.setAttribute('tabindex', '0');
904+
const focusSinkElement = DOM.append(container, $('.cell-editor-focus-sink'));
905+
focusSinkElement.setAttribute('tabindex', '0');
902906
const bottomCellContainer = DOM.append(container, $('.cell-bottom-toolbar-container'));
903907
DOM.append(bottomCellContainer, $('.separator'));
904908
const betweenCellToolbar = this.createBetweenCellToolbar(bottomCellContainer, disposables, contextKeyService);
@@ -916,6 +920,7 @@ export class CodeCellRenderer extends AbstractCellRenderer implements IListRende
916920
focusIndicator,
917921
toolbar,
918922
betweenCellToolbar,
923+
focusSinkElement,
919924
runToolbar,
920925
runButtonContainer,
921926
executionOrderLabel,
@@ -930,8 +935,8 @@ export class CodeCellRenderer extends AbstractCellRenderer implements IListRende
930935

931936
this.dndController.registerDragHandle(templateData, () => new CodeCellDragImageRenderer().getDragImage(templateData, templateData.editor, 'code'));
932937

933-
disposables.add(DOM.addDisposableListener(focusSink, DOM.EventType.FOCUS, () => {
934-
if (templateData.currentRenderedCell) {
938+
disposables.add(DOM.addDisposableListener(focusSinkElement, DOM.EventType.FOCUS, () => {
939+
if (templateData.currentRenderedCell && (templateData.currentRenderedCell as CodeCellViewModel).outputs.length) {
935940
this.notebookEditor.focusNotebookCell(templateData.currentRenderedCell, 'output');
936941
}
937942
}));
@@ -961,6 +966,14 @@ export class CodeCellRenderer extends AbstractCellRenderer implements IListRende
961966
}
962967
}
963968

969+
private updateForOutputs(element: CodeCellViewModel, templateData: CodeCellRenderTemplate): void {
970+
if (element.outputs.length) {
971+
DOM.show(templateData.focusSinkElement);
972+
} else {
973+
DOM.hide(templateData.focusSinkElement);
974+
}
975+
}
976+
964977
private updateForMetadata(element: CodeCellViewModel, templateData: CodeCellRenderTemplate): void {
965978
const metadata = element.getEvaluatedMetadata(this.notebookEditor.viewModel!.notebookDocument.metadata);
966979
DOM.toggleClass(templateData.cellContainer, 'runnable', !!metadata.runnable);
@@ -1045,6 +1058,9 @@ export class CodeCellRenderer extends AbstractCellRenderer implements IListRende
10451058
}
10461059
}));
10471060

1061+
this.updateForOutputs(element, templateData);
1062+
elementDisposables.add(element.onDidChangeOutputs(_e => this.updateForOutputs(element, templateData)));
1063+
10481064
this.setupCellToolbarActions(templateData.contextKeyService, templateData, elementDisposables);
10491065

10501066
const toolbarContext = <INotebookCellActionContext>{

0 commit comments

Comments
 (0)