Fix kernel shortcuts, add migration, fix defaults population (jupyter…

…lab#15639) * Fix default shortcuts and port old shortcuts transparently * Fix incorrect default for arrays in JSON setting editor * Do not populate non-required default fields from rsjf * Fix incorrect population of defaults in settings * Add a test against user settings getting mangled up * Add documentation in extension migration guide * Grammar * Correct variable name * Deduplicate warnings about selectors
GCabane · Jan 16, 2024 · 6c5cce7 · 6c5cce7
1 parent a6f6b9d
commit 6c5cce7
Show file tree

Hide file tree

Showing 7 changed files with 229 additions and 30 deletions.
diff --git a/docs/source/extension/extension_migration.rst b/docs/source/extension/extension_migration.rst
@@ -79,10 +79,38 @@ CSS class name change in the ``WindowedList`` superclass of ``StaticNotebook``
 - The notebook scroll container is now ``.jp-WindowedPanel-outer`` rather than ``.jp-Notebook``
 - Galata notebook helpers `getCell` and `getCellCount` were updated accordingly
 
+
+Change of notebook focus handling impacting command-mode shortcut selectors
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Previously, focus in the notebook would revert to the notebook HTML node
+when switching to command mode, which was preventing :kbd:`Tab` navigation
+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)``
+
+Please note that ``:not(:read-write)`` fragment disables shortcuts
+when text fields  (such as cell editor) are focused to avoid intercepting
+characters typed by the user into the text fields, however if your shortcut
+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.
+
+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.
+
+
 Visibility of ``StatusBar`` elements at high magnifications
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Statusbar items are now hidden by default at high magnification/low resolution to prevent overlap for those using the application at high magnifications.
-- An additional ``priority`` property has been added to the options of ``IStatusBar.registerStatusItem`` method to allow the status bar item to remain visible; the intended usage is for small statusbar items that either add functionality that would be particularly useful at high zoom or is inaccessible otherwise.
+- An additional ``priority`` property has been added to the options of ``IStatusBar.registerStatusItem`` method to allow the status bar item to remain visible;
+  the intended usage is for small statusbar items that either add functionality that would be particularly useful at high zoom or is inaccessible otherwise.
 
 JupyterLab 3.x to 4.x
 ---------------------

diff --git a/examples/notebook/src/commands.ts b/examples/notebook/src/commands.ts
@@ -396,17 +396,17 @@ export const setupCommands = (
       command: COMMAND_IDS.findPrevious
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: '.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['I', 'I'],
       command: COMMAND_IDS.interrupt
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: '.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['0', '0'],
       command: COMMAND_IDS.restart
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: '.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['Enter'],
       command: COMMAND_IDS.editMode
     },
@@ -416,7 +416,7 @@ export const setupCommands = (
       command: COMMAND_IDS.commandMode
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['Shift M'],
       command: COMMAND_IDS.merge
     },
@@ -426,42 +426,42 @@ export const setupCommands = (
       command: COMMAND_IDS.split
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['J'],
       command: COMMAND_IDS.selectBelow
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['ArrowDown'],
       command: COMMAND_IDS.selectBelow
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['K'],
       command: COMMAND_IDS.selectAbove
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['ArrowUp'],
       command: COMMAND_IDS.selectAbove
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['Shift K'],
       command: COMMAND_IDS.extendAbove
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['Shift J'],
       command: COMMAND_IDS.extendBelow
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['Z'],
       command: COMMAND_IDS.undo
     },
     {
-      selector: '.jp-Notebook.jp-mod-commandMode:focus',
+      selector: 'jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
       keys: ['Y'],
       command: COMMAND_IDS.redo
     }

