Rich-text fixes (Mobile, Enter, Range errors, alignment etc.) (#1802)

* Make baseline-alignment work for empty, readonly rich-text fields

* Add story for comparing inputs in different states

* Make storybook accessible on local network

* Further conform prose-mirror into the image of input

* Revert making more like input and less like textarea
Wrapping is good

* Add inline clarifying comments

* Add <br> so trailing \n is rendered.

* Slight mobile height standardization

* resolve merge issues

---------

Co-authored-by: Kevin Hahn <kevin_hahn@sil.org>

Author: myieye

frontend/viewer/package.json

@@ -30,12 +30,13 @@
     "test:watch": "vitest",
     "test:storybook": "vitest --project=storybook",
     "test:unit": "vitest --project=unit",
+    "test:browser": "vitest --project=browser",
     "check": "svelte-check",
     "lint": "eslint .",
     "lint:report": "eslint . --output-file eslint_report.json --format json",
     "i18n:extract": "lingui extract --clean && lingui extract",
     "generate-icon-types": "node ./generate-icon-types.js",
-    "storybook": "storybook dev -p 6006",
+    "storybook": "storybook dev -p 6006 --host 0.0.0.0",
     "build-storybook": "storybook build"
   },
   "devDependencies": {

frontend/viewer/src/lib/components/lcm-rich-text-editor/editor-schema.ts

@@ -1,10 +1,14 @@
 import {Schema} from 'prosemirror-model';
 import {gt} from 'svelte-i18n-lingui';
 import {cn} from '$lib/utils';
-
+/*
+  whitespace: pre tells prose-mirror how to parse the dom, not how to render it, that's our job
+  */
 export const textSchema = new Schema({
   nodes: {
-    text: {},
+    text: {
+      whitespace: 'pre',
+    },
     /**
      * Note: it seems that our spans likely should have been modeled as "marks" rather than "nodes".
      * Conceptually our users "mark" up a text.
@@ -13,7 +17,9 @@ export const textSchema = new Schema({
      * */
     span: {
       selectable: false,
-      content: 'text*',
+      content: 'text* br?',
+      // If we remove this Backspace + Delete start removing whole spans
+      inline: true,
       whitespace: 'pre',
       toDOM: (node) => {
         return ['span', {
@@ -28,6 +34,20 @@ export const textSchema = new Schema({
       //this allows us to update the text property without having to map all the span properties into the schema
       attrs: {richSpan: {default: {}}, className: {default: ''}}
     },
-    doc: {content: 'span*', attrs: {}}
+    br: {
+      inline: true,
+      group: 'inline',
+      selectable: false,
+      linebreakReplacement: true,
+      toDOM: () => ['br'],
+      parseDOM: [{tag: 'br'}]
+    },
+    doc: {
+      whitespace: 'pre',
+      // if we remove this Shift + Enter creates new spans and then Backspace starts removing whole spans
+      inline: true,
+      content: 'span*',
+      attrs: {}
+    }
   }
 });

frontend/viewer/src/lib/components/lcm-rich-text-editor/lcm-rich-text-editor.svelte

@@ -13,7 +13,7 @@
   import type {IRichString} from '$lib/dotnet-types/generated-types/MiniLcm/Models/IRichString';
   import {Label} from '$lib/components/ui/label';
   import {EditorView} from 'prosemirror-view';
-  import {AllSelection, EditorState} from 'prosemirror-state';
+  import {AllSelection, EditorState, Selection} from 'prosemirror-state';
   import {keymap} from 'prosemirror-keymap';
   import {baseKeymap} from 'prosemirror-commands';
   import {undo, redo, history} from 'prosemirror-history';
@@ -22,8 +22,10 @@
   import type {HTMLAttributes} from 'svelte/elements';
   import {IsUsingKeyboard} from 'bits-ui';
   import type {IRichSpan} from '$lib/dotnet-types/generated-types/MiniLcm/Models/IRichSpan';
-  import {inputVariants} from '../ui/input/input.svelte';
   import {on} from 'svelte/events';
+  import {IsMobile} from '$lib/hooks/is-mobile.svelte';
+  import {findNextTabbable} from '$lib/utils/tabbable';
+  import {inputVariants} from '../ui/input/input.svelte';
 
   let {
     value = $bindable(),
@@ -36,6 +38,8 @@
     readonly = false,
     onchange = () => {},
     autofocus,
+    class: className,
+    enterkeyhint = 'next',
     ...rest
   }:
     {
@@ -54,6 +58,8 @@
   let editor: EditorView | null = null;
 
   const isUsingKeyboard = new IsUsingKeyboard();
+  // isUsingKeyboard.current isn't entirely reliable on mobile due to virtual keyboards
+  let pointerDown = false;
 
   onMount(() => {
     // docs https://prosemirror.net/docs/
@@ -72,7 +78,7 @@
         editor.updateState(newState);
       },
       attributes: {
-        class: inputVariants({class: 'min-h-10 h-auto block'}),
+        class: inputVariants({class: 'min-h-10 h-auto pb-1.5'}),
         // todo: the distribution of props between the editor and the elementRef is not good
         // there should probably be a wrapper component that provides the elementRef to this one
         ...(id ? {id} : {}),
@@ -81,13 +87,25 @@
         role: 'textbox',
         ...(autocapitalize ? {autocapitalize} : {}),
         spellcheck: 'false',
+        ...(enterkeyhint ? {enterkeyhint} : {}),
       },
       editable() {
         return !readonly;
       },
       handleDOMEvents: {
-        'focus': onfocus,
+        pointerdown() {
+          pointerDown = true;
+          setTimeout(() => pointerDown = false, 100); // yes, apparently we need a decently high timeout value
+        },
+        'focus'(editor) {
+          onfocus(editor, !pointerDown);
+        },
         'blur': onblur,
+        keydown(_view, event) {
+          if (event.key === 'Enter') {
+            onEnterKey();
+          }
+        },
       }
     });
     editor.dom.setAttribute('tabindex', '0');
@@ -97,9 +115,22 @@
     if (relatedLabel) return on(relatedLabel, 'click', onFocusTargetClick);
   });
 
-  function onfocus(editor: EditorView) {
-    if (isUsingKeyboard.current) { // tabbed in
-      selectAll(editor);
+  let prevSelection: Selection | undefined;
+  function onfocus(editor: EditorView, viaKeyboard: boolean) {
+    const usingKeyboard = isUsingKeyboard.current ||
+      IsMobile.value && viaKeyboard;
+    if (usingKeyboard) { // tabbed in
+      if (IsMobile.value) {
+        if (prevSelection) {
+          const prevSelectionForCurrentDoc = Selection.fromJSON(editor.state.doc, prevSelection.toJSON());
+          setSelection(prevSelectionForCurrentDoc);
+          prevSelection = undefined;
+        } else {
+          setSelection(Selection.atEnd(editor.state.doc));
+        }
+      } else {
+        selectAll(editor);
+      }
     }
   }
 
@@ -108,6 +139,7 @@
       onchange(value);
       dirty = false;
     }
+    prevSelection = editor.state.selection;
     clearSelection(editor);
   }
 
@@ -139,6 +171,15 @@
         keymap({
           'Mod-z': undo,
           'Mod-y': redo,
+          // eslint-disable-next-line @typescript-eslint/naming-convention
+          'Enter': () => {
+            // This handler is not triggered reliably on mobile, so we're using handleDOMEvents.keydown instead
+            // if (IsMobile.value) onEnterKey();
+
+            // and we never want to do anything on desktop
+            // (partially because it causes range errors - on mobile too)
+            return true;
+          },
           'Shift-Enter': (state, dispatch) => {
             if (dispatch) dispatch(state.tr.insertText(newLine));
             return true;
@@ -149,27 +190,49 @@
     });
   }
 
+  function onEnterKey(): void {
+    if (IsMobile.value && enterkeyhint === 'next' && editor?.dom) {
+      // mimic <input> 'next' behaviour
+      const nextTabbable = findNextTabbable(editor.dom);
+      nextTabbable?.focus();
+    }
+  }
+
   function valueToDoc(): Node {
     return textSchema.node('doc', {}, value?.spans
       .filter(s => s.text)
-      .map(s => richSpanToNode(s)) ?? []);
+      .map((s, i, all) => richSpanToNode(s, i === all.length - 1)) ?? []);
   }
 
-  function richSpanToNode(s: IRichSpan) {
+  function richSpanToNode(s: IRichSpan, isLast: boolean): Node {
     //we must pull text out of what is stored on the node attrs
     //ProseMirror will keep the text up to date itself, if we store it on the richSpan attr then it will become out of date
     let {text, ...rest} = s;
     //if the ws doesn't match expected, or there's more than just the ws key in props
     const isCustomized = (!!s.ws && !!normalWs && normalWs !== s.ws) || Object.keys(rest).length > 1;
-    return textSchema.node('span', {richSpan: rest, className: cn(isCustomized && 'customized')}, [textSchema.text(replaceLineSeparatorWithNewLine(text))]);
+    return textSchema.node('span', {richSpan: rest, className: cn(isCustomized && 'customized')}, [
+      textSchema.text(replaceLineSeparatorWithNewLine(text)),
+      // a <br> seems to be the only thing that will cause a trailing \n to be rendered
+      // this is what Prose-Mirror does if inline: false, which we can't use
+      ...(isLast ? [textSchema.node('br')] : [])
+    ]);
   }
 
   function richSpanFromNode(node: Node) {
     return {...node.attrs.richSpan, text: replaceNewLineWithLineSeparator(node.textContent)};
   }
 
   function selectAll(editor: EditorView) {
-    editor.dispatch(editor.state.tr.setSelection(new AllSelection(editor.state.doc)));
+    setSelection(new AllSelection(editor.state.doc));
+
+    if (!readonly) return; // happens automatically if not readonly
+
+    const selection = window.getSelection();
+    if (!selection) return;
+    const range = document.createRange();
+    range.selectNodeContents(editor.dom);
+    selection.removeAllRanges();
+    selection.addRange(range);
   }
 
   function clearSelection(editor: EditorView) {
@@ -181,6 +244,19 @@
     }
   }
 
+  function setSelection(selection: Selection): void {
+    if (!editor) return;
+    editor.dispatch(editor.state.tr.setSelection(selection));
+    setTimeout(() => {
+      if (!editor) return;
+      if (editor.state.selection.eq(selection) || selection.$anchor.doc !== editor.state.doc) return;
+      // when tabbing back and forth between 2 rich text editors, the previous setSelection is not sufficient 🤷
+      // (It doesn't have anything to do with the clearSelection() below)
+      // The first call is kept, because it avoid a visible delay when possible
+      editor.dispatch(editor.state.tr.setSelection(selection));
+    }, 1); // 0 is not enough on Firefox
+  }
+
   //lcm expects line separators, but html does not render them, so we replace them with \n
   function replaceNewLineWithLineSeparator(text: string) {
     return text.replaceAll(newLine, lineSeparator);
@@ -190,17 +266,24 @@
   }
 
   function onFocusTargetClick(event: MouseEvent) {
-    if (!editor) return;
-    if (event.target === editor?.dom) return; // the editor will handle focus itself
-    editor.focus();
+    if (!editor || !event.target) return;
+    if (event.target === editor.dom || event.target instanceof globalThis.Node && editor.dom.contains(event.target))
+      return; // the editor will handle focus itself
+    editor?.focus();
+    // Minorly dissatisfying is that when you click on the padding to the right of prose-mirror,
+    // the caret is not intelligently placed at the end of the text (and on the correct line if multi-line)
+    // Everything else seems good
   }
 </script>
-<style lang="postcss">
-  :global(.ProseMirror) {
+
+<style lang="postcss" global>
+  .ProseMirror {
+    display: block;
+    place-content: center;
     flex-grow: 1;
     outline: none;
     cursor: text;
-    /*white-space must be here, if it's directly on span then it will crash with a null node error*/
+
     white-space: pre-wrap;
 
     :global(.customized) {
@@ -210,14 +293,19 @@
     :global(.customized ~ .customized) {
       margin-left: 2px;
     }
+
+    &::before {
+      /* Ensure baseline-alignment even works when empty */
+      content: '\200B';
+    }
   }
 </style>
 
 {#if label}
-  <div {...rest}>
-    <Label onclick={onFocusTargetClick}>{label}</Label>
+  <div class={className} {...rest}>
+    <Label>{label}</Label>
     <div bind:this={elementRef}></div>
   </div>
 {:else}
-  <div bind:this={elementRef} {...rest}></div>
+  <div bind:this={elementRef} class={className} {...rest}></div>
 {/if}

frontend/viewer/src/lib/components/ui/input/input-shell.svelte

@@ -8,7 +8,7 @@
     focusRingClass?: string;
   }
 
-  const anyChildHasFocusRing = 'has-[:focus-visible]:ring-ring has-[:focus-visible]:outline-none has-[:focus-visible]:ring-2 has-[:focus-visible]:ring-offset-2';
+  const anyChildHasFocusRing = 'ring-ring outline-none ring-offset-2 has-[:focus-visible]:ring-2';
 
   let {
     ref = $bindable(null),