microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.89k stars 29.17k forks source link

F6 navigation does not work inside notebook editors #107580

Closed isidorn closed 4 years ago

isidorn commented 4 years ago

F6 can navigate you across the workbench parts. Pressing F6 when the focus is in a notebook cell has no effect

fyi @rebornix reported by @webczat

isidorn commented 4 years ago

The issue here is that the Notebook Cell List is not a child of the Editor Part workbench.parts.editor.part.editor @rebornix is there a special reason why the Notebook cell list does not respect the workbench HTML hierarchy. Because it is not a child of the parts.editor element this action here does not know that the editor part is focused and goes to the default - to focus the editor which is already focused.

isidorn commented 4 years ago

@rebornix friendly ping Also @roblourens or @jrieken might have more details

rebornix commented 4 years ago

@isidorn the notebook list view contains a webview, which will be destroyed when reparenting it so we have to render notebook list out of the grid view. That's why the notebook list view is not part of parts.editor. Do you have any suggestion what we should do here to support F6?

isidorn commented 4 years ago

@rebornix is there some service which can tell me if the focus is in the notebook?

rebornix commented 4 years ago

we can probably check IEditorService to check if the editor is notebook editor and then check if the active element is in the notebook editor's list view

https://github.com/microsoft/vscode/blob/master/src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts#L706-L710

for example

export function isActiveElementInNotebookEditor(editorService: IEditorService): boolean {
    const activeEditorPane = editorService.activeEditorPane as unknown as { isNotebookEditor?: boolean } | undefined;
    if (activeEditorPane?.isNotebookEditor) {
        const control = editorService.activeEditorPane?.getControl() as INotebookEditor;
        const activeElement = document.activeElement;
        return isAncestor(activeElement, control.getOverflowContainerDomNode());
    }

    return false;
}
isidorn commented 4 years ago

Went with @rebornix solution fyi @bpasero if you have a good idea on a common way to get an HTML element of any editor (including custom editors). Could not find it, since getDomNode() is only exposed by some eidtor types.

bpasero commented 4 years ago

@isidorn what HTML element are you wanting to access specifically, the container of an editor?

isidorn commented 4 years ago

@bpasero yeah, container should be fine.

bpasero commented 4 years ago

@isidorn logically it should be a property on IEditorPane imho, but the challenge is that this interface lives in common and cannot reference HTMLElement.

isidorn commented 4 years ago

@bpasero yeah I was expecting it there. But I see the issue, thanks.