diff --git a/galata/test/jupyterlab/settings.test.ts b/galata/test/jupyterlab/settings.test.ts
@@ -210,7 +210,7 @@ test('Check codemirror settings can all be set at the same time.', async ({
 
   await expect(page.locator('.jp-SettingsForm')).toHaveCount(1);
 
-  const textList: Array[string] = [
+  const textList: Array<string> = [
     'Code Folding',
     'Highlight the active line',
     'Highlight trailing white space',
@@ -226,3 +226,29 @@ test('Check codemirror settings can all be set at the same time.', async ({
     await expect(locator).toBeChecked();
   }
 });
+
+test('Opening Keyboard Shortcuts settings does not mangle user shortcuts', async ({
+  page
+}) => {
+  // Testing against https://github.com/jupyterlab/jupyterlab/issues/12056
+  await page.evaluate(async () => {
+    await window.jupyterapp.commands.execute('settingeditor:open', {
+      query: 'Keyboard Shortcuts'
+    });
+  });
+
+  await expect(page.locator('.jp-SettingsForm')).toHaveCount(1);
+
+  await page.evaluate(async () => {
+    await window.jupyterapp.commands.execute('settingeditor:open-json');
+  });
+
+  await expect(page.locator('#json-setting-editor')).toHaveCount(1);
+  await page.click(
+    '#json-setting-editor .jp-PluginList-entry[data-id="@jupyterlab/shortcuts-extension:shortcuts"]'
+  );
+  const userPanelLines = await page
+    .locator('.jp-SettingsRawEditor-user .cm-line')
+    .count();
+  expect(userPanelLines).toBeLessThan(10);
+});
diff --git a/packages/mainmenu-extension/schema/plugin.json b/packages/mainmenu-extension/schema/plugin.json
@@ -270,22 +270,22 @@
     {
       "command": "kernelmenu:interrupt",
       "keys": ["I", "I"],
-      "selector": "[data-jp-kernel-user]:focus"
+      "selector": "[data-jp-kernel-user] :focus:not(:read-write)"
     },
     {
       "command": "kernelmenu:restart",
       "keys": ["0", "0"],
-      "selector": "[data-jp-kernel-user]:focus"
+      "selector": "[data-jp-kernel-user] :focus:not(:read-write)"
     },
     {
       "command": "kernelmenu:restart-and-clear",
       "keys": [""],
-      "selector": "[data-jp-kernel-user]:focus"
+      "selector": "[data-jp-kernel-user] :focus:not(:read-write)"
     },
     {
       "command": "kernelmenu:shutdown",
       "keys": [""],
-      "selector": "[data-jp-kernel-user]:focus"
+      "selector": "[data-jp-kernel-user] :focus:not(:read-write)"
     },
     {
       "command": "runmenu:restart-and-run-all",

diff --git a/packages/settingeditor/src/SettingsFormEditor.tsx b/packages/settingeditor/src/SettingsFormEditor.tsx
@@ -221,6 +221,9 @@ export class SettingsFormEditor extends React.Component<
           idPrefix={`jp-SettingsEditor-${this.props.settings.id}`}
           onChange={this._onChange}
           translator={this.props.translator}
+          experimental_defaultFormStateBehavior={{
+            emptyObjectFields: 'populateRequiredDefaults'
+          }}
         />
       </>
     );

diff --git a/packages/settingregistry/src/settingregistry.ts b/packages/settingregistry/src/settingregistry.ts
@@ -1224,7 +1224,7 @@ export namespace SettingRegistry {
     });
 
     // Return all the shortcuts that should be registered
-    return (
+    return Private.upgradeShortcuts(
       user
         .concat(defaults)
         .filter(shortcut => !shortcut.disabled)
@@ -1437,14 +1437,25 @@ namespace Private {
 
   /**
    * Create a fully extrapolated default value for a root key in a schema.
+   *
+   * @todo This function would ideally reuse `getDefaultFormState` from rjsf
+   * with appropriate`defaultFormStateBehavior` setting, as currently
+   * these two implementations duplicate each other.
+   *
+   * Note: absence of a property may mean something else than the default.
    */
   export function reifyDefault(
     schema: ISettingRegistry.IProperty,
     root?: string,
-    definitions?: PartialJSONObject
+    definitions?: PartialJSONObject,
+    required?: boolean
   ): PartialJSONValue | undefined {
     definitions = definitions ?? (schema.definitions as PartialJSONObject);
     // If the property is at the root level, traverse its schema.
+    required = root
+      ? schema.required instanceof Array &&
+        schema.required?.includes(root as any)
+      : required;
     schema = (root ? schema.properties?.[root] : schema) || {};
 
     if (schema.type === 'object') {
@@ -1457,14 +1468,23 @@ namespace Private {
         result[property] = reifyDefault(
           props[property],
           undefined,
-          definitions
+          definitions,
+          schema.required instanceof Array &&
+            schema.required?.includes(property as any)
         );
       }
 
       return result;
     } else if (schema.type === 'array') {
+      const defaultDefined = typeof schema.default !== 'undefined';
+      const shouldPopulateDefaultArray = defaultDefined || required;
+      if (!shouldPopulateDefaultArray) {
+        return undefined;
+      }
       // Make a copy of the default value to populate.
-      const result = JSONExt.deepCopy(schema.default as PartialJSONArray);
+      const result = defaultDefined
+        ? JSONExt.deepCopy(schema.default as PartialJSONArray)
+        : [];
 
       // Items defines the properties of each item in the array
       let props = (schema.items as PartialJSONObject) || {};
@@ -1478,21 +1498,90 @@ namespace Private {
       }
       // Iterate through the items in the array and fill in defaults
       for (const item in result) {
-        // Use the values that are hard-coded in the default array over the defaults for each field.
-        const reified =
-          (reifyDefault(props, undefined, definitions) as PartialJSONObject) ??
-          {};
-        for (const prop in reified) {
-          if ((result[item] as PartialJSONObject)?.[prop]) {
-            reified[prop] = (result[item] as PartialJSONObject)[prop];
+        if (props.type === 'object') {
+          // Use the values that are hard-coded in the default array over the defaults for each field.
+          const reified =
+            (reifyDefault(
+              props,
+              undefined,
+              definitions
+            ) as PartialJSONObject) ??
+            result[item] ??
+            {};
+          for (const prop in reified) {
+            if ((result[item] as PartialJSONObject)?.[prop]) {
+              reified[prop] = (result[item] as PartialJSONObject)[prop];
+            }
           }
+          result[item] = reified;
         }
-        result[item] = reified;
       }
 
       return result;
     } else {
       return schema.default;
     }
   }
+
+  /**
+   * Selectors which were previously warned about.
+   */
+  const selectorsAlreadyWarnedAbout = new Set<string>();
+
+  /**
+   * Upgrade shortcuts to ensure no breaking changes between minor versions.
+   */
+  export function upgradeShortcuts(
+    shortcuts: ISettingRegistry.IShortcut[]
+  ): ISettingRegistry.IShortcut[] {
+    const selectorDeprecationWarnings = new Set();
+    const changes = [
+      {
+        old: '.jp-Notebook:focus.jp-mod-commandMode',
+        new: '.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
+        versionDeprecated: 'JupyterLab 4.1'
+      },
+      {
+        old: '.jp-Notebook:focus',
+        new: '.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
+        versionDeprecated: 'JupyterLab 4.1'
+      },
+      {
+        old: '[data-jp-traversable]:focus',
+        new: '.jp-Notebook.jp-mod-commandMode :focus:not(:read-write)',
+        versionDeprecated: 'JupyterLab 4.1'
+      },
+      {
+        old: '[data-jp-kernel-user]:focus',
+        new: '[data-jp-kernel-user] :focus:not(:read-write)',
+        versionDeprecated: 'JupyterLab 4.1'
+      }
+    ];
+    const upgraded = shortcuts.map(shortcut => {
+      const oldSelector = shortcut.selector;
+      let newSelector = oldSelector;
+      for (const change of changes) {
+        if (
+          oldSelector.includes(change.old) &&
+          !selectorsAlreadyWarnedAbout.has(oldSelector)
+        ) {
+          newSelector = oldSelector.replace(change.old, change.new);
+          selectorDeprecationWarnings.add(
+            `"${change.old}" was replaced with "${change.new}" in ${change.versionDeprecated} (present in "${oldSelector}")`
+          );
+          selectorsAlreadyWarnedAbout.add(oldSelector);
+        }
+      }
+      shortcut.selector = newSelector;
+      return shortcut;
+    });
+    if (selectorDeprecationWarnings.size > 0) {
+      console.warn(
+        'Deprecated shortcut selectors: ' +
+          [...selectorDeprecationWarnings].join('\n') +
+          '\n\nThe selectors will be substituted transparently this time, but need to be updated at source before next major release.'
+      );
+    }
+    return upgraded;
+  }
 }