Skip to content

Conversation

@alexgarbarev
Copy link

Hello @matthew-carroll and thanks for the wonderful library (the code is really good)!

I've tried to implement an editable table on my own, but I found very quickly that it's impossible to do without modifying a core components of the library.
So, after few trials I found this draft PR #2765 and it looks like a proper way to go.
Indeed, we need a CompositeNode that would allow us overcome "single column layout" limitation.

So I decided to continue your PR. This is still work-in-progress, but what I did there is:

  • IME serialization for composite nodes
  • adjusted ColumnDocumentComponent so it works with real children node ids.
  • fixed CompositeNodeViewModel, so it propagates selection/selectionColor to it's children view models.
  • insert characters into child (leaf) nodes by typing

Demo:

Screen.Recording.2025-11-12.at.04.11.13.mov

Notes about implementation:

  • In order to migrate to CompositeNode easily and avoid rewriting existing logic, we can keep all these is TextNode checks, we just need to get leaf nodes by DocumentPosition instead of root nodes by nodeId where it matters.
  • Some Events, like TextInsertionEvent needs to be migrated to use DocumentPosition instead of int offset (see NodeTextInsertionEvent suggested implementation). The plan is to continue and migrate other events to be position-based.

The goal of that PR is to avoid conflicts going forward and get some feedback (Like is there any chance to contribute?). If it makes sense, I can continue with that task in that branch. Thank you!

@alexgarbarev alexgarbarev changed the title Aleksey/2746 hack tables into existing doc structure WIP 2746 hack tables into existing doc structure Nov 12, 2025
@alexgarbarev
Copy link
Author

Update:

  • Merged with the main branch (maybe not the best idea, as it’s now harder to review; I just wanted to stay in sync with the latest changes)
  • Default text-editing behavior is now working for leaf nodes:
    • Deletions implemented (Backspace, Delete, and selection-based deletion)
    • Text selection (word or paragraph) via double/triple-click
    • Replace selected text by typing
    • Copy/paste
    • Undo/redo

While working on the above, I found that some built-in logic isn’t obviously applicable to CompositeNode. This mostly concerns multi-node management.

Examples:

  • When multiple nodes are selected and the user presses Delete, all are removed. But when multiple nodes within a CompositeNode are selected — it’s unclear whether they should be deletable.
  • Pressing Delete or Backspace sometimes deletes an entire node, but it’s uncertain whether this should happen inside a CompositeNode.
  • When pasting content with Cmd+V and the cursor is inside a CompositeNode, the default implementation may attempt to insert multiple nodes — but CompositeNode should decide whether it can accept additional children. For now, I’m inserting the pasted text into a leaf TextNode.

I see three possible approaches to handle this:

  1. Forbid multi-node logic in default implementations. Users can implement own keyboardActions or reactions to intercept Delete, Backspace, Enter, or Cmd+V and define custom behavior in their CompositeNode subclasses.
    (This is how the current PR works.)

    • Pros:
      • Minimal implementation effort
    • Cons:
      • Can be verbose and lead to code duplication
  2. Refactor built-in logic so it calls CompositeNode/DocumentNode to decide whether to split paragraphs, insert, or delete nodes.

    • Pros:
      • Most flexible; users control default behavior
    • Cons:
      • Harder to implement; requires rewriting existing logic (risky)
      • Needs careful API design
  3. Introduce a new class: ColumnCompositeNode extends CompositeNode. Adjust built-in logic to preserve current behavior but treat ColumnCompositeNode specially — nodes within ColumnCompositeNode should behave as root-level nodes. A column can contain multiple vertically stacked nodes, and users can freely insert/delete within it.

    • Pros:
      • Moderate implementation effort
      • ColumnCompositeNode is more intuitive than generic CompositeNode when implementing actions
      • User can mix CompositeNode (with first approach) and ColumnCompositeNode(with third approach) to implement desired behavior. For example the Table can be CompositeNode, but each cell can be ColumnCompositeNode, if user wants more than one Node inside cell
    • Cons:
      • Adds a new core class that the entire system must recognize

I think the third approach is probably a way to go, then during the implementation I can try move logic into ColumnCompositeNode class (so user can override if needed).

@matthew-carroll what do you think about this?

@alexgarbarev alexgarbarev changed the base branch from 2746_hack-tables-into-existing-doc-structure to main November 13, 2025 20:30
…chy. Updated all NodeDocumentChange to use `NodePath` instead of `nodeId`.

Start working on upstream/downstream deletion inside CompositeNode
@alexgarbarev alexgarbarev marked this pull request as draft November 15, 2025 00:41
@alexgarbarev
Copy link
Author

Update

Keep working on this PR. Marked it Draft, as it's not yet ready.

Changes:

  • A few bug fixes related to CompositeNode. Now multiple nested nodes works fine.
  • introduced new NodePath model that should be used in most cases instead of nodeId, when working with leaf nodes (including root nodes without children). Reworked all NodeDocumentChange event classes, to use NodePath.
  • keep refactoring my initial implementation: I found that in most cases using NodePath is much cleaner and straightforward than DocumentPosition.
  • Linkify and hr reactions updated, so they works fine inside CompositeNode.

As for my previous question:

While working on the above, I found that some built-in logic isn’t obviously applicable to CompositeNode. This mostly concerns multi-node management.

I decided to go with second approach, and just defined 2 methods in CompositeNode class:

  bool canInsertChildAfter(String nodeId) {
    return true;
  }

  bool canDeleteChild(String nodeId) {
    final nodeToDelete = getChildByNodeId(nodeId);
    if (nodeToDelete == null || !nodeToDelete.isDeletable) {
      return false;
    }
    // If CompositeNode is not deletable, at least one child must be inside
    if (!isDeletable && children.length == 1) {
      return false;
    }
    return true;
  }

that way CompositeNode can decide on default behavior.
My goal is to have same default behavior inside ColumnDocumentComponent as with root nodes: new paragraph on enter, reactions to create lists, delete anything by selection, etc.

@alexgarbarev alexgarbarev force-pushed the aleksey/2746_hack-tables-into-existing-doc-structure branch from 6de4ace to 4bbd126 Compare November 15, 2025 01:59
@alexgarbarev alexgarbarev force-pushed the aleksey/2746_hack-tables-into-existing-doc-structure branch from 4bbd126 to a84cfbd Compare November 15, 2025 02:00
@matthew-carroll
Copy link
Contributor

Thanks for working on this. I'm not sure when I'll have a chance to dig into your changes, and the review might be quite involved when we get there, but just letting you know that I'm aware of this PR.

Also, FYI, there may have been local changes that I had not yet pushed to my PR for this. I haven't checked. So be prepared for significant conflicts if that ends up being the case.

@alexgarbarev
Copy link
Author

alexgarbarev commented Nov 15, 2025

@matthew-carroll thank for feedback.

Just an update and my plan going forward:

I was keep working on this until now, and I think I'm about to finish most of it today/tomorrow. Most APIs are done (like NodePath-based requests/commands. Also APIs to iterate through leaf nodes:

  @override
  Iterable<(NodePath, DocumentNode)> getLeafNodes({bool? reversed, NodePath? since}) sync* {
    final iterator = _LeafNodeIterator(this, sincePath: since, reversed: reversed);
    while (iterator.moveNext()) {
      yield iterator.current;
    }
  }

Now I'm just keep reworking existing code to be compatible with NodePath (i.e. inside CompositeNode). So far I tested with demo app in macos and most typing events are handled: inserting paragraph, inserting hr, deleting upstream/downstream. Right now I'm working on selection deletion across multiple nodes of different parents.
Once this is done - I think I'll wrap it up, do some minor cleanups and move this PR from draft.

Regarding unpublished changes, what kind of changes are you referring to? Maybe they are not related to my changes. I'd like to coordinate on plans to avoid wasted effort from any side. At least I'd like to agree on core api.

With the changes I'm about to release, I think we would be able to implement proper table and similar components.

The only future improvements I have in mind are:

  • Improve presenter code, if one leaf viewModel updated, only it's widget rebuilt (not whole root parent). Also, changes tracking can be improved to be child-level (with NodePath instead of nodeId).
  • IME serialization can be improved, so only selected leaf child is converted to text (not whole root node as it is today).
  • Not all reactions, keyboard actions, requests are rewritten to use NodePath (and hence support being used inside CompositeNode). They will work with flat documents as today, but once we add CompositeNode to document they would either not work or crash, based on implementation inside.

But I'm not sure if I should work on these points too, as it is time consuming and more changes to review/merge. I think we can do that with subsequent PRs, as public API won't be changed there, just internal improvements.

Would you like to discuss any of these topics?

@matthew-carroll
Copy link
Contributor

Regarding unpublished changes, what kind of changes are you referring to?

For my PR, I was writing bits of implementation and then pushing them. It's possible that I've got a bunch of commits locally that I never pushed. I haven't touched it in a while, so I don't know if I do, and if I do, I don't know which changes those are.

But I'm not sure if I should work on these points too, as it is time consuming and more changes to review/merge

Given the volume of changes you've already made, you might as well continue. The existing work will already take a major effort to review.

Please make sure to write thorough tests along the way. You can use our many thousands of existing tests to demonstrate the variety of what should be tested, and how to test it. A meaningful review can't take place until we have corresponding tests, so better to do that sooner than later. Also, tests tend to reveal missing features.

Lastly, with regard to follow up PRs, we pretty much need to tackle everything in one PR. Anything we put off until later might show a fundamental flaw in the approach. Therefore, we need to update every EditRequest, EditCommand, reaction, document layout, etc. to support the new approaches in this PR. Only then can we be sure that these foundational changes can co-exist with existing requirements.

@alexgarbarev
Copy link
Author

alexgarbarev commented Nov 17, 2025

After some thinking, it looks like I can revert lots of changes I did and go back to (mostly) original API, where we always address nodes by id. I'll just create a mapping Map<String, NodePath> _nodePathPerId. That way we can always understand where specific nodeId sits in tree.

Also, to make all EditRequest, EditCommand work as it is, without any changes - we should get rid of CompositeNodePosition, instead we should always have leaf nodePosition + leaf node id. That will make migration really easy. I'll try and see how it works.

@alexgarbarev
Copy link
Author

Update:

New approach works really well. I reverted a lot of changes I did before.

What's the main idea, and key points:

  1. In the Document now we have a mapping between nodeId and NodePath - so we can understand where specified nodeId belongs to. That allows us keeping existing API mostly untouched. And it's not just hack, I think existing API is good, when we have nodeId + nodePosition, keeping in mind that each node should have unique id (which is right thing to have).
  2. In the Layout (and similar places), once I receive CompositeNodePosition, I post-process it, unwrapping into leaf position. Same for DocumentSelection.
  3. Just implement componentForNodeId, nodeById and other methods in Document/MutableDocument.

In result, after doing these 3 points, everything seems to work fine without any changes to any EditRequest or EditCommand.

Next:

  • clean up, implement all methods in Document (including ones that are not used by demo/tests)
  • cover by tests (I didn't cover before, because the API I was working on was changed multiple times during r&d).
  • maybe implement more demo components (row, table, column) and mix of them, to prove that the concept is working fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants