Skip to content

Commit

Permalink
Fix typing in editable elements inside of open shadow DOM (#15774)
Browse files Browse the repository at this point in the history
* Add a test case for typing in an input in shadow DOM

* Introduce `.jp-mod-readWrite` class exposing `:read-write`

Contrarily to `:read-write` the new class can be used to
detect the focus state of editable elements buried in the
open shadow DOM.

* Document keyboard interactions in outputs/`lm-suppress-shortcuts`

* Implement `.jp-mod-readWrite` in console too,

rename and move the utility function (`hasActiveEditableElement`)
to DOMUtils as it is now used in both notebook and console,
add tests for this function.

* Keep the `:not(:read-write)` part on `[data-jp-kernel-user]`

to err on the side of caution, as these could be used in donwstream
widgets for which we do not control the DOM so these could lack the
newly added `:not(.jp-mod-readWrite)`

* Fix a typo in a test file

* Align the selector migration rule

* Use `toggle` to simplify the code

Co-authored-by: Nicolas Brichet <32258950+brichet@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: gabalafou <gabriel@fouasnon.com>

* Correct TODO comment

Co-authored-by: gabalafou <gabriel@fouasnon.com>

* Fix hanging bracket

---------

Co-authored-by: Nicolas Brichet <32258950+brichet@users.noreply.github.com>
Co-authored-by: gabalafou <gabriel@fouasnon.com>
  • Loading branch information
3 people committed Feb 16, 2024
1 parent 51d6956 commit 1a98dea
Show file tree
Hide file tree
Showing 11 changed files with 321 additions and 95 deletions.
21 changes: 17 additions & 4 deletions docs/source/extension/extension_migration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,12 @@ between cells, especially impacting users with accessibility needs.
In JupyterLab 4.1+ the focus stays on the active cell when switching to command
mode; this requires all shortcut selectors to be adjusted as follows:

- ``.jp-Notebook:focus.jp-mod-commandMode`` should be replaced with ``.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)``
- ``.jp-Notebook:focus`` should be replaced with ``.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)``
- ``[data-jp-traversable]:focus`` should be replaced with ``.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)``
- ``[data-jp-kernel-user]:focus`` should be replaced with ``[data-jp-kernel-user] :focus:not(:read-write)``
- ``.jp-Notebook:focus.jp-mod-commandMode``, ``.jp-Notebook:focus``, and ``[data-jp-traversable]:focus`` should be replaced with:
- ``.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)`` for JupyterLab 4.1.0+
- ``.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus`` for JupyterLab 4.1.1+
- ``[data-jp-kernel-user]:focus`` should be replaced with:
- ``[data-jp-kernel-user] :focus:not(:read-write)`` for JupyterLab 4.1.0+
- ``[data-jp-kernel-user]:not(.jp-mod-readWrite) :focus:not(:read-write)`` for JupyterLab 4.1.1+

Please note that ``:not(:read-write)`` fragment disables shortcuts
when text fields (such as cell editor) are focused to avoid intercepting
Expand All @@ -101,6 +103,17 @@ does not correspond to any key/typographic character (e.g. most shortcuts
with :kbd:`Ctrl` modifier) you may prefer to drop this fragment
if you want the shortcut to be active in text fields.

Further, JupyterLab 4.1.1 introduced indicator class ``.jp-mod-readWrite``
that is applied to the notebook node when the active element accepts
keyboard input as defined by ``:read-write`` selector. This indicator
class is required to detect ``:read-write`` elements which are nested
within an *open* shadow DOM (such as Panel framework widgets).

If your framework uses a *closed* shadow DOM, or expects keyboard
interactions on elements that are not recognised as editable by browser
heuristics of ``:read-write`` selector, you need to set a data attribute
`lm-suppress-shortcuts` on the outer host element to suppress shortcuts.

To prevent breaking the user experience these changes are made transparently
in the background, but will emit a warning and extension developers should
make the change at the source before the next major JupyterLab release.
Expand Down
37 changes: 37 additions & 0 deletions docs/source/extension/notebook.rst
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,43 @@ The ipywidgets widget manager is an example of an extension that adds a
notebook-specific renderer, since rendering a widget depends on
notebook-specific widget state.

Keyboard interaction model
""""""""""""""""""""""""""

Multiple elements can receive focus in the Notebook:
- the main toolbar,
- cells,
- cell components (editor, toolbar, outputs).

When the focus is outside of the cell input editor,
the Notebook switches to so-called "command" mode.
In the command mode additional keyboard shortcuts are accessible to the user,
enabling quick access to cell- and notebook-specific actions.
These shortcuts are only active when the notebook is in command mode
and the active element is non-editable,
as signalled by absence of ``.jp-mod-readWrite`` class on the notebook node.
This class is set if the active element is editable as ascertained by matching
to the ``:read-write`` pseudo-selector, and accounts for any elements nested
in the open shadow DOM, but not for the closed shadow DOM nor non-editable
elements with custom key event handlers (such as
``<div contenteditable="false" onkeydown="alert()" tabindex="0"></div>``).
If your output widget (for example created with ``IPython.display.HTML``,
or created by your MIME renderer on cell output in a notebook or console)
uses closed shadow DOM or non-editable elements with custom
key event handlers, you may wish to set ``lm-suppress-shortcuts`` data attribute
on the host element to prevent side-effects from the command-mode actions, e.g:

.. code:: html

<div
contenteditable="false"
onkeydown="alert()"
tabindex="1"
data-lm-suppress-shortcuts="true"
>
Click on me and press "A" with and without "lm-suppress-shortcuts"
</div>

.. _extend-notebook-plugin:

How to extend the Notebook plugin
Expand Down
24 changes: 12 additions & 12 deletions examples/notebook/src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -396,17 +396,17 @@ export const setupCommands = (
command: COMMAND_IDS.findPrevious
},
{
selector: '.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['I', 'I'],
command: COMMAND_IDS.interrupt
},
{
selector: '.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['0', '0'],
command: COMMAND_IDS.restart
},
{
selector: '.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['Enter'],
command: COMMAND_IDS.editMode
},
Expand All @@ -416,7 +416,7 @@ export const setupCommands = (
command: COMMAND_IDS.commandMode
},
{
selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['Shift M'],
command: COMMAND_IDS.merge
},
Expand All @@ -426,42 +426,42 @@ export const setupCommands = (
command: COMMAND_IDS.split
},
{
selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['J'],
command: COMMAND_IDS.selectBelow
},
{
selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['ArrowDown'],
command: COMMAND_IDS.selectBelow
},
{
selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['K'],
command: COMMAND_IDS.selectAbove
},
{
selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['ArrowUp'],
command: COMMAND_IDS.selectAbove
},
{
selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['Shift K'],
command: COMMAND_IDS.extendAbove
},
{
selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['Shift J'],
command: COMMAND_IDS.extendBelow
},
{
selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['Z'],
command: COMMAND_IDS.undo
},
{
selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
selector: '.jp-Notebook.jp-mod-commandMode:not(.jp-mod-readWrite) :focus',
keys: ['Y'],
command: COMMAND_IDS.redo
}
Expand Down
64 changes: 39 additions & 25 deletions galata/test/jupyterlab/outputarea-stdin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ print('before sleep')
sleep(0.1)
print('after sleep')`;

const openShadowDOM = `\
from IPython.display import HTML
HTML("""<div id='root'></div><script>
document.querySelector('#root').attachShadow({mode: 'open'}).innerHTML = '<input id="shadow-input"/>';
</script>""")`;

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

Expand Down Expand Up @@ -67,31 +73,39 @@ test.describe('Stdin for ipdb', () => {
await expect(page.locator(ACTIVE_INPUT)).toHaveValue('foofoox');
});

test('Typing in stdin box', async ({ page }) => {
// Test to ensure that notebook shortcuts do not capture text typed into inputs.
// This may not be sufficient to ensure no conflicts with other languages but
// should catch the most severe issues.
const alphabet = 'abcdefghijklmnopqrstuvwxyz';
const digits = '0123456789';
await page.notebook.setCell(0, 'code', 'input()');
// 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');

await page.waitForSelector('.jp-Stdin-input');
for (const letter of alphabet) {
await page.keyboard.press(`Key${letter.toUpperCase()}`);
}
for (const letter of alphabet) {
await page.keyboard.press(`Shift+Key${letter.toUpperCase()}`);
}
for (const digit of digits) {
await page.keyboard.press(`Digit${digit}`);
}
await expect(page.locator('.jp-Stdin-input')).toHaveValue(
alphabet + alphabet.toUpperCase() + digits
);
});
const typingScenarios = [
{ name: 'stdin box', code: 'input()', selector: '.jp-Stdin-input' },
{ name: 'shadow DOM input', code: openShadowDOM, selector: '#shadow-input' }
];
for (const testCase of typingScenarios) {
test(`Typing in ${testCase.name}`, async ({ page }) => {
// Test to ensure that notebook shortcuts do not capture text typed into inputs.
// This may not be sufficient to ensure no conflicts with other languages but
// should catch the most severe issues.
const alphabet = 'abcdefghijklmnopqrstuvwxyz';
const digits = '0123456789';
await page.notebook.setCell(0, 'code', testCase.code);
// 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');

await page.waitForSelector(testCase.selector);
await page.focus(testCase.selector);

for (const letter of alphabet) {
await page.keyboard.press(`Key${letter.toUpperCase()}`);
}
for (const letter of alphabet) {
await page.keyboard.press(`Shift+Key${letter.toUpperCase()}`);
}
for (const digit of digits) {
await page.keyboard.press(`Digit${digit}`);
}
await expect(page.locator(testCase.selector)).toHaveValue(
alphabet + alphabet.toUpperCase() + digits
);
});
}

test('Subsequent execution in short succession', async ({ page }) => {
await page.notebook.setCell(0, 'code', loopedInput);
Expand Down
18 changes: 18 additions & 0 deletions packages/apputils/src/domutils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,22 @@ export namespace DOMUtils {
export function createDomID(): string {
return `id-${UUID.uuid4()}`;
}

/**
* Check whether the active element descendant from given parent is editable.
* When checking active elements it includes elements in the open shadow DOM.
*/
export function hasActiveEditableElement(
parent: Node | DocumentFragment,
root: ShadowRoot | Document = document
): boolean {
const element = root.activeElement;
return !!(
element &&
parent.contains(element) &&
(element.matches(':read-write') ||
(element.shadowRoot &&
hasActiveEditableElement(element.shadowRoot, element.shadowRoot)))
);
}
}
68 changes: 68 additions & 0 deletions packages/apputils/test/domutils.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Copyright (c) Jupyter Development Team.
// Distributed under the terms of the Modified BSD License.

import { DOMUtils } from '@jupyterlab/apputils';

describe('@jupyterlab/apputils', () => {
describe('DOMUtils', () => {
describe('hasActiveEditableElement', () => {
const testCases = [
// editable elements
['.input', true],
['.textarea', true],
['.div-editable', true],
// non-editable elements
['.input-readonly', false],
['.textarea-readonly', false],
['.div', false]
];

const div = document.createElement('div');
div.innerHTML = `
<div class="light-host">
<input class="input" />
<input class="input-readonly" readonly />
<textarea class="textarea"></textarea>
<textarea class="textarea-readonly" readonly></textarea>
<div class="div" tabindex="1"></div>
<div class="div-editable" contenteditable="true" tabindex="1"></div>
</div>
<div class="shadow-host">
</div>
`;
document.body.appendChild(div);

const lightHost = div.querySelector('.light-host')!;
const shadowHost = div.querySelector('.shadow-host')!;
const shadowRoot = shadowHost.attachShadow({ mode: 'open' });
// mirror test cases from light DOM in the shadow DOM
shadowRoot.innerHTML = lightHost.innerHTML;

it.each(testCases)(
'should work in light DOM: `%s` element should result in `%s`',
(selector, expected) => {
const element = lightHost.querySelector(
selector as string
) as HTMLElement;
element.focus();

const result = DOMUtils.hasActiveEditableElement(div);
expect(result).toBe(expected);
}
);

it.each(testCases)(
'should work in shadow DOM: `%s` element should result in `%s`',
(selector, expected) => {
const element = shadowRoot.querySelector(
selector as string
) as HTMLElement;
element.focus();

const result = DOMUtils.hasActiveEditableElement(div);
expect(result).toBe(expected);
}
);
});
});
});

0 comments on commit 1a98dea

Please sign in to comment.