[lexical][lexical-table] Feature: Scrollable tables with experimental getDOMSlot API

etrepum commented 1 month ago

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

https://github.com/user-attachments/assets/a94320ca-4e90-487e-b0c1-1d2979ae538e

It also incorporates the img linebreak hack for Safari from https://github.com/facebook/lexical/pull/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

https://github.com/user-attachments/assets/7f1df365-5721-45d2-9d77-53c0bf417c45

Safari end-of-line decorator after imghack

https://github.com/user-attachments/assets/ea63c3a4-0059-445d-8c0f-f4d6d5febc76

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[bot] commented 1 month ago

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[bot] commented 1 month ago

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%)

vercel[bot] commented 1 week ago

Deployment failed with the following error:

The provided GitHub repository does not contain the requested branch or commit reference. Please ensure the repository is not empty.

GermanJablo commented 1 week ago

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 commented 1 week ago

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.

matveeva-as commented 1 week ago

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

mnlkrs commented 1 week ago

Thanks a lot for the work :)

etrepum commented 1 week ago

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

etrepum commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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?

chrome-capture-2024-11-15

etrepum commented 1 week ago

Can you report this as a new issue please?

etrepum commented 1 week ago

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

JeffreyQ commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

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 / lexical