Skip to content

WIP 2746 hack tables into existing doc structure#2830

Open
alexgarbarev wants to merge 36 commits intoFlutter-Bounty-Hunters:mainfrom
alexgarbarev:aleksey/2746_hack-tables-into-existing-doc-structure
Open

WIP 2746 hack tables into existing doc structure#2830
alexgarbarev wants to merge 36 commits intoFlutter-Bounty-Hunters:mainfrom
alexgarbarev:aleksey/2746_hack-tables-into-existing-doc-structure

Conversation

@alexgarbarev
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

…SelectionWithCompositeNodeAdjustments" methods for correct cell-based selection snapping and square-base selection
…ownstream, selection). New isIsolating and replacementWhenEmpty features implemented for CompositeNodes
Nodes traversal by arrow keys implemented for composite nodes
@alexgarbarev
Copy link
Copy Markdown
Author

Ok, it’s been tough 2 weeks.
I think I’ve achieved what I wanted and I’d like to encourage you to see how it works in main_table_composite_component and main_components_in_components demos.

The idea behind main_table_composite_component was to create proof of concept implementation of the table, to make sure that with the new APIs we are fully covered and all features can be done.
I haven't started actual Table component, as it's a whole new story (especially keeping in mind, that I'd like a Table to support cells merging both vertically and horizontally, so TableRenderObject from flutter doesn't support that, and creating a new render object - that's a whole new project).
The implementation inside main_table_composite_component is quick and dirty.

As I mentioned before, I reverted many changes since 17 of November due to new approach. The new approach implies that we are working with leaf nodes everywhere, and can reference any node with nodeId. Due to that - all existing EditRequest, EditCommand and reactions just works as they were before. I modified some of them later, just to implement additional features that relies on CompositeNode logic.

Key highlights of the demo table implementation:

  • Selection within table can be square-like, so if we select cell [0, 0] and [1, 1], it would select cells [[0, 0], [0, 1], [1, 0], [1, 1]], without cells from third column. This is done via new optional method: CompositeNode.getSelectedChildrenBetween, which works as additional filter in getNodesInsde method. If method returns null, then natural flow of children array would be used.
  • Cell-level selection:
    • CompositeNodeViewModel can receive CompositeNodeSelection and display something special on parent level (I did a selection frame around cell + fill cell with background color, when more than one cell selected)
    • CompositeNodeViewModel can disable selection for it's children - there is shouldApplySelectionToChildren method in view model.
    • CompositeNode can adjust selection after mouse gestures by CompositeNode.adjustUpstreamPosition and CompositeNode.adjustDownstreamPosition method. This is useful when user expand selection from middle of first cell to the middle of second cell, then to achieve cell-level selection, we should correct positions from the beginning of first cell to end of second cell.
      This works via new method refineSelectionWithCompositeNodeAdjustments, that's called from MouseInteractor.
    • Finally, there is an additional method displayCaretWithExpandedSelection in the CompositeComponent, so we can hide cursor, when we draw cell-level selection with multiple cells.
  • Content Deletion: As we are always working with leaf-nodes, existing content deletion Request/Command were mostly working fine. Just few edge cases were handled:
    • CompositeNode cannot be empty, so once everything is deleted, we should either delete the Node itself, or insert the placeholder as first child.
      There is a new method in CompositeNode - resolveWhenChildrenAffected. It's called every time child is deleted or emptied in the CompositeNode, so CompositeNode can decide what to do - it can provide a replacement for itself, be deleted, or kept untouched.
      • In current Banner demo, CompositeNodes just deletes itself once content is empty
      • In Table demo, Cells inserts Empty Paragraph once become empty. And Table can be deleted when all Cells are empties.
  • Content isolation. In cases like table cell, we would like to change default behavior of selection and content flow, so you should not go to previous cell when you press backspace at the beginning of the cell. Or merge text of two cells with delete/backspace keys, etc. There is new getter isIsolating in the CompositeNode that does exactly that. In the Table demo, cells are isolating, while in the Banner demo - just default flow.
  • Arrow-Navigation. To provide precise arrows navigation, there 2 new methods in CompositeNode:
    • getNextChildInDirection - returns a next child after given child in specified direction (up, down, left, right).
    • getFirstChildInDirection - returns a first child in given direction, also it takes nearX optional double arg, so Table can utilize it and return correct column, when user press up/down from the center of paragraph.
  • Shift + Arrow for cell-level selection. I've implemented it with custom keyboardAction inside demo, just to prove the concept:
    • When inside table, we do square-like cell-rounded selection
    • When we go outside table with shift+arrow, or when we go inside table from the outside - we round selection by rows.
  • Copy content with cmd+c. I just updated extractTextFromSelection method, so instead of leaf iteration, it goes by root nodes, then CompositeNode can override copyContent or copyChildrenContent method and provide the text. In the Table demo, the table provides it's content in CSV format as an example.
  • Ah, also I've updated DocumentLayout implementation, so it stores GlobalKeys of the children nodes and reuses them between builds, so States are not lost.

What is missing - is unit tests for new infrastructure methods. I couldn't use TDD here, because APIs and features were keep changing while I tried to achieve desired behavior, and that behavior wasn't isolated, so I tested manually.
The plan is to cover every new method with unit tests to avoid regression and check edge cases.
As for goldens, I'm not sure yet, if I should create some demo CompositeNodes inside tests. It looks to me, that isolated unit tests should be enough, as I'm not introducing anything visible. Coverage by goldens should go with new composite components, like Table or Banner going forward.
Btw all existing tests are passing, because most if my changes are ignored when CompositeNodes are not in use.

I plan to take some break on that now (I have to do some client work ;) ).

@matthew-carroll, please check the demo and new API methods in the infra. If we are ok with that - I can continue with the tests once I come back.

FYI @angelosilvestre tagging you here, as I see you did the Markdown Table and might be interested in the demo too.

@alexgarbarev alexgarbarev marked this pull request as ready for review November 28, 2025 13:07
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