[lexical][lexical-table] Feature: Scrollable tables with experimental getDOMSlot API by etrepum · Pull Request #6759 · facebook/lexical

etrepum · 2024-10-23T23:17:47Z

Description

This refactors the reconciler's treatment of ElementNode to allow you to specify where children should be inserted, which should allow for nodes that have specific requirements like a wrapping DIV element for display or UI reasons. Currently exposed as an @internal API.

As a demonstration it implements the horizontal scroll for tables from #6713. The TablePlugin has a new hasHorizontalScroll property that defaults to false (for backwards compatibility considerations) but the playground has a new setting tableHorizontalScroll that defaults to true and can (only) be disabled with a query string setting.

Screen recording showing scrollable table

scrollable-tables.mov

It also incorporates the img linebreak hack for Safari from #6282 since that part of the reconciler was modified anyway. I don't think it's perfect, I believe there may still be some arrow key navigation quirks in that specific scenario (end of line decorator), but it's a net improvement that can be refined further. I suspect that the full solution would include hiding or removing these img nodes during arrow key navigation.

Safari end-of-line decorator before

safari-imghack-before.mov

Safari end-of-line decorator after imghack

safari-imghack-after.mov

Closes #6713
Closes #6282
Closes #6480
Closes #4487
Closes #6587
Closes #5981

0.20 Upgrade Notes

Scrollable tables are disabled by default in this initial release for compatibility reasons. You can add support for them with <TablePlugin hasHorizontalScroll={true} />.
A new theme property tableScrollableWrapper was added, a warning will be issued if you are using scrollable tables without it. It should include overflow-x: auto;.

New concepts

DOMSlot is an abstraction much like a DOM ParentNode with methods like insertChild, replaceChild, etc. Notably there are three components to a DOMSlot: element, after, and before.

element is the parentNode for any managed lexical children of this ElementNode. The default is the result of createDOM.
after is a DOM node (or null) that all managed children will be after. Default is null.
before is a DOM node (or null) that all managed children will be before (e.g. this.element.insertChild(child, this.before) which is equivalent to this.element.appendNode(child) in the null case). Default is null.

All internal interactions with managing children have been ported to use elementNode.getDOMSlot(dom), so that the ElementNode has an opportunity to set up a non-default DOMSlot.

setDOMUnmanaged / isDOMUnmanaged - These functions are used to mark a DOM node as unmanaged, much like a decorator node, which means that the mutation observer shouldn't worry about it. You would use this for DOM nodes in a DOMSlot that do not have corresponding LexicalNodes.

LexicalNode.reconcileObservedMutation - some of the functionality that was hard-coded into the mutation observer and branched on node type has been moved to this method

Test plan

Unit tests were added for <TablePlugin hasHorizontalScroll={false} /> and <TablePlugin hasHorizontalScroll={true} /> to demonstrate that the tables are wrapped in a div.
The e2e test suite currently sets tableHorizontalScroll set to false in several suites so fewer tests needed to be updated to account for the wrappers, but otherwise it is tested by default. The legacy behavior without a wrapper can be tested with the environment E2E_TABLE_MODE=legacy, but I opted not to add that full suite to our already long e2e tests since a lot of functionality is still tested in legacy mode outside of the Tables.spec.mjs suite.
All existing e2e and unit tests pass

vercel · 2024-10-23T23:17:51Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 11, 2024 4:53pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 11, 2024 4:53pm

github-actions · 2024-10-23T23:19:30Z

size-limit report 📦

Path	Size
lexical - cjs	29.94 KB (0%)
lexical - esm	29.75 KB (0%)
@lexical/rich-text - cjs	38.56 KB (0%)
@lexical/rich-text - esm	31.64 KB (0%)
@lexical/plain-text - cjs	37.18 KB (0%)
@lexical/plain-text - esm	28.95 KB (0%)
@lexical/react - cjs	40.35 KB (0%)
@lexical/react - esm	33.07 KB (0%)

etrepum · 2024-11-11T16:19:41Z

packages/lexical-table/src/LexicalTableNode.ts

  }
 }

+const scrollableEditors = new WeakSet<LexicalEditor>();


I don't love doing this but we also don't have another place to put this sort of state

etrepum · 2024-11-11T16:21:09Z

packages/lexical-playground/__tests__/e2e/Collaboration.spec.mjs

    // Left collaborator types two pieces of text in the same paragraph, but with different styling.
    await focusEditor(page);
    await page.keyboard.type('normal');
