Skip to content

Commit

Permalink
Editors can steal focus later when opening slowly (fix microsoft#128117…
Browse files Browse the repository at this point in the history 
…) (microsoft#170328)

* Editors can steal focus later when opening slowly (fix microsoft#128117)

* play it a bit safer

* fix tests
  • Loading branch information
@bpasero
bpasero authored Jan 1, 2023
1 parent 4c0cecf commit f01a44b
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 58 deletions.
89 changes: 42 additions & 47 deletions extensions/vscode-api-tests/src/singlefolder-tests/window.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,82 +71,77 @@ suite('vscode API - window', () => {
reg.dispose();
});

test('editor, onDidChangeTextEditorViewColumn (close editor)', () => {

let actualEvent: TextEditorViewColumnChangeEvent;
test('editor, onDidChangeTextEditorViewColumn (close editor)', async () => {

const registration1 = workspace.registerTextDocumentContentProvider('bikes', {
provideTextDocumentContent() {
return 'mountainbiking,roadcycling';
}
});

return Promise.all([
workspace.openTextDocument(Uri.parse('bikes://testing/one')).then(doc => window.showTextDocument(doc, ViewColumn.One)),
workspace.openTextDocument(Uri.parse('bikes://testing/two')).then(doc => window.showTextDocument(doc, ViewColumn.Two))
]).then(async editors => {
const doc1 = await workspace.openTextDocument(Uri.parse('bikes://testing/one'));
await window.showTextDocument(doc1, ViewColumn.One);

const [one, two] = editors;
const doc2 = await workspace.openTextDocument(Uri.parse('bikes://testing/two'));
const two = await window.showTextDocument(doc2, ViewColumn.Two);

await new Promise<void>(resolve => {
const registration2 = window.onDidChangeTextEditorViewColumn(event => {
actualEvent = event;
registration2.dispose();
resolve();
});
// close editor 1, wait a little for the event to bubble
one.hide();
});
assert.ok(actualEvent);
assert.ok(actualEvent.textEditor === two);
assert.ok(actualEvent.viewColumn === two.viewColumn);
assert.strictEqual(window.activeTextEditor?.viewColumn, ViewColumn.Two);

registration1.dispose();
const actualEvent = await new Promise<TextEditorViewColumnChangeEvent>(resolve => {
const registration2 = window.onDidChangeTextEditorViewColumn(event => {
registration2.dispose();
resolve(event);
});
// close editor 1, wait a little for the event to bubble
commands.executeCommand('workbench.action.closeEditorsInOtherGroups');
});
});
assert.ok(actualEvent);
assert.ok(actualEvent.textEditor === two);
assert.ok(actualEvent.viewColumn === two.viewColumn);

test('editor, onDidChangeTextEditorViewColumn (move editor group)', () => {
registration1.dispose();
});

const actualEvents: TextEditorViewColumnChangeEvent[] = [];
test('editor, onDidChangeTextEditorViewColumn (move editor group)', async () => {

const registration1 = workspace.registerTextDocumentContentProvider('bikes', {
provideTextDocumentContent() {
return 'mountainbiking,roadcycling';
}
});

return Promise.all([
workspace.openTextDocument(Uri.parse('bikes://testing/one')).then(doc => window.showTextDocument(doc, ViewColumn.One)),
workspace.openTextDocument(Uri.parse('bikes://testing/two')).then(doc => window.showTextDocument(doc, ViewColumn.Two))
]).then(editors => {

const [, two] = editors;
two.show();
const doc1 = await workspace.openTextDocument(Uri.parse('bikes://testing/one'));
await window.showTextDocument(doc1, ViewColumn.One);

return new Promise<void>(resolve => {
const doc2 = await workspace.openTextDocument(Uri.parse('bikes://testing/two'));
await window.showTextDocument(doc2, ViewColumn.Two);

const registration2 = window.onDidChangeTextEditorViewColumn(event => {
actualEvents.push(event);
assert.strictEqual(window.activeTextEditor?.viewColumn, ViewColumn.Two);

if (actualEvents.length === 2) {
registration2.dispose();
resolve();
}
});
const actualEvents = await new Promise<TextEditorViewColumnChangeEvent[]>(resolve => {

// move active editor group left
return commands.executeCommand('workbench.action.moveActiveEditorGroupLeft');
const actualEvents: TextEditorViewColumnChangeEvent[] = [];

}).then(() => {
assert.strictEqual(actualEvents.length, 2);
const registration2 = window.onDidChangeTextEditorViewColumn(event => {
actualEvents.push(event);

for (const event of actualEvents) {
assert.strictEqual(event.viewColumn, event.textEditor.viewColumn);
if (actualEvents.length === 2) {
registration2.dispose();
resolve(actualEvents);
}

registration1.dispose();
});

// move active editor group left
return commands.executeCommand('workbench.action.moveActiveEditorGroupLeft');

});
assert.strictEqual(actualEvents.length, 2);

for (const event of actualEvents) {
assert.strictEqual(event.viewColumn, event.textEditor.viewColumn);
}

registration1.dispose();
});

test('active editor not always correct... #49125', async function () {
Expand Down
3 changes: 2 additions & 1 deletion src/vs/workbench/browser/parts/editor/editorGroupView.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
this.element.appendChild(this.editorContainer);

// Editor pane
this.editorPane = this._register(this.scopedInstantiationService.createInstance(EditorPanes, this.editorContainer, this));
this.editorPane = this._register(this.scopedInstantiationService.createInstance(EditorPanes, this.element, this.editorContainer, this));
this._onDidChange.input = this.editorPane.onDidChangeSizeConstraints;

// Track Focus
Expand Down Expand Up @@ -511,6 +511,7 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
// not changed meanwhile. This prevents focus from being
// stolen accidentally on startup when the user already
// clicked somewhere.

if (this.accessor.activeGroup === this && activeElement === document.activeElement) {
this.focus();
}
Expand Down
58 changes: 48 additions & 10 deletions src/vs/workbench/browser/parts/editor/editorPanes.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import Severity from 'vs/base/common/severity';
import { Disposable, DisposableStore } from 'vs/base/common/lifecycle';
import { EditorExtensions, EditorInputCapabilities, IEditorOpenContext, IVisibleEditorPane, isEditorOpenError } from 'vs/workbench/common/editor';
import { EditorInput } from 'vs/workbench/common/editor/editorInput';
import { Dimension, show, hide, IDomNodePagePosition } from 'vs/base/browser/dom';
import { Dimension, show, hide, IDomNodePagePosition, isAncestor } from 'vs/base/browser/dom';
import { Registry } from 'vs/platform/registry/common/platform';
import { IEditorPaneRegistry, IEditorPaneDescriptor } from 'vs/workbench/browser/editor';
import { IWorkbenchLayoutService } from 'vs/workbench/services/layout/browser/layoutService';
Expand Down Expand Up @@ -91,7 +91,8 @@ export class EditorPanes extends Disposable {
private readonly editorPanesRegistry = Registry.as<IEditorPaneRegistry>(EditorExtensions.EditorPane);

constructor(
private parent: HTMLElement,
private editorGroupParent: HTMLElement,
private editorPanesParent: HTMLElement,
private groupView: IEditorGroupView,
@IWorkbenchLayoutService private readonly layoutService: IWorkbenchLayoutService,
@IInstantiationService private readonly instantiationService: IInstantiationService,
Expand Down Expand Up @@ -237,20 +238,57 @@ export class EditorPanes extends Disposable {
// Editor pane
const pane = this.doShowEditorPane(descriptor);

// Remember current active element for deciding to restore focus later
const activeElement = document.activeElement;

// Apply input to pane
const { changed, cancelled } = await this.doSetInput(pane, editor, options, context);

// Focus unless cancelled
if (!cancelled) {
const focus = !options || !options.preserveFocus;
if (focus) {
pane.focus();
}
// Focus only if not cancelled and not prevented
const focus = !options || !options.preserveFocus;
if (!cancelled && focus && this.shouldRestoreFocus(activeElement)) {
pane.focus();
}

return { pane, changed, cancelled };
}

private shouldRestoreFocus(expectedActiveElement: Element | null): boolean {
if (!this.layoutService.isRestored()) {
return true; // restore focus if we are not restored yet on startup
}

if (!expectedActiveElement) {
return true; // restore focus if nothing was focused
}

const activeElement = document.activeElement;

if (!activeElement || activeElement === document.body) {
return true; // restore focus if nothing is focused currently
}

const same = expectedActiveElement === activeElement;
if (same) {
return true; // restore focus if same element is still active
}

if (activeElement.tagName !== 'INPUT' && activeElement.tagName !== 'TEXTAREA') {

// This is to avoid regressions from not restoring focus as we used to:
// Only allow a different input element (or textarea) to remain focused
// but not other elements that do not accept text input.

return true;
}

if (isAncestor(activeElement, this.editorGroupParent)) {
return true; // restore focus if active element is still inside our editor group
}

return false; // do not restore focus
}

private getEditorPaneDescriptor(editor: EditorInput): IEditorPaneDescriptor {
if (editor.hasCapability(EditorInputCapabilities.RequiresTrust) && !this.workspaceTrustService.isWorkspaceTrusted()) {
// Workspace trust: if an editor signals it needs workspace trust
Expand Down Expand Up @@ -281,7 +319,7 @@ export class EditorPanes extends Disposable {

// Show editor
const container = assertIsDefined(editorPane.getContainer());
this.parent.appendChild(container);
this.editorPanesParent.appendChild(container);
show(container);

// Indicate to editor that it is now visible
Expand Down Expand Up @@ -403,7 +441,7 @@ export class EditorPanes extends Disposable {
// Remove editor pane from parent
const editorPaneContainer = this._activeEditorPane.getContainer();
if (editorPaneContainer) {
this.parent.removeChild(editorPaneContainer);
this.editorPanesParent.removeChild(editorPaneContainer);
hide(editorPaneContainer);
}

Expand Down

0 comments on commit f01a44b

Please sign in to comment.