[lexical-table][lexical-playground] Feature: Add column widths to TableNode

[patrick-atticus]

Description

This PR adds an optional colWidths property to TableNode, which renders a <colgroup> element within tables.

Defining the column widths has 2 main advantages:

1. We can simplify table resizing logic, particularly around merged cell cases.
2. It enables better control of tables with the style table-layout: fixed

If using table-layout: fixed, you must set a table width. Lexical playground uses max-content which masks some issues. If you want to truncate text within the table, the best way is to change the table width to fit-content (and add css to truncate).

However, without a <col> element, a table with layout fixed will use the first row to determine column widths for the rest of the table. If there are merged cells in the first row, this makes it impossible to correctly size (and resize) columns in the remaining rows.

Adding colWidths to TableNode is the cleanest way to enable the ability to truncate text in tables and simplify resizing.

Changes

• Add optional colWidth to TableNode (an array of pixel width numbers)
• Update createDOM to render <colgroup> and <col> elements with widths, if colWidths is set.
• Update playground TableCellResizerPlugin to get and set colWidths for better resizing

Closes #6623 #6624 (resize bugs)

Now it is also possible to truncate text in cells. This was not possible before and is the main motivation for this PR

Test plan

Since this changes the output of TableNode, it breaks many tests that expect specific HTML. I will update tests if maintainers are open to this approach.

Demo

🔉 narrated
https://github.com/user-attachments/assets/aa7902f2-df6f-4975-b83c-2a28e7f086c5

[vercel]

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	Sep 18, 2024 0:16am
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 18, 2024 0:16am

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	29.78 KB (0%)
lexical - esm	29.62 KB (0%)
@lexical/rich-text - cjs	38.21 KB (0%)
@lexical/rich-text - esm	31.51 KB (0%)
@lexical/plain-text - cjs	36.87 KB (0%)
@lexical/plain-text - esm	28.82 KB (0%)
@lexical/react - cjs	40.01 KB (0%)
@lexical/react - esm	32.91 KB (0%)

[etrepum]

Overall this seems like a great approach. There are going to be some issues with sharing of __colWidths because node properties can not be shared across versions (unless they are effectively immutable). Removing the rest of the width support from TableCellNode would also make sense, and of course fixing the tests is going to be the big thing.

[patrick-atticus]

@etrepum thanks for the review!

There are going to be some issues with sharing of __colWidths because node properties can not be shared across versions (unless they are effectively immutable).

Could you please expand on this? I'm not sure what this means

I'll take a look at the tests

[etrepum]

So the way that Lexical works is that the first time you get a writable version of a node in an update cycle it will clone the node. The previous version of the node should be considered completely frozen and immutable. However, in this implementation you have a mutable array property that is returned and mutated in-place with slice, so any changes to that array would also affect all previous versions of that node. It breaks undo/redo and any history or rollback related feature in general because version B is using the identical array from version A and it gets mutated in-place.

[patrick-atticus]

Thanks, I assume there's no other nodes that do this at the moment? Would it be enough to type the property as readonly, and Object.freeze the value in the setter? (see recent commit)

This worked well to flag an error in another place I had mutated it.

[etrepum]

Using the readonly type is probably sufficient, we don't do a lot of runtime enforcement of these kinds of constraints that the type checker can enforce especially in production. Copying it everywhere even when not necessary is something to be avoided (e.g. copying in the setter, getter and clone would be most robust but do unnecessary work pretty often).

It's not done a lot in this codebase, I can't think of many properties that are a mutable type to begin with. It's normally primitive value types that don't have any sort of sharing issues.

[etrepum]

I suspect that there's more code & tests that should be written to handle when the column count of a table changes (both insert and remove) but this is looking great so far

[etrepum]

It looks like the tests are passing, when you think it's ready for review (this PR is still in draft) and then we'll review it again and merge when ready

[patrick-atticus]

There's a lot of noise from fixing the tests, but I've tried to keep the commits clean.

I added one unit test that covers setting widths and inserting a column. The e2e tests have some resizing merged cells which verifies the colwidth output. Let me know if there's any other cases you'd like covered.

As far as I can tell the remaining failures are due to flakes.

[etrepum]

I think the remaining failures are not just flaky, I gave the failed tests a couple additional runs. Some of it is HTML differences because of the added colgroup

[1]   2 failed
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Tables.spec.mjs:2277:3 › Tables › Select multiple merged cells (selection expands to a rectangle) 
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/Tables.spec.mjs:3378:3 › Tables › Can align text using Table selection 

[patrick-atticus]

Ah I see now, thank you. Fixed.

As a side note, I wasn't able to run the collab e2e tests locally. With the server running npm run start and the collab tests eg npm run test-e2e-collab-chromium the playground connection button state always starts as disconnected. All the tests fail like this:

      -   unexpected value "Connect Collaborative Editing"


       at packages/lexical-playground/__tests__/utils/index.mjs:119

      117 |         await expect(
      118 |           frameLocator.locator('.action-button.connect'),
    > 119 |         ).toHaveAttribute('title', /Disconnect/);

[etrepum]

Ah that's interesting, it looks like there's a dev mode collab bug probably due to react strict mode. I think because the effects run twice the provider is disconnecting the websocket, I wonder when that started. The ci versions appear to work (although those also trigger a build which can be a bit slower to start).

[etrepum]

Thank you for all your work on this!

[patrick-atticus]

Thanks! Keen to make use of this