[lexical][lexical-react][lexical-table] Fix: Add horizontal scroll for tables #6713
Conversation
GermanJablo
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz,
ivailop7,
Sahejkm and
potatowagon
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
size-limit report 📦
|
There was a problem hiding this comment.
I don't have time for QA right now or to audit the tests but I took a quick look at the code. Seems like a useful improvement! The downside is that it is a bit backwards incompatible due to the additional node and requirements for table use. Would maybe be a bit nicer if it was done in a way that could be backwards compatible. @ivailop7 should have a look
|
Will give this a read this evening! |
|
@ivailop7 an assessment on this one sooner than later would be good, since it changes the DOM of all tables it will generate conflicts pretty quickly |
The problem with the breaking change is in the introduction of a new node, as there can be logic that ignores its existence. Not with its max-width of 100%. Considering that the new node is going to be there for everyone, I can't think of any reason why anyone would want to give it a different behavior. |
|
We discussed this PR in the weekly meeting today and it seems very promising and overall the right direction given the current constraints in the reconciler. However, the backwards compatibility issue makes it very tricky to accept as-is because it would likely cause some problems with existing deployments (particularly those using yjs, rolling out with multiple concurrent versions of lexical, etc.). It would be ideal if there was a way to preserve the possibility to deploy table nodes as-is without the ScrollableNode wrapper and then gracefully upgrade somehow to include the wrapper. We think it will be possible to make the ScrollableNode opt-in by moving the transforms from static methods to separately exported transform functions that need to be registered explicitly with the editor (this could be abstracted with a register function that does a mergeRegister of all necessary transforms and a React plugin to make it easier to install). This would also allow future usage of ScrollableNode with other types of nodes as the logic would be in a function that can be replaced (e.g. by parameterizing the It's acceptable that the tests will expect that this plugin is already registered so that work wouldn't have to be undone, but we'd probably want to add some minimal tests to show that some table functionality still works without the opt-in ScrollableNode. Overall, if we can get this into an opt-in state to make the upgrade path smooth then we are onboard with merging this quickly! Maybe something along these lines (with export interface ScrollableNodeConfig {
scrollableChildNodes: readonly Klass<ElementNode>[];
$isScrollableChild?: (node: LexicalNode | null) => boolean;
}
export function registerScrollableNodeTransform(
editor: LexicalEditor,
config: ScrollableNodeConfig,
): () => void {
invariant(
editor.hasNodes([ScrollableNode]),
'TablePlugin: ScrollableNode not registered on editor',
);
const {
scrollableChildNodes,
$isScrollableChild = (node) =>
node !== null &&
scrollableChildNodes.some((klass) => node instanceof klass),
} = config;
return mergeRegister(
editor.registerNodeTransform(ScrollableNode, (node) => {
let onlyScrollableChild: LexicalNode | null = null;
for (const child of node.getChildren()) {
if (onlyScrollableChild === null && $isScrollableChild(child)) {
onlyScrollableChild = child;
} else {
child.remove();
}
}
if (onlyScrollableChild === null) {
node.remove();
const root = $getRoot();
if (root.isEmpty()) {
root.append($createParagraphNode());
}
return;
}
const selection = $getSelection();
if (!selection) {
return;
}
const nodeKey = node.getKey();
for (const point of selection.getStartEndPoints() || []) {
if (point.key === nodeKey) {
$normalizePoint__EXPERIMENTAL(point);
}
}
}),
...scrollableChildNodes.map((klass) =>
editor.registerNodeTransform(klass, (node) => {
const parent = node.getParent();
if (!$isScrollableNode(parent)) {
const scrollable = $createScrollableNode();
node.insertBefore(scrollable);
scrollable.append(node);
}
}),
),
);
} |
|
Thanks for the review @etrepum. Everything you say makes sense. The biggest difficulty will be not in moving the transforms to a plugin, but in maintaining the 2 versions of |
|
Maybe the best way to do that would be to instead override TableNode.selectNext, TableNode.selectPrevious, TableNode.insertBefore, etc. to check the parent and delegate if necessary? I guess the insertBefore/insertAfter stuff could maybe be fixed in a more robust way by having the ScrollableNode transform move non-scrollable elements before or after itself instead of removing them |
|
Hmm, you would also need to extend Also, that wouldn't solve every scenario. For example if a paragraph does |
|
I don't think that this code does any better at solving all of those scenarios, and I don't think that you would want the |
|
I cherry picked your commit, and modified The next step would be to retrieve the initial tests for cases where it is not registered. But I'm not sure what the best way to do that is. I've made a PoC to make its registration configurable in the bottom left options menu in the playground. The commit is in another branch: c788896 I see that with richtext/plaintext this + environment variables are used. But I don't know if I'm making it more complicated than necessary. In this case, it is only necessary to run a subset of tests with both configurations. |
There was a problem hiding this comment.
This looks like a good approach to me. I'm not personally that concerned about having another toggle in the playground settings for separate e2e testing of scrollable and non-scrollable tables. I think a small suite of unit tests that build an editor without the scrollable plugin would be fine to demonstrate that things still work without it.
| return $getEditor().hasNode(ScrollableNode) | ||
| ? (tableNode.getParentOrThrow() as ScrollableNode) | ||
| : tableNode; | ||
| } |
There was a problem hiding this comment.
It might be cleaner to put an $isScrollableNode check on the parent instead of using the editor config, that way you could toggle the behavior using only the plugin.
| child.remove(); | ||
| } | ||
| } | ||
| if (onlyScrollableChild === null) { | ||
| node.remove(); |
There was a problem hiding this comment.
based on the other discussions I think that these children should be moved out of the scrollable node either before or after the scrollable node depending on whether there's a child or not
Co-authored-by: Bob Ippolito <[email protected]>
Co-authored-by: Bob Ippolito <[email protected]>
|
Sorry for the delay on this. I'm getting back to this PR and will try to finish it ASAP. |
|
I did mock up an implementation based on this in 351dc2f (#6759) using a new proposed experimental getDOMSlot API which would allow the TableNode to control where its children are rendered. I didn't write any tests but it seemed to behave in the playground when I was testing it locally. Requires a setting to be enabled https://lexical-playground-irlbe8n5m-fbopensource.vercel.app/?tableHorizontalScroll=true |
|
That looks super cool! I'm going to pause this PR until we see if |
Description
Closes #6480
Having tables overflow is a mistake, as their content can conflict with other elements of the interface:
Another incorrect behavior is the one that occurs in the Lexical Playground, where if the table is too large, the entire editor becomes larger. The horizontal scroll bar is not placed over the table, but over the entire content.
A super quick way to fix overflow is to add a max-width of 100% to the table.
The problem is, as this user noted, that this creates conflicts with the column width property.
This is what OneNote does and I've always hated it. Sometimes columns resize in a magical or hard-to-understand way. It's better to give the user the flexibility to adjust column widths to their liking and to have overflow if needed.
The correct solution to this is for tables to have an overflow scroll independent of the rest of the editor, similar to what Notion does:
The problem is that the
<table>tag doesn't support overflow: scroll. A div is needed to wrap it.This PR introduces a new
Scrollablenode that automatically wraps tables.BREAKING CHANGE:
TablePluginnow requires the editor to also register theScrollableNodenode. All tables are automatically wrapped in aScrollableNode.Test plan
Before
After