[lexical-table] feat: Add row striping

[ivailop7]

Adding support for row striping, increases tables visual accessiblity.

row_striping.mov

[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	Aug 29, 2024 8:18pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 29, 2024 8:18pm

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	29.38 KB (0%)
lexical - esm	29.22 KB (0%)
@lexical/rich-text - cjs	37.87 KB (0%)
@lexical/rich-text - esm	31.08 KB (0%)
@lexical/plain-text - cjs	36.45 KB (0%)
@lexical/plain-text - esm	28.44 KB (0%)
@lexical/react - cjs	39.64 KB (0%)
@lexical/react - esm	32.52 KB (0%)

[ivailop7]

@etrepum could you help me on this one, there something happening on a deeper level that I'm unable to pin down. If you run my branch and create a table -> enable row striping -> disable row striping (or add col header, etc.) it would crash, because it can no longer find a table observer. Not urgent, when you have some free time.

[etrepum]

@ivailop7 I've added a commit that I think fixes the issue you're having, I did not do the rebase/conflict resolution

[ivailop7]

@ivailop7 I've added a commit that I think fixes the issue you're having, I did not do the rebase/conflict resolution

It does work, thanks Bob. A very interesting commit. I'll handle the remaining details.

[etrepum]

I think the root cause was the incorrect type and management of the property that's added to the element and it's just a cast so there was no compiler checking. I added the abort signal because I also noticed that the listener cleanup there wasn't comprehensive when auditing the code. Probably would be better off moving most of that code out of effects for cross-platform and just simplicity reasons.

[ivailop7]

I think the root cause was the incorrect type and management of the property that's added to the element and it's just a cast so there was no compiler checking. I added the abort signal because I also noticed that the listener cleanup there wasn't comprehensive when auditing the code. Probably would be better off moving most of that code out of effects for cross-platform and just simplicity reasons.

When you say property, do you mean the TableNode rowStriping one?

Unfortunately, the issue seems to be still there. As in table selection gets broken after toggling row striping, no highlight shows up when doing a table selection or cells resizing works, until you do a table selection and then resizing cells works, so the event of enabling rowStriping is still the breaking point.

I do suspect it has to do with introducting of TableNode 'updateDOM' not being false all the time now and the observer is missing that rerender and things get out of sync.

[etrepum]

I meant the property attached to HTMLElement to store the TableObserver. Can you type up repro steps for what's happening now?

[ivailop7]

I meant the property attached to HTMLElement to store the TableObserver. Can you type up repro steps for what's happening now?

1. Create a table
2. Enable row striping
3. Try to resize the table (cursor doesn't change to two-way arrow) -> Can't
4. Try to do a table selection -> No highlight displayed, but selection is done. (The observer issue)
5. Now try to resize the table -> OK

[ivailop7]

I meant the property attached to HTMLElement to store the TableObserver. Can you type up repro steps for what's happening now?

1. Create a table
2. Enable row striping
3. Try to resize the table (cursor doesn't change to two-way arrow) -> Can't
4. Try to do a table selection -> No highlight displayed, but selection is done. (The observer issue)
5. Now try to resize the table -> OK

@etrepum let me know, if you still are struggling replicating. Will do a recording.

[etrepum]

I've just been AFK all day, asked for repro instructions to save time for when I am back at a computer

[ivailop7]

I've just been AFK all day, asked for repro instructions to save time for when I am back at a computer

Sure, no rush. Appreciate the help here.

[etrepum]

It looks like the problem is the updateDOM returning true, which creates a new HTMLElement and does not initialize a new TableObserver. Mutation listeners are kind of a bad abstraction for managing DOM because "update" could mean that an in-place DOM update occurred OR that the DOM was replaced (e.g. logically destroy then create would make more sense for this case, at least as far as DOM tracking goes). The simplest fix is to make the DOM update row striping in-place.

[etrepum]

I've added a commit that does both fixes, the in-place update is more efficient and would've required less changes by itself. I thought it also made sense to handle updateDOM true case to demonstrate how to do it correctly and in case it's subclassed with an updateDOM that can return true.

[ivailop7]

I've added a commit that does both fixes, the in-place update is more efficient and would've required less changes by itself. I thought it also made sense to handle updateDOM true case to demonstrate how to do it correctly and in case it's subclassed with an updateDOM that can return true.

Totally missed the mutation listener was only doing the 'create' event. Thank you for fixing this in-place as well. 🚀

[ivailop7]

This is ready for review, can I get a review. Big shout out to Bob for helping on the Table Observer issues. @etrepum @zurfyx @potatowagon

[etrepum]

LGTM! but of course I had a hand in fixing some of the edge cases so it's probably worth another set of eyes