Display shortcuts #14053

HaudinFlorence · 2023-02-20T21:36:59Z

References

Supersedes PR from #11960

Code changes

Changes are mainly style changes for the display of the shortcuts, also changes on the level of details/ the choice of informations to be displayed.

User-facing changes

Backwards-incompatible changes

jupyterlab-probot · 2023-02-20T21:37:00Z

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

krassowski

Thank you for picking this up! Some quick feedback.

krassowski · 2023-02-20T22:24:44Z

packages/apputils-extension/src/shortcuts.tsx

+      /* <DEPRECATED> */
+      if (targ.hasAttribute('data-p-suppress-shortcuts')) {
+        return -1;
+      }
+      /* </DEPRECATED> */


Safe to remove, we dropped p- prefixes in Lumino 2.0 the current implementation here does not include it.

Suggested change

/* <DEPRECATED> */

if (targ.hasAttribute('data-p-suppress-shortcuts')) {

return -1;

}

/* </DEPRECATED> */

It feels to me that we should just change signature of targetDistance in lumino, expose it publically and reuse here. This can be done in a follow up.

packages/apputils-extension/src/shortcuts.tsx

krassowski · 2023-02-20T22:28:46Z

packages/apputils-extension/src/shortcuts.tsx

+        groupedBindings.get(d)!.map(b => (
+          <tr key={b.command}>
+            <td
+              style={{


We should prefer defining styles in CSS files to enable consistent theming and use our CSS variables whenever possible (font size, colour, etc).

@krassowski Thanks for the comments. They should have been taken into account. Concerning the css properties, some of them seem not to be taken into account when defined in the css file like border-style, border-width, font-size for instance.

packages/apputils-extension/src/shortcuts.tsx

packages/apputils-extension/package.json

fcollonval

Thanks @HaudinFlorence

Could you add a integration test for this one?

You can add it in galata/test/documentation/general.test.ts around Line 490. The steps would be to open a notebook, trigger the display of the help dialog, check the dialog; something like:

  test('Command Palette', async ({ page, tmpPath }) => {
    await page.goto(`tree/${tmpPath}`);

    await page.notebook.createNew();

    await page.keyboard.press('Control+Shift+H');

    await expect(page.locator('.jp-Dialog')).toHaveScreenshot('shortcuts_help.png');
  });

This test will compare screenshots. At the end it is better as we will be able to use the screenshots for documentation purpose.

packages/apputils-extension/package.json

packages/apputils-extension/src/shortcuts.tsx

packages/filebrowser-extension/src/index.ts

yarn.lock

packages/apputils-extension/src/index.ts

jtpio · 2023-02-23T08:34:22Z

It would indeed be nice to have the shortcuts command available as a menu item and via the command palette.

The logic would be similar to the license plugin with IMainMenu and ICommandPalette defined as optional here:

jupyterlab/packages/help-extension/src/index.tsx

Line 458 in 3ecf75a

optional: [IMainMenu, ICommandPalette, ILayoutRestorer],

Which are then used here:

jupyterlab/packages/help-extension/src/index.tsx

Lines 595 to 603 in 3ecf75a

    
           // handle optional integrations 
        
           if (palette) { 
        
             palette.addItem({ command: CommandIDs.licenses, category }); 
        
           } 
        
           if (menu) { 
        
             const helpMenu = menu.helpMenu; 
        
             helpMenu.addGroup([{ command: CommandIDs.licenses }], 0); 
        
           }

It seems that the shortcuts logic in this PR looks for the document.activeElement though, not sure how this will work when the command is executed via the menu or the palette.

The main advantage of having the shortcuts available from the menu and palette would be that the downstream plugin in Notebook 7 could then be removed to use the one from JupyterLab instead.

HaudinFlorence · 2023-02-23T23:42:43Z

It would indeed be nice to have the shortcuts command available as a menu item and via the command palette.

The logic would be similar to the license plugin with IMainMenu and ICommandPalette defined as optional here:

jupyterlab/packages/help-extension/src/index.tsx

Line 458 in 3ecf75a

optional: [IMainMenu, ICommandPalette, ILayoutRestorer],

Which are then used here:

jupyterlab/packages/help-extension/src/index.tsx

Lines 595 to 603 in 3ecf75a

// handle optional integrations

if (palette) {

palette.addItem({ command: CommandIDs.licenses, category });

}

if (menu) {

const helpMenu = menu.helpMenu;

helpMenu.addGroup([{ command: CommandIDs.licenses }], 0);

}

It seems that the shortcuts logic in this PR looks for the document.activeElement though, not sure how this will work when the command is executed via the menu or the palette.

The main advantage of having the shortcuts available from the menu and palette would be that the downstream plugin in Notebook 7 could then be removed to use the one from JupyterLab instead.

@jtpio Thanks for your help. From what I see at first sight (not checked in detail) and as I was suspecting, the contextual help seems to always be the same when opened from the help menu or from the command palette.

HaudinFlorence · 2023-02-24T08:40:13Z

bot please update snapshots

fcollonval

@HaudinFlorence here are some suggestions that should fix the CI.

packages/apputils-extension/src/index.ts

packages/apputils-extension/style/base.css

yarn.lock

github-actions · 2023-02-24T09:29:35Z

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

The mean relative comparison is computed with 95% confidence.

Results table

Test file	large_code_notebook	large_md_notebook
open
`chromium`
actual	676 <- [753 - 799 - 833] -> 1077	872 <- [1563 - 1688 - 1779] -> 2007
expected	683 <- [758 - 804 - 850] -> 1184	801 <- [1184 - 1633 - 1716] -> 2054
Mean relative change	⚠️ -1.0% ± 2.6%	3.5% ± 6.2%
switch-from
`chromium`
actual	819 <- [885 - 914 - 947] -> 1066	383 <- [473 - 494 - 526] -> 628
expected	795 <- [894 - 923 - 946] -> 1162	389 <- [450 - 476 - 504] -> 643
Mean relative change	⚠️ -0.8% ± 1.5%	3.9% ± 2.7%
switch-to
`chromium`
actual	1522 <- [1671 - 1749 - 1814] -> 2089	1149 <- [1214 - 1252 - 1288] -> 1405
expected	1582 <- [1720 - 1780 - 1867] -> 2138	1070 <- [1174 - 1205 - 1238] -> 1368
Mean relative change	⚠️ -2.5% ± 1.7%	3.9% ± 1.2%
close
`chromium`
actual	156 <- [196 - 205 - 219] -> 258	248 <- [276 - 289 - 301] -> 420
expected	171 <- [197 - 204 - 215] -> 267	228 <- [267 - 284 - 296] -> 322
Mean relative change	⚠️ 0.0% ± 2.3%	3.2% ± 2.4%

Changes are computed with expected as reference.

@jupyterlab/benchmarks@1.0.0 test:mocha
mocha ./tests/

Waiting for localhost:8888
localhost:8888 is up

Cell memory leaks

- Drag and drop a cell

File editor memory leaks

Create a file

Memory change: -77.3 kB Leak detected: No

Leaking objects:

Object	# added	Retained size increase
Detached HTMLUListElement	1	+3.17 kB
Detached V8EventHandlerNonNull	1	+40 B
VirtualElementPass	1	+46 B
Detached HTMLButtonElement	2	+23.8 kB
Detached SVGCircleElement	2	+1.12 kB
LabIcon	2	+1.08 kB
Detached CSSStyleDeclaration	3	+228 B
Detached HTMLLIElement	3	+6.11 kB
Detached NodeList	3	+204 B
NavigationHistoryEntry	3	+720 B
VirtualText	3	+109 B
Detached SVGGElement	4	+3 kB
VirtualElement	4	+962 B
Detached SVGAnimatedPreserveAspectRatio	5	+320 B
Detached SVGAnimatedRect	5	+320 B
Detached SVGPathElement	5	+1.81 kB
Detached SVGSVGElement	5	+8.15 kB
Detached HTMLSpanElement	6	+2.42 kB
Detached DOMStringMap	7	+424 B
Detached SVGAnimatedNumber	7	+448 B
Detached Attr	10	+800 B
Detached Text	10	+996 B
Detached HTMLCollection	13	+1.42 kB
Detached DOMTokenList	14	+928 B
Detached HTMLDivElement	15	+33.6 kB
Detached SVGAnimatedString	16	+896 B
Detached SVGAnimatedTransformList	16	+1.02 kB
Detached SVGAnimatedLength	26	+1.66 kB
Array	42	+23.6 kB
Detached V8EventListener	130	+10.9 kB
Detached EventListener	131	+20.4 kB
(closure)	138	+26.1 kB

✔ Opening a text file (86048ms)

Notebook memory leaks

Create a notebook

Memory change: +31.2 kB Leak detected: Yes

Leaking objects:

Object	# added	Retained size increase
Detached HTMLUListElement	1	+2.3 kB
Detached V8EventHandlerNonNull	1	+40 B
VirtualElementPass	1	+46 B
Detached HTMLButtonElement	2	+23.7 kB
Detached SVGCircleElement	2	+1.12 kB
HTMLDivElement	2	+16 B
LabIcon	2	+85 B
Detached CSSStyleDeclaration	3	+196 B
Detached HTMLLIElement	3	+6.13 kB
Detached NodeList	3	+180 B
NavigationHistoryEntry	3	+720 B
VirtualText	3	+109 B
VirtualElement	4	+1.09 kB
Detached SVGAnimatedPreserveAspectRatio	5	+320 B
Detached SVGAnimatedRect	5	+320 B
Detached SVGSVGElement	5	+9.39 kB
Detached HTMLSpanElement	6	+3.14 kB
Detached SVGGElement	6	+5.02 kB
Detached SVGPathElement	7	+2.53 kB
Detached SVGAnimatedNumber	9	+576 B
Detached Attr	10	+800 B
Detached Text	10	+768 B
Detached HTMLCollection	13	+1.35 kB
Detached DOMTokenList	15	+900 B
Detached HTMLDivElement	15	+33.3 kB
Detached SVGAnimatedString	20	+1.12 kB
Detached SVGAnimatedTransformList	20	+1.28 kB
Detached SVGAnimatedLength	26	+1.66 kB
Detached V8EventListener	130	+10.9 kB
Detached EventListener	131	+20.4 kB
(closure)	138	+43.1 kB

✔ Opening a notebook (70233ms)

2 passing (3m)
1 pending
1 failing

   Adding a cell:
 TimeoutError: Waiting for selector `.//div[contains(@class, "lm-TabBar-tabLabel")][text()="Launcher"]` failed: Waiting failed: 30000ms exceeded
  at Timeout.<anonymous> (file:///home/runner/work/_actions/jupyterlab/benchmarks/v1/memory-leaks/node_modules/puppeteer-core/lib/esm/puppeteer/common/WaitTask.js:60:32)
  at listOnTimeout (node:internal/timers:569:17)
  at process.processTimers (node:internal/timers:512:7)

packages/apputils-extension/src/index.ts

jtpio · 2023-02-24T12:32:42Z

From what I see at first sight (not checked in detail) and as I was suspecting, the contextual help seems to always be the same (the one of the notebook it seems) when opened from the help menu or from the command palette.

Ah ok thanks @HaudinFlorence for looking into this. Then we could just skip adding the command to the main menu and command palette for now. I might check it out later separately to not block this PR.

fcollonval · 2023-02-24T14:37:03Z

Kicking the CI

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

…s for paddings. Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com> Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> Co-authored-by: Andrii Ieroshenko <ieroshenkoa@gmail.com>

…exists.

…shop.

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

andrii-i

Thank you for the updates @HaudinFlorence. Updated Keyboard Shortcuts table and help menu item look good to me and work well. Good job!

Note #14240 (comment) interaction for when some extension registers a keyboard shortcut with a long description but this can be dealt with as a part of V2.

Please also note that currently there are 2 failing tests:

Visual Regression. There is screenshot comparasion fail for newly-added Shortcuts tests: test/documentation/general.test.ts:582:7 › General › Keyboard Shortcuts Help . This comment by @fcollonval and one under it describe how to run and debug visual regression tests locally.
Linux (usage, 3.11. There is something going on with duplicate package and then a failure on trying to clone cookiecutter package. Not exactly sure if this is connected to this PR, maybe other reviewers who understand CI better could advise, CC @krassowski @fcollonval

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/home/runner/work/jupyterlab/jupyterlab/jupyterlab/upgrade_extension.py", line 201, in <module>
    update_extension(args.path, args.branch, args.no_input is False)
  File "/home/runner/work/jupyterlab/jupyterlab/jupyterlab/upgrade_extension.py", line 80, in update_extension
    cookiecutter(
  File "/opt/hostedtoolcache/Python/3.11.2/x64/lib/python3.11/site-packages/cookiecutter/main.py", line 69, in cookiecutter
    repo_dir, cleanup = determine_repo_dir(
                        ^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.2/x64/lib/python3.11/site-packages/cookiecutter/repository.py", line 106, in determine_repo_dir
    cloned_repo = clone(
                  ^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.2/x64/lib/python3.11/site-packages/cookiecutter/vcs.py", line 95, in clone
    subprocess.check_output(  # nosec
  File "/opt/hostedtoolcache/Python/3.11.2/x64/lib/python3.11/subprocess.py", line 466, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.2/x64/lib/python3.11/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['git', 'clone', 'https://github.com/jupyterlab/extension-cookiecutter-ts']' returned non-zero exit status 128.

fcollonval · 2023-04-03T13:51:52Z

bot please update documentation snapshots

github-actions · 2023-04-03T14:16:22Z

Documentation snapshots updated.

fcollonval

Thanks @HaudinFlorence

Merging this first version

github-actions bot assigned HaudinFlorence Feb 20, 2023

github-actions bot added pkg:apputils pkg:filebrowser labels Feb 20, 2023

krassowski added enhancement tag:Feature Parity labels Feb 20, 2023

krassowski added this to the 4.0.0 milestone Feb 20, 2023

krassowski reviewed Feb 20, 2023

View reviewed changes

fcollonval changed the title ~~Display shortcuts superseed~~ Display shortcuts Feb 21, 2023

HaudinFlorence force-pushed the display_shortcuts_superseed branch from 7292d36 to cfd40dd Compare February 21, 2023 23:02

github-actions bot added Design System CSS tag:CSS For general CSS related issues and pecadilloes labels Feb 21, 2023

HaudinFlorence force-pushed the display_shortcuts_superseed branch 3 times, most recently from 0e6acbc to c66351a Compare February 22, 2023 08:15

fcollonval requested changes Feb 22, 2023

View reviewed changes

jtpio mentioned this pull request Feb 22, 2023

Update to JupyterLab 4.0.0a35 jupyter/notebook#6734

Closed

7 tasks

github-actions bot added the tag:Testing label Feb 22, 2023

fcollonval requested changes Feb 24, 2023

View reviewed changes

github-advanced-security bot found potential problems Feb 24, 2023

View reviewed changes

packages/apputils-extension/src/index.ts Fixed Show fixed Hide fixed

HaudinFlorence force-pushed the display_shortcuts_superseed branch 2 times, most recently from 88249e3 to fc055a0 Compare February 24, 2023 11:04

fcollonval closed this Feb 24, 2023

fcollonval reopened this Feb 24, 2023

github-actions bot and others added 15 commits March 31, 2023 15:40

Update Playwright Snapshots

b2d9915

Apply suggestions from code review

3fa9cf7

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

Add check to see wether currentWidget contains the activeElement or not.

7318353

Change the style of the shortcuts interface.

38b10dc

Package integrity updates

a502083

Package integrity updates

bfdd08d

Update Playwright Snapshots

552fff5

Add automatic labels using function formatLabel for cases where none …

64cc0b8

…exists.

Update the style of the table to try to fit actions items of the work…

280ae3e

…shop.

Apply suggestions from code review

ff4c2d9

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>

Update style of the table of the shortcuts display.

03f7a13

Update Playwright Snapshots

37cab0c

Apply suggestions from code review

cea2d66

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

Add a padding between the label and the shortcut combination.

218e376

fcollonval force-pushed the display_shortcuts_superseed branch from 816caa9 to 218e376 Compare March 31, 2023 13:40

andrii-i reviewed Mar 31, 2023

View reviewed changes

Update snapshots

1a06c6f

fcollonval force-pushed the display_shortcuts_superseed branch from 3066161 to 1a06c6f Compare April 3, 2023 15:07

jtpio mentioned this pull request Apr 5, 2023

V7 many shortcut displayed many times in help jupyter/notebook#6823

Closed

fcollonval approved these changes Apr 5, 2023

View reviewed changes

fcollonval merged commit 7b97b44 into jupyterlab:master Apr 6, 2023

jtpio mentioned this pull request Apr 7, 2023

Update to JupyterLab 4.0.0b1 and 4.0.0b2 jupyter/notebook#6838

Merged

5 tasks

fcollonval mentioned this pull request May 9, 2023

Display keyboard shortcuts #11960

Closed

jtpio mentioned this pull request May 25, 2023

Make all shortcuts discoverable #2637

Closed

krassowski mentioned this pull request Aug 30, 2023

[WIP] Add user accessibility documentation #15001

Closed

github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2024

HaudinFlorence deleted the display_shortcuts_superseed branch September 16, 2025 08:21

Uh oh!

Display shortcuts #14053

Display shortcuts #14053

Uh oh!

Conversation

HaudinFlorence commented Feb 20, 2023 • edited by JasonWeill Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Code changes

User-facing changes

Backwards-incompatible changes

Uh oh!

jupyterlab-probot bot commented Feb 20, 2023

Uh oh!

krassowski left a comment

Choose a reason for hiding this comment

Uh oh!

krassowski Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

krassowski Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

krassowski Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

HaudinFlorence Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fcollonval left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtpio commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HaudinFlorence commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HaudinFlorence commented Feb 24, 2023

Uh oh!

fcollonval left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark report

Cell memory leaks

File editor memory leaks

Notebook memory leaks

Uh oh!

Uh oh!

jtpio commented Feb 24, 2023

Uh oh!

fcollonval commented Feb 24, 2023

Uh oh!

andrii-i left a comment

Choose a reason for hiding this comment

Uh oh!

fcollonval commented Apr 3, 2023

HaudinFlorence commented Feb 20, 2023 •

edited by JasonWeill

Loading

HaudinFlorence Feb 21, 2023 •

edited

Loading

jtpio commented Feb 23, 2023 •

edited

Loading

HaudinFlorence commented Feb 23, 2023 •

edited

Loading

github-actions bot commented Feb 24, 2023 •

edited

Loading