[lexical][lexical-table] Feature: Scrollable tables with experimental getDOMSlot API #6759
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
size-limit report 📦
|
etrepum
force-pushed
the
reconciler-children
branch
from
14fda1c to
9d9fc14
Compare
etrepum
force-pushed
the
reconciler-children
branch
from
9d9fc14 to
6ce96d0
Compare
etrepum
force-pushed
the
reconciler-children
branch
from
6ce96d0 to
480aaeb
Compare
etrepum
force-pushed
the
reconciler-children
branch
from
480aaeb to
6899a28
Compare
etrepum
force-pushed
the
reconciler-children
branch
from
6899a28 to
b6f65bb
Compare
etrepum
force-pushed
the
reconciler-children
branch
from
dc53dfb to
f9b537a
Compare
etrepum
force-pushed
the
reconciler-children
branch
from
629d934 to
5bd93be
Compare
I would prefer to have the property handle adding |
|
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. |
| @@ -28,10 +28,11 @@ export default function Settings(): JSX.Element { | |||
| isAutocomplete, | |||
| showTreeView, | |||
| showNestedEditorTreeView, | |||
| disableBeforeInput, | |||
| // disableBeforeInput, | |||
There was a problem hiding this comment.
maybe delete with the switch code below?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Let make the switches smaller, I don't think the url query arg will be discoverable enough. Anyhow, your call.
There was a problem hiding this comment.
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.
| showTableOfContents, | ||
| shouldUseLexicalContextMenu, | ||
| shouldPreserveNewLinesInMarkdown, | ||
| // tableHorizontalScroll, |
| @@ -167,6 +168,13 @@ export default function Settings(): JSX.Element { | |||
| checked={shouldPreserveNewLinesInMarkdown} | |||
| text="Preserve newlines in Markdown" | |||
| /> | |||
| {/* <Switch | |||
There was a problem hiding this comment.
I'm happy to have the switch in the playground 👍🏻
There was a problem hiding this comment.
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.
| @@ -1538,6 +1587,10 @@ function $handleArrowKey( | |||
| } | |||
| } | |||
| } | |||
| if (direction === 'down' && $isScrollableTablesActive(editor)) { | |||
| // Enable Firefox workaround | |||
There was a problem hiding this comment.
Firefox-specific bug? Do we know more about this one?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| if (!tableNodeParentDOM) { | ||
| return undefined; | ||
| } | ||
|
|
||
| // TODO: Add support for nested tables |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| (dom as Node & Record<typeof prop, NodeKey | undefined>)[prop] = key; | ||
| } | ||
|
|
||
| export function getNodeKeyFromDOMNode( |
There was a problem hiding this comment.
excited about those utility functions!
|
Hi! Thanks a lot for this!! When are you planing to release a new version of Lexical with this changes? |
|
Thanks a lot for the work :) |
|
Releases typically happen monthly, but if you need to try it sooner you can always use a nightly release |
There was a problem hiding this comment.
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/insertAfterwould work very closely to the DOM API. withAfterworks the opposite way as I would anticipate.withAfter(tableElement.querySelector('colgroup')). To me this an element that inserts another element before.slot.elementis fairly ambiguous when a wrapper is present. Why doescreateDOMhave to return the wrapper whereasdomSlothas 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?
|
DOM doesn't have an |
|
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). |
|
Naming of element is arbitrary, could be |
|
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) |
|
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.
|
|
Can you report this as a new issue please? |
|
Nevermind, should be fixed in #6839 and released with the next nightly |
|
Hi, I had some questions around I see that you implement it here in packages/lexical-table/src/LexicalTableObserver.ts. However, you are importing it here from However, when I'm looking at |
|
The entire lexical monorepo is versioned together. The playground is not using |
Got it, thank you for clarifying! |
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
@internalAPI.As a demonstration it implements the horizontal scroll for tables from #6713. The
TablePluginhas a newhasHorizontalScrollproperty that defaults tofalse(for backwards compatibility considerations) but the playground has a new settingtableHorizontalScrollthat 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
<TablePlugin hasHorizontalScroll={true} />.tableScrollableWrapperwas added, a warning will be issued if you are using scrollable tables without it. It should includeoverflow-x: auto;.New concepts
DOMSlotis an abstraction much like a DOM ParentNode with methods likeinsertChild,replaceChild, etc. Notably there are three components to a DOMSlot:element,after, andbefore.elementis the parentNode for any managed lexical children of this ElementNode. The default is the result ofcreateDOM.afteris a DOM node (or null) that all managed children will be after. Default is null.beforeis a DOM node (or null) that all managed children will be before (e.g.this.element.insertChild(child, this.before)which is equivalent tothis.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 methodTest plan
<TablePlugin hasHorizontalScroll={false} />and<TablePlugin hasHorizontalScroll={true} />to demonstrate that the tables are wrapped in a div.tableHorizontalScrollset 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 environmentE2E_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.