[lexical-table] Bug Fix: Enable observer updates on table elements attributes change

[EugeneVorobyev]

Description

Now table observer doesn't listen to table DOM element attibutes updates. So when attempting to extend table node with features that require table element attibute updates (for example adding/remoing classes to table element) it won't re-render table.

This change will enable observer to listen to attibutes changes as well as to changes in children.

Closes #6410

Details on the topic

Here's documentation about mutation observer: https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver/observe

attributes is set to false by default and setting it to true will watch for changes to the value of attributes on the node or nodes being monitored.

In case we can't enable all attibutes, it should be possible to upgrade table creation payload to include attribute names filter to enable monitoring only specified attributes for those who needs it by passing names to attributeFilter

Test plan

No new tests seem to be needed. All unit tests and e2e tests pass. No changes to existing table functionality.

[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	Jul 31, 2024 2:09pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 31, 2024 2:09pm

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	29.05 KB (0%)
lexical - esm	28.86 KB (0%)
@lexical/rich-text - cjs	37.39 KB (0%)
@lexical/rich-text - esm	28.33 KB (0%)
@lexical/plain-text - cjs	36.02 KB (0%)
@lexical/plain-text - esm	25.6 KB (0%)
@lexical/react - cjs	39.29 KB (0%)
@lexical/react - esm	29.77 KB (0%)

[etrepum]

Wouldn't you want a new test that shows that this functionality works, and to prevent a regression in the future?

[EugeneVorobyev]

Wouldn't you want a new test that shows that this functionality works, and to prevent a regression in the future?

I wouldn't mind to add more tests. Though I am not quite sure how to approach it here. This is internal table observer configuration and it doesn't affect any existing table cases.

The only way to test it would be creating a table node override and run e2e test against that new node. The whole setup would require a reasonable example of such node to be contributed into playground and new tests added. Fairly large effort and I don't feel it worth it and I frankly can see any example of such testing in the repository (I mean testing extra beviour in overriden nodes).

Probably if this change go through and I'm able to implement a custom table node with the logic based on attibute changes on my side I would be able to contribute back as example for playground.

[etrepum]

It looks like MutationObserver should be supported by jsdom so you may be able to write a unit test for it without changing the playground at all

[EugeneVorobyev]

Yes, jsdom supports, but it doesn't seem to be helpful in this case.

Here're few things:

• There is no existing unit tests for TableObserver at the moment, at least I can't find them in repo. Perhaps it should be addressed as a separate issue.
• MutationObserver is a standart Web API and testing it (different options) is mostly respontibility of browsers or engines that implemets it. Pretty sure each browser have tests for that.
• [ from my experience ] Jsdom historically had issues with event loop implementation. Not saying it has now, but I would not recommend testing anything required promises or observables using jsdom.

Again, I am totally for writing unit and e2e tests, but in this particular case it's just not reasonable.

P.S. If you think it's required to have tests for changes in TableObserver, probably new issue must be created and this one marked as blocked until there is a minimal required coverage. wdyt?

[etrepum]

LGTM, tests can be added separately the next time TableObserver is updated