Fix composer crash when a list_item has no list around it (MAILSPRING-CLIENT-EV)

app/spec/components/composer-editor/base-block-plugins-spec.tsx

@@ -0,0 +1,100 @@
+import { Editor, Value } from 'slate';
+import { EditListPlugin } from '../../../src/components/composer-editor/base-block-plugins';
+
+const text = (t: string) => ({ object: 'text', leaves: [{ object: 'leaf', text: t, marks: [] }] });
+const block = (type: string, nodes: any[]) => ({ object: 'block', type, data: {}, nodes });
+const div = (t: string) => block('div', [text(t)]);
+const listItem = (...nodes: any[]) => block('list_item', nodes);
+const list = (...nodes: any[]) => block('ul_list', nodes);
+
+// Built without plugins on purpose: the schema would repair an orphaned `list_item`, and the
+// crash we're guarding against only happens in an editor that is no longer normalizing.
+function editorWith(nodes: any[]) {
+  return new Editor({
+    value: Value.fromJSON({
+      object: 'value',
+      document: { object: 'document', data: {}, nodes },
+    } as any),
+    plugins: [],
+    onChange: () => {},
+  });
+}
+
+function childTypes(editor: Editor) {
+  return (editor.value.document.nodes.toArray() as any[]).map((n) => n.type);
+}
+
+function pressKey(editor: Editor, key: string, textToFocus: string, offset = 0) {
+  const target = editor.value.document.getTexts().find((t) => t.text === textToFocus);
+  editor.moveTo(target.key as any, offset);
+
+  let passedToNext = false;
+  EditListPlugin.onKeyDown(
+    { key, shiftKey: false, preventDefault: () => {} } as any,
+    editor,
+    () => {
+      passedToNext = true;
+    }
+  );
+  return passedToNext;
+}
+
+describe('EditListPlugin', () => {
+  // MAILSPRING-CLIENT-EV: email HTML can contain an <li> with no <ul>/<ol> around it. Slate's
+  // `unwrapNodeByPath` lifts such a node's one-element path to the empty root path, which
+  // `getDescendant` resolves to null, and `splitNodeByPath` then reads `.type` off null.
+  ['Enter', 'Backspace', 'Tab'].forEach((key) => {
+    it(`ignores ${key} inside a list_item that is not in a list`, () => {
+      const editor = editorWith([div('before'), listItem(div('')), div('after')]);
+      let passedToNext = false;
+      expect(() => {
+        passedToNext = pressKey(editor, key, '');
+      }).not.toThrow();
+      expect(passedToNext).toBe(true);
+      expect(childTypes(editor)).toEqual(['div', 'list_item', 'div']);
+    });
+  });
+
+  it('ignores Enter inside an orphaned list_item that is the only node in the document', () => {
+    const editor = editorWith([listItem(div(''))]);
+    expect(() => pressKey(editor, 'Enter', '')).not.toThrow();
+  });
+
+  it('still exits the list when Enter is pressed in an empty item', () => {
+    const editor = editorWith([
+      div('before'),
+      list(listItem(div('x')), listItem(div(''))),
+      div('after'),
+    ]);
+    expect(pressKey(editor, 'Enter', '')).toBe(false);
+    expect(childTypes(editor)).toEqual(['div', 'ul_list', 'div', 'div']);
+  });
+
+  it('still splits the item when Enter is pressed mid-text', () => {
+    const editor = editorWith([div('before'), list(listItem(div('xy'))), div('after')]);
+    expect(pressKey(editor, 'Enter', 'xy', 1)).toBe(false);
+    const items = (editor.value.document.nodes.get(1) as any).nodes.toArray() as any[];
+    expect(items.map((n) => n.text)).toEqual(['x', 'y']);
+  });
+});
+
+describe('Slate withoutNormalizing patch', () => {
+  // Required here, not at module scope, since installing it mutates `Editor.prototype` for
+  // every other spec in the run and the `EditListPlugin` specs above don't need it — they
+  // build their editors with `plugins: []` so nothing ever normalizes. A `describe` body runs
+  // once, synchronously, before any of its `it`s, so this installs the patch exactly once.
+  require('../../../src/components/composer-editor/patch-slate-normalizing');
+
+  it('restores normalization when the callback throws', () => {
+    const editor = editorWith([div('hello')]);
+    expect((editor as any).tmp.normalize).toBe(true);
+
+    expect(() =>
+      editor.withoutNormalizing(() => {
+        throw new Error('simulated crash');
+      })
+    ).toThrow();
+
+    expect((editor as any).tmp.normalize).toBe(true);
+  });
+});

app/src/components/composer-editor/base-block-plugins.tsx

@@ -233,12 +233,39 @@ export const BLOCK_CONFIG: {
   },
 };
 
-export const EditListPlugin = new EditList({
+const EditListPluginBase = new EditList({
   types: [BLOCK_CONFIG.ol_list.type, BLOCK_CONFIG.ul_list.type],
   typeItem: BLOCK_CONFIG.list_item.type,
   typeDefault: BLOCK_CONFIG.div.type,
 });
 
+// `slate-edit-list` decides "is the cursor in a list?" with `getCurrentItem`, which returns
+// any parent block of type `list_item` — including one that isn't inside an `ol_list`/`ul_list`.
+// Email HTML regularly contains an `<li>` with no surrounding list, so that shape does reach
+// the composer. Handing Enter/Backspace to the plugin there routes into its `unwrapList`, whose
+// `unwrapNodeByKey` lifts the orphan's one-element path to the empty root path: Slate's
+// `assertNode` accepts that path (it resolves to the document itself) but `getDescendant`
+// returns null for it, so `splitNodeByPath` reads `.type` off null and throws
+// (MAILSPRING-CLIENT-EV). Fall through to the default handling when the item isn't really
+// in a list — the orphan then behaves like any other block.
+export const EditListPlugin = {
+  ...EditListPluginBase,
+  onKeyDown: (event: React.KeyboardEvent, editor: Editor, next: () => void) => {
+    // slate-edit-list's own onKeyDown only acts on these three keys; matching its gate here
+    // avoids running the guard below (a couple of tree walks) on every other keystroke.
+    if (event.key !== 'Enter' && event.key !== 'Tab' && event.key !== 'Backspace') {
+      return EditListPluginBase.onKeyDown(event, editor, next);
+    }
+    const { utils } = EditListPluginBase;
+    const { value } = editor;
+    // `startBlock` is the precondition `getCurrentItem` itself assumes.
+    if (value.startBlock && utils.getCurrentItem(value) && !utils.getCurrentList(value)) {
+      return next();
+    }
+    return EditListPluginBase.onKeyDown(event, editor, next);
+  },
+};
+
 function renderNode(props, editor: Editor = null, next = () => {}) {
   const config = BLOCK_CONFIG[props.node.type];
   return config ? config.render(props) : next();

app/src/components/composer-editor/conversion.tsx

@@ -25,6 +25,7 @@ import GrammarCheckPlugins from './grammar-check-plugins';
 import { Rule, ComposerEditorPlugin } from './types';
 
 import './patch-chrome-ime';
+import './patch-slate-normalizing';
 import { deepenPlaintextQuote } from './plaintext';
 
 export const schema = {

app/src/components/composer-editor/patch-slate-normalizing.ts

@@ -0,0 +1,28 @@
+/*
+Slate's `Editor#withoutNormalizing` clears `editor.tmp.normalize`, runs the callback, and then
+restores the flag — but it does so without a `try/finally`, so a command that throws from inside
+the callback leaves normalization disabled for the rest of that editor's life.
+
+Almost every structural Slate command (delete, insertFragment, splitDescendants, unwrapNode, ...)
+runs inside `withoutNormalizing`, and exceptions thrown from a React event handler don't unmount
+anything — we report them and the composer stays open. So a single Slate error means the document
+is never repaired again for the rest of the session, and structurally invalid nodes that the
+schema would normally fix (eg. a `list_item` with no list around it, deserialized from email HTML)
+survive to crash later commands.
+
+Restore the flag when the callback throws so one error can't cascade.
+*/
+import { Editor } from 'slate';
+
+const prototype = (Editor as any).prototype;
+const withoutNormalizing = prototype.withoutNormalizing;
+
+prototype.withoutNormalizing = function (fn: (editor: Editor) => void) {
+  const previous = this.tmp.normalize;
+  try {
+    return withoutNormalizing.call(this, fn);
+  } catch (err) {
+    this.tmp.normalize = previous;
+    throw err;
+  }
+};