Skip to content
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

Typing into into fields that are wrapped with shadow DOM is intercepted by commands #15757

Closed
krassowski opened this issue Feb 7, 2024 · 4 comments · Fixed by #15774
Closed
Labels
bug pkg:notebook tag:Regression Behavior that had been broken, was fixed, and is broken again
Milestone

Comments

@krassowski
Copy link
Member

krassowski commented Feb 7, 2024

Description

Typing most characters into fields wrapped in shadow DOM, such as widgets generated by panel, is impossible because command shortcuts intercept the keystrokes.

Reproduce

See holoviz/panel#6314, but briefly:

import panel as pn
pn.extension()
text_input = pn.widgets.TextInput(name='Text Input', placeholder='Enter a string starting with "A" or "B" here...')
text_input

and try to type A, C, X, etc.

Dependency-free reproducer:

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

Note: iframes work as they also isolate keystrokes.

from IPython.display import HTML
HTML("<iframe srcdoc='<input>'></iframe>")
from IPython.display import HTML
HTML("<iframe srcdoc='<div><template shadowrootmode=\"open\"><input></template></div>' />")

Expected behavior

Typing works as in 4.0 and earlier.

Context

  • JupyterLab version: 4.1.0
@krassowski krassowski added bug pkg:notebook status:Needs Triage Applied to new issues that need triage tag:Regression Behavior that had been broken, was fixed, and is broken again labels Feb 7, 2024
@krassowski krassowski added this to the 4.1.x milestone Feb 7, 2024
@krassowski
Copy link
Member Author

krassowski commented Feb 7, 2024

This is because of the rework of focus handling and command selectors in #14115 that fixed errant scrolling to the top and accessibility issues. Technically, the problem is that the guard (:focus:not(:read-write)) in the new selector, .jp-Notebook.jp-mod-commandMode :focus:not(:read-write) does not see into the shadow DOM.

Unfortunately, the ::shadow and /deep/ pseudoselectors proposed few years back did not make it into the official spec and are no longer supported by any browser.

Here are two ideas:

  1. We could write a custom deepQuerySelector method that would be used by lumino to match even inside shadow DOM (see SO)
    • this will certainly have a big performance penalty so I do not want to do this, unless as a last resort
  2. We could use document.activeElement as a second-pass filter for command-mode commands. document.activeElement correctly returns the element within shadow DOM which could be tested for :focus:not(:read-write)
    • if implemented in individual commands this will be quite repetitive, and might not resolve the issue of the keypress event being prevented (but would avoid side effects such as cell deletion)
    • thus we may need to implement it on the command dispatcher level in lumino
    • while we could have this as a special treatment for selectors ending with :focus:not(:read-write), it raises a question if we should have a more generic solution to avoid inconsistencies, or if we should have a modified selector syntax to convey the "deep" modifier; we could try to use the deprecated /deep/ specifier, this is making it .jp-Notebook.jp-mod-commandMode /deep/ :focus:not(:read-write). (note: this is called shadow-piercing descendant combinator and has >>> alias; while no browsers implement it, some frameworks do).

Any thoughts or other ideas @brichet, @gabalafou?

@fcollonval
Copy link
Member

I'm wondering if we should not instead advertise the use of data-lm-suppress-shortcuts.

https://github.com/jupyterlab/lumino/blob/c0ee4a71c3db03817617a9e51cb6bf4e331eed27/packages/commands/src/index.ts#L1616

@krassowski
Copy link
Member Author

I agree that we should document data-lm-suppress-shortcuts for certain use cases (e.g. if someone has a div which is for some reasons accepting keyboard input), but I also believe we should support all outputs which use standard input elements out of the box.

@krassowski
Copy link
Member Author

krassowski commented Feb 8, 2024

I opened a draft PR in lumino demonstrating that implementation of the /deep/ selector for our purposes is rather trivial (jupyterlab/lumino#683). However, here are two more ideas which touch a lower API surface area:

  1. Use global focus listener to add/remove a .jp-mod-editable-has-focus class (or data attribute) to .jp-Notebook if the focused element matches :read-write; we would then modify the command selector accordingly
    • (-) might trigger CSS recalculation for the entire notebook; the impact of which is limited for windowed notebook, but potentially severe for non-windowed mode; this should not usually happen if we do not have styles conditioned on this class/attribute, but browsers use a bloom filter as a heuristic which may give false positives
    • (-) all command selectors need to be modified accordingly, including in extensions such as jupyterlab-vim
    • (+) will marginally reduce the cost of selector matching for commands (when we have many commands)
  2. Modify isEnabled of commands to make use of _luminoEvent introduced in Pass _luminoEvent argument when executing commands via keybinding lumino#644 and do the second pass filtering there.
    • (+) the least API surface area of all options
    • (-) every command used in Notebook command mode (including those from extensions, such as jupyterlab-vim) needs to be modified

Note: selector changes can be applied transparently during shortcuts loading so (2) with /deep/ or (3) might be better with respect to extent of changes needed downstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug pkg:notebook tag:Regression Behavior that had been broken, was fixed, and is broken again
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants