Skip to content

Commit

Permalink
Workaround/input box focus (jupyterlab#15479)
Browse files Browse the repository at this point in the history
* Re-focus input after modifying model, keep input for 0.5s

* Add a test case against the focus loss issue

Tested that it fails on main reproducing the issue:

```
    - Expected  - 0
    + Received  + 8

      from time import sleep
      input()
      print('before sleep')
      sleep(0.1)
      print('after sleep')
    +
    +
    + x
    +
    +
    +
    +
    +
```

* Give up on using animations and :has() solution due to FF bug

https://bugzilla.mozilla.org/show_bug.cgi?id=1867893

`:has()` is not only not yet officially supported but is hard to
implement well so no wonder there are difficult invalidation bugs

* More robust stdin test
  • Loading branch information
krassowski committed Dec 13, 2023
1 parent d62f842 commit 68be103
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 8 deletions.
55 changes: 50 additions & 5 deletions galata/test/jupyterlab/outputarea-stdin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ test.use({
locale: 'en-US'
});

const loopedInput = `\
from time import sleep
input()
print('before sleep')
sleep(0.1)
print('after sleep')`;

const ACTIVE_INPUT =
'.jp-OutputArea-stdin-item:not(.jp-OutputArea-stdin-hiding) .jp-Stdin-input';

test.describe('Stdin for ipdb', () => {
test.beforeEach(async ({ page }) => {
await page.notebook.createNew();
Expand All @@ -24,20 +34,20 @@ test.describe('Stdin for ipdb', () => {
await page.keyboard.press('Control+Enter');

// enter a bunch of nonsense commands into the stdin attached to ipdb
await page.waitForSelector('.jp-Stdin-input');
await page.waitForSelector(ACTIVE_INPUT);
await page.keyboard.insertText('foofoo');
await page.keyboard.press('Enter');

await page.waitForSelector('.jp-Stdin-input');
await page.waitForSelector(ACTIVE_INPUT);
await page.keyboard.insertText('barbar');
await page.keyboard.press('Enter');

await page.waitForSelector('.jp-Stdin-input');
await page.waitForSelector(ACTIVE_INPUT);
await page.keyboard.insertText('bazbaz');
await page.keyboard.press('Enter');

// search for the first nonsense command
await page.waitForSelector('.jp-Stdin-input');
await page.waitForSelector(ACTIVE_INPUT);
await page.keyboard.insertText('foo');
await page.keyboard.press('Control+ArrowUp');

Expand All @@ -54,7 +64,7 @@ test.describe('Stdin for ipdb', () => {

// Check that the input remains focused and cursor is at the end.
await page.keyboard.insertText('x');
await expect(page.locator('.jp-Stdin-input')).toHaveValue('foofoox');
await expect(page.locator(ACTIVE_INPUT)).toHaveValue('foofoox');
});

test('Typing in stdin box', async ({ page }) => {
Expand Down Expand Up @@ -82,4 +92,39 @@ test.describe('Stdin for ipdb', () => {
alphabet + alphabet.toUpperCase() + digits
);
});

test('Subsequent execution in short succession', async ({ page }) => {
await page.notebook.setCell(0, 'code', loopedInput);
// Run the selected (only) cell without proceeding and without waiting
// for it to complete (as it should stay waiting for input).
await page.keyboard.press('Control+Enter');

// Wait for first input
await page.waitForSelector('.jp-Stdin-input');

// Note: this test does not wait for subsequent inputs on purpose

await page.getByText('before sleep').waitFor();

// Press enter five times (should do nothing)
for (let j = 0; j < 5; j++) {
await page.keyboard.press('Enter');
}
// Press a key which should go to the input
await page.keyboard.press('x');

await page.getByText('after sleep').waitFor();

// Press enter five times (should submit and then do nothing)
for (let j = 0; j < 5; j++) {
await page.keyboard.press('Enter');
}

const cellInput = await page.notebook.getCellInput(0);
const editor = await cellInput.$('.cm-content');
const contentAfter = await editor.evaluate((e: any) =>
e.cmView.view.state.doc.toString()
);
expect(contentAfter).toBe(loopedInput);
});
});
39 changes: 36 additions & 3 deletions packages/outputarea/src/widget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ const OUTPUT_AREA_OUTPUT_CLASS = 'jp-OutputArea-output';
*/
const OUTPUT_AREA_PROMPT_CLASS = 'jp-OutputArea-prompt';

const OUTPUT_AREA_STDIN_HIDING_CLASS = 'jp-OutputArea-stdin-hiding';

/**
* The class name added to OutputPrompt.
*/
Expand Down Expand Up @@ -457,10 +459,11 @@ export class OutputArea extends Widget {
if (this.model.length >= this.maxNumberOutputs) {
this.maxNumberOutputs = this.model.length;
}
this.layout.addWidget(panel);

this._inputRequested.emit(input);

// Get the input node to ensure focus after updating the model upon user reply.
const inputNode = input.node.getElementsByTagName('input')[0];

/**
* Wait for the stdin to complete, add it to the model (so it persists)
* and remove the stdin widget.
Expand All @@ -470,14 +473,37 @@ export class OutputArea extends Widget {
if (this.model.length >= this.maxNumberOutputs) {
this.maxNumberOutputs = this.model.length + 1;
}
panel.addClass(OUTPUT_AREA_STDIN_HIDING_CLASS);
// Use stdin as the stream so it does not get combined with stdout.
// Note: because it modifies DOM it may (will) shift focus away from the input node.
this.model.add({
output_type: 'stream',
name: 'stdin',
text: value + '\n'
});
panel.dispose();
// Refocus the input node after it lost focus due to update of the model.
inputNode.focus();

// Keep the input in view for a little while; this (along refocusing)
// ensures that we can avoid the cell editor stealing the focus, and
// leading to user inadvertently modifying editor content when executing
// consecutive commands in short succession.
window.setTimeout(() => {
// Tack currently focused element to ensure that it remains on it
// after disposal of the panel with the old input
// (which modifies DOM and can lead to focus jump).
const focusedElement = document.activeElement;
// Dispose the old panel with no longer needed input box.
panel.dispose();
// Refocus the element that was focused before.
if (focusedElement && focusedElement instanceof HTMLElement) {
focusedElement.focus();
}
}, 500);
});

// Note: the `input.value` promise must be listened to before we attach the panel
this.layout.addWidget(panel);
}

/**
Expand Down Expand Up @@ -1093,6 +1119,11 @@ export class Stdin extends Widget implements IStdin {
* not be called directly by user code.
*/
handleEvent(event: KeyboardEvent): void {
if (this._resolved) {
// Do not handle any more key events if the promise was resolved.
event.preventDefault();
return;
}
const input = this._input;

if (event.type === 'keydown') {
Expand All @@ -1112,6 +1143,7 @@ export class Stdin extends Widget implements IStdin {
this._value += input.value;
Stdin._historyPush(this._historyKey, input.value);
}
this._resolved = true;
this._promise.resolve(void 0);
} else if (event.key === 'Escape') {
// currently this gets clobbered by the documentsearch:end command at the notebook level
Expand Down Expand Up @@ -1227,6 +1259,7 @@ export class Stdin extends Widget implements IStdin {
private _trans: TranslationBundle;
private _value: string;
private _valueCache: string;
private _resolved: boolean = false;
}

export namespace Stdin {
Expand Down
6 changes: 6 additions & 0 deletions packages/outputarea/style/base.css
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,12 @@ body.lm-mod-override-cursor .jp-OutputArea-output.jp-mod-isolated::before {
opacity: 1;
}

.jp-OutputArea-stdin-hiding {
/* soft-hide the output, preserving focus */
opacity: 0;
height: 0;
}

/*-----------------------------------------------------------------------------
| Output Area View
|----------------------------------------------------------------------------*/
Expand Down

0 comments on commit 68be103

Please sign in to comment.