+    await assertHTML(


Adding the assertions and the delay/sleeps here seems to sort out the flakiness of this test. It would be nice if we had a better way to wait on specific history events. I assume the arrow key delay fix has something to do with waiting a tick for the selection to get resolved correctly.

etrepum · 2024-11-11T16:21:44Z

packages/lexical-playground/__tests__/e2e/CopyAndPaste/html/TablesHTMLCopyAndPaste.spec.mjs

 test.describe('HTML Tables CopyAndPaste', () => {
-  test.beforeEach(({isCollab, page}) => initialize({isCollab, page}));
+  test.beforeEach(({isCollab, page}) =>
+    initialize({isCollab, page, tableHorizontalScroll: false}),


Use legacy behavior in these tests, no need to test the wrapper (it is not exported or imported)

etrepum · 2024-11-11T16:22:28Z

packages/lexical-playground/__tests__/e2e/Selection.spec.mjs

 test.describe.parallel('Selection', () => {
-  test.beforeEach(({isCollab, page}) => initialize({isCollab, page}));
+  test.beforeEach(({isCollab, page}) =>
+    initialize({isCollab, page, tableHorizontalScroll: false}),


Selection scenarios with the wrapper are tested in the tables suite, no need to refactor here

etrepum · 2024-11-11T16:23:24Z

packages/lexical-playground/__tests__/e2e/Tables.spec.mjs

+  );
+}
+
+const WRAPPER = IS_TABLE_HORIZONTAL_SCROLL ? [0] : [];


We add this to all DOM anchor/focus paths at exactly the right place to account for the wrapper, if present

etrepum · 2024-11-11T16:25:51Z

packages/lexical-playground/__tests__/utils/index.mjs

 const IS_RICH_TEXT = process.env.E2E_EDITOR_MODE !== 'plain-text';
 const IS_PLAIN_TEXT = process.env.E2E_EDITOR_MODE === 'plain-text';
 export const LEGACY_EVENTS = process.env.E2E_EVENTS_MODE === 'legacy-events';
+export const IS_TABLE_HORIZONTAL_SCROLL =


If E2E_TABLE_MODE=legacy then use hasHorizontalScroll={false} otherwise true.

etrepum · 2024-11-11T16:26:44Z

packages/lexical-playground/__tests__/utils/index.mjs

  }
 }
+
+function removeSafariLinebreakImgHack(actualHtml) {


Like tables, we fix up the expectations rather than branching in the individual tests.

etrepum · 2024-11-11T16:29:50Z

packages/lexical-playground/src/themes/PlaygroundEditorTheme.css

+.PlaygroundEditorTheme__tableScrollableWrapper {
+  overflow-x: auto;
+  margin: 0px 25px 30px 0px;
+}
+.PlaygroundEditorTheme__tableScrollableWrapper > .PlaygroundEditorTheme__table {
+  /* Remove the table's margin and put it on the wrapper */
+  margin: 0;
+}


The margin is only moved around in this way because we support both wrapper and wrapper-less with the same CSS. If/when we only support wrapper tables then we would remove the margin from the table directly and apply it only to the wrapper.

etrepum · 2024-11-11T16:33:51Z

packages/lexical/src/LexicalEditor.ts

  tableRow?: EditorThemeClassName;
+  tableScrollableWrapper?: EditorThemeClassName;
  tableSelected?: EditorThemeClassName;
+  tableSelection?: EditorThemeClassName;


I just noticed that this one was missing from the type

etrepum · 2024-11-11T16:35:32Z

packages/lexical/src/LexicalNode.ts

+  reconcileObservedMutation(dom: HTMLElement, editor: LexicalEditor): void {
+    this.markDirty();
+  }


This was moved here because TextNode and DecoratorNode had the same behavior but ElementNode didn't so we just move it to a method for specialization

GermanJablo · 2024-11-11T18:08:54Z

A new theme property tableScrollableWrapper was added, a warning will be issued if you are using scrollable tables without it. It should include overflow-x: auto;.

I would prefer to have the property handle adding overflow-x: auto. I understand your reasoning, it's CSS. But in this case I don't see any advantage in having to repeat the intention in the theme config.

etrepum · 2024-11-11T18:50:17Z

The intention is that it's useful to have a class for other reasons, and it does't really make sense to set both a class and a style. We could unconditionally set the style, I don't have a very strong opinion about it, but there's plenty of other things that don't work if the theme isn't configured appropriately.

ivailop7

30% through, putting some comments will continue tomorrow

ivailop7 · 2024-11-11T23:18:08Z

packages/lexical-playground/src/Settings.tsx

      showTreeView,
      showNestedEditorTreeView,
-      disableBeforeInput,
+      // disableBeforeInput,


maybe delete with the switch code below?

I was mostly commenting things out for space, the UI is not very good with as many options as we have now so I was trying to limit the ones that shouldn't really get used. I don't think there's any reason for a user to toggle this one and it has to refresh the page to work anyway so it might as well be done by URL only

Let make the switches smaller, I don't think the url query arg will be discoverable enough. Anyhow, your call.

I left it out because I couldn't think of a good reason for someone to discover this toggle from the UX, but I'm happy to uncomment either way. I'll wait until the reviews are done before code churn on that nit.

ivailop7 · 2024-11-11T23:18:47Z

packages/lexical-playground/src/Settings.tsx

      showTableOfContents,
      shouldUseLexicalContextMenu,
      shouldPreserveNewLinesInMarkdown,
+      // tableHorizontalScroll,


ivailop7 · 2024-11-11T23:20:52Z

packages/lexical-playground/src/Settings.tsx

            checked={shouldPreserveNewLinesInMarkdown}
            text="Preserve newlines in Markdown"
          />
+          {/* <Switch


I'm happy to have the switch in the playground 👍🏻

I didn't really see a good reason why people would want to turn it on and off, we can do it in the URL with the tests, but I put it there just in case. No strong opinion here, but this UX is bad when there are a lot of options.

packages/lexical-react/src/LexicalTablePlugin.ts

packages/lexical-table/src/LexicalTableNode.ts

ivailop7

LGTM, for @zurfyx to give this a look too.

ivailop7 · 2024-11-12T22:10:11Z

packages/lexical-table/src/LexicalTableSelectionHelpers.ts

      }
    }
+    if (direction === 'down' && $isScrollableTablesActive(editor)) {
+      // Enable Firefox workaround


Firefox-specific bug? Do we know more about this one?

Yes I think I wrote some more about it elsewhere in this PR but in Firefox natively moving the down arrow will skip over the table when wrapped with a div so the last cell get focused instead of the first. Couldn't find a CSS arrangement that allows scrolling with keyboard down arrow navigation works in Firefox at the same time. We really should have better selection APIs because there isn't really a way to fix it before reconciliation in "user" code

Now that I'm back at a computer here's a link to where the problem is described https://github.com/facebook/lexical/pull/6759/files#diff-9f98cefa6e4564feb2a48d20b6e4278bcf999351abeceb27122732af6cd82413R273-R281

ivailop7 · 2024-11-12T22:11:13Z

packages/lexical-table/src/LexicalTableSelectionHelpers.ts

-    return undefined;
-  }
-
  // TODO: Add support for nested tables


I think you can remove the TODO comment IMO, I don't think we'll support nested tables any time soon or if we should in a first place.

People write bugs about nested tables at least once a month. I don't think they're that far off of working, someone just has to care enough to write those tests and PR.

ivailop7 · 2024-11-12T22:21:54Z

packages/lexical/src/LexicalUtils.ts

+  (dom as Node & Record<typeof prop, NodeKey | undefined>)[prop] = key;
+}
+
+export function getNodeKeyFromDOMNode(


excited about those utility functions!

matveeva-as · 2024-11-13T09:52:29Z

Hi! Thanks a lot for this!! When are you planing to release a new version of Lexical with this changes?

mnlkrs · 2024-11-13T09:54:10Z

Thanks a lot for the work :)

etrepum · 2024-11-13T15:19:54Z

Releases typically happen monthly, but if you need to try it sooner you can always use a nightly release

zurfyx

Late review but that's awesome work! This decouples the strict Node to DOM Node map which has been an inconvenience in some cases.

Terminology nits

While I like the flexibility of the DOMSlotAPI, withAfter still seems confusing to me.

Ideally, we would've had something like the DOM where insertBefore/insertAfter would work very closely to the DOM API.
withAfter works the opposite way as I would anticipate .withAfter(tableElement.querySelector('colgroup')). To me this an element that inserts another element before.
slot.element is fairly ambiguous when a wrapper is present. Why does createDOM have to return the wrapper whereas domSlot has control over the child?

Testing

Did we test DOMSlot with inlines? I believe that LexicalElementNode.test and the E2E tests cover the block case and the particularly domSlot API well but I think it's important to have a test that covers inlines with wrappers if it's not there already.

Similarly, validate how selection behaves when before and after elements are presents. The selection logic seems to cover resolution for discovery, but is this sufficient for adjusting anchor/focus points accordingly on all other events?

etrepum · 2024-11-14T15:30:32Z

DOM doesn't have an insertAfter API, only insertBefore(dom, before). The after and before properties (set by withAfter and withBefore) are the markers that would be used in those methods (if a native insertAfter existed). I don't think there is a non-confusing way to name those markers using the same terminology that DOM does, because the before sentinel is after the children and vice versa for after. If we name the properties relative to the children it will be opposite to how the DOM semantics are (we would be doing insertBefore(dom, after) in that case, which is more confusing IMO).

etrepum · 2024-11-14T15:36:26Z

That said, I'm not against changing the naming around, just that it's going to be confusing however you do it because there are two opposite and obvious points of reference (the markers relative to the children - what you were thinking, and the insertion of children relative to the markers - how DOM and this API work). The whole point of the slot's properties is to reference a specific element in the tree and provide those markers for insertBefore (and the simulated insertAfter).

etrepum · 2024-11-14T15:43:16Z

createDOM has to return the DOM that's inserted into the document, and if it didn't, we'd have to break compatibility by changing the LexicalNode interface. It was done this way so that no user code or types would have to change.

Naming of element is arbitrary, could be elementThatChildrenAreInsertedInto or something like that, I guess. If you have a good name then please suggest it. I think most names are going to be confusing in one way or another.

etrepum · 2024-11-14T15:50:16Z

Inlines will be tested when we have a use case to add them. I wasn't going to add another week or two of effort to come up with and test a use case that we don't have yet for an internal experimental API. I am sure there are going to be selection quirks because of how we use platform native arrow key navigation but don't have a good way to fix them up post-event in user code yet. I am pretty certain that the lexical selection will be mapped to the closest lexical point to where the DOM selection is, but visually the caret might not end up where the user wants (especially if they don't provide the correct css and contentEditable settings for their accessory elements, if present)

matveeva-as · 2024-11-15T11:11:20Z

Hi! I've found a bug but don't sure how it can be related to this PR, but 0.19.0 works good, 0.20.1-nightly.20241114.0 doesn't.

When we have two tables in the Editor and press arrowDown in the second table, focus goes to the first cell in the first table.
Do you have any idea why does it happen?

etrepum · 2024-11-15T15:02:46Z

Can you report this as a new issue please?

etrepum · 2024-11-15T17:59:53Z

Nevermind, should be fixed in #6839 and released with the next nightly

JeffreyQ · 2024-11-17T18:55:58Z

Hi, I had some questions around $getTableAndElementByKey.

I see that you implement it here in packages/lexical-table/src/LexicalTableObserver.ts.

However, you are importing it here from @lexical/table which is of version 0.20.0.

However, when I'm looking at LexicalTableUtils.d.ts file in version 0.20.0 on npm I do not see a $getTableAndElementByKey.

etrepum · 2024-11-17T20:16:02Z

The entire lexical monorepo is versioned together. The playground is not using @lexical/table v0.20 from npm, it is built with the exact version that is in the same commit as the playground. The versions don't update in git until a new release is created.

JeffreyQ · 2024-11-17T20:39:51Z

The entire lexical monorepo is versioned together. The playground is not using @lexical/table v0.20 from npm, it is built with the exact version that is in the same commit as the playground. The versions don't update in git until a new release is created.

Got it, thank you for clarifying!

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 23, 2024

vercel bot deployed to Preview – lexical October 23, 2024 23:18 View deployment

vercel bot deployed to Preview – lexical-playground October 23, 2024 23:19 View deployment

etrepum force-pushed the reconciler-children branch from 14fda1c to 9d9fc14 Compare October 24, 2024 04:49

vercel bot deployed to Preview – lexical October 24, 2024 04:50 View deployment

vercel bot deployed to Preview – lexical-playground October 24, 2024 04:50 View deployment

etrepum force-pushed the reconciler-children branch from 9d9fc14 to 6ce96d0 Compare October 24, 2024 05:22

vercel bot deployed to Preview – lexical-playground October 24, 2024 05:23 View deployment

vercel bot deployed to Preview – lexical October 24, 2024 05:23 View deployment

etrepum force-pushed the reconciler-children branch from 6ce96d0 to 480aaeb Compare October 24, 2024 05:26

vercel bot deployed to Preview – lexical October 24, 2024 05:27 View deployment

vercel bot deployed to Preview – lexical-playground October 24, 2024 05:27 View deployment

etrepum force-pushed the reconciler-children branch from 480aaeb to 6899a28 Compare October 29, 2024 16:23

vercel bot deployed to Preview – lexical-playground October 29, 2024 16:24 View deployment

vercel bot deployed to Preview – lexical October 29, 2024 16:26 View deployment

etrepum force-pushed the reconciler-children branch from 6899a28 to b6f65bb Compare October 29, 2024 21:57

vercel bot deployed to Preview – lexical October 29, 2024 21:58 View deployment

vercel bot deployed to Preview – lexical-playground October 29, 2024 21:58 View deployment

vercel bot deployed to Preview – lexical-playground November 5, 2024 05:31 View deployment

vercel bot deployed to Preview – lexical November 5, 2024 05:31 View deployment

etrepum added 2 commits November 5, 2024 11:42

WIP getDOMSlot

4639f77

Refactor mutation observer

f9b537a

etrepum force-pushed the reconciler-children branch from dc53dfb to f9b537a Compare November 5, 2024 19:42

vercel bot deployed to Preview – lexical November 5, 2024 19:43 View deployment

vercel bot deployed to Preview – lexical-playground November 5, 2024 19:43 View deployment

vercel bot deployed to Preview – lexical November 5, 2024 20:30 View deployment

vercel bot deployed to Preview – lexical-playground November 5, 2024 20:30 View deployment

etrepum force-pushed the reconciler-children branch from 629d934 to 5bd93be Compare November 5, 2024 20:33

etrepum mentioned this pull request Nov 11, 2024

[lexical-table] Bug Fix: get table-cell background selection color from a class #6658

Merged

etrepum added 2 commits November 11, 2024 08:13

Update @lexical/react/TablePlugin documentation

50dadbc

flow types

9bc9c33

etrepum commented Nov 11, 2024

View reviewed changes

Merge branch 'main' into reconciler-children

6453a4b

ivailop7 reviewed Nov 11, 2024

View reviewed changes

ivailop7 approved these changes Nov 12, 2024

View reviewed changes

etrepum mentioned this pull request Nov 13, 2024

[lexical-rich-text][lexical-plain-text] workaround for Korean IME issue on iOS #6819

Merged

mnlkrs mentioned this pull request Nov 13, 2024

[BUG] Safari: Wrong cursor placement mdx-editor/editor#624

Closed

2 tasks

zurfyx reviewed Nov 14, 2024

View reviewed changes

etrepum mentioned this pull request Nov 15, 2024

[lexical-table] Bug Fix: Fix down arrow key handling in TableObserver #6839

Merged

etrepum mentioned this pull request Dec 2, 2024

Proposal: DecoratorElementNode #5930

Open

GermanJablo mentioned this pull request Dec 9, 2024

[lexical][@lexical/selection] Feature: add a generic classes property to all nodes for easier customization #6929

Closed

etrepum mentioned this pull request Jan 12, 2025

[lexical] Bug Fix: Handle MutationObserver/input event re-ordering when using contentEditable inside of an iframe #7045

Merged

etrepum mentioned this pull request Mar 14, 2025

[lexical-playground] Bug Fix: Table action menu visibility with cell overflow #7334

Merged

Conversation

etrepum commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

0.20 Upgrade Notes

New concepts

Test plan

Uh oh!

vercel bot commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GermanJablo commented Nov 11, 2024

Uh oh!

etrepum commented Nov 11, 2024

Uh oh!

ivailop7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivailop7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matveeva-as commented Nov 13, 2024

Uh oh!

mnlkrs commented Nov 13, 2024

Uh oh!

etrepum commented Nov 13, 2024

Uh oh!

etrepum commented Oct 23, 2024 •

edited

Loading

vercel bot commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited

Loading

matveeva-as commented Nov 15, 2024 •

edited

Loading