New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Focus notebook output #97185
Focus notebook output #97185
Conversation
src/vs/workbench/contrib/notebook/browser/contrib/coreActions.ts
Outdated
Show resolved
Hide resolved
@@ -405,6 +466,14 @@ ${loaderJs} | |||
initialize(content: string) { | |||
this.webview = this._createInset(this.webviewService, content); | |||
this.webview.mountTo(this.element); | |||
this.webview.onDidFocus(() => { | |||
if (this.activeCellId) { | |||
this.webview.sendMessage({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this will break users' regular operations like clicking a button, selecting some text, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this.activeCellId = undefined after calling the focus listener. This should ensure that the message is sent only once per Ctrl + UpArrow in and will never send on focus via mouse. I'll leave this unresolved though in case I missed something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine for now, I will probably mess around with it. @rebornix We saw a weird issue if we send this message immediately after calling webview.focus() without waiting
let lowerWrapperElement = document.createElement('div'); | ||
lowerWrapperElement.tabIndex = 0; | ||
container.appendChild(lowerWrapperElement); | ||
lowerWrapperElement.addEventListener('focus', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -310,6 +336,32 @@ ${loaderJs} | |||
data: { } | |||
}); | |||
}); | |||
|
|||
const handleKeyDown = (event) => { | |||
if (event.defaultPrevented || !(event.key === 'ArrowUp' && event.ctrlKey)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a command with keybinding, I can make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@@ -207,11 +208,11 @@ registerAction2(class extends Action2 { | |||
// Try to select below, fall back on inserting | |||
const nextCell = editor.viewModel?.viewCells[idx + 1]; | |||
if (nextCell) { | |||
editor.focusNotebookCell(nextCell, activeCell.editState === CellEditState.Editing); | |||
editor.focusNotebookCell(nextCell, activeCell.editState === CellEditState.Editing ? 'editor' : 'container'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can add an enum to notebookCommon
Thanks @ctmayn! |
These changes allow focus to be set within the output of a notebook cell using only the keyboard.
While this PR makes it partially accessible there are still a couple issues (and possibly a few more undiscovered ones) that need to be handled before considering the issue fixed:
This PR fixes #93896