search

SimplyEdit / simplyedit

The first CMS designed for the decentralized web
https://simplyedit.io/
MIT License
25 stars 5 forks source link

Mutation events have been removed from Google Chrome #20

Open evanebb opened 1 month ago

evanebb commented 1 month ago

Recently (July 23rd 2024), Chrome 127 has been released which removes mutation events: https://developer.chrome.com/blog/mutation-events-deprecation

This codebase uses these in multiple places: https://github.com/search?q=repo%3ASimplyEdit%2Fsimplyedit+EventListener%28%22DOM&type=code

Some examples:

These events are not fired anymore, so functionality that relies on them breaks in Google Chrome.

The blog post linked at the top suggests either migrating to MutationObserver or using a polyfill.

poef commented 1 month ago

Hi, these event listeners are there for backwards compatibility with older browsers. If you look closely you will find calls to MutationObservers as well. See for example: https://github.com/SimplyEdit/simplyedit/blob/9db41be64bb3ec2c1ae6c74f557ae1b5fbe38ca6/simply/databind.js#L633

However, I do think that if support for MutationObserver is found, the event listeners shouldn't be added anymore, that will get rid of the warnings in the console.

I've double checked with Chrome 127.0.6533.99 (ubuntu) and have no problems there. So if you do have problems, which are limited to Chrome, please tell us what the visible issues are.

MarcHagen commented 1 month ago

@poef (and @ylebre as he knows our codebase a bit) I've been able to setup a CodePen = https://codepen.io/marchagen/full/PorKVzx

Our problem is that when removing a list item, it does not update the editor.pageData.... anymore. Which it did on Firefox, Edge and Chrome.

It seems to be from specific versions: Chrome 127.0.6533.89 works, >= 127.0.6533.100 does not work Edge 127.0.2651.74 works, >= 127.0.2651.86 does not work

Digging bit deeper in our code, we use the listItem.parentNode.removeChild(listItem); and seems that is not working with events.

Edit: Same concept as the TodoMVP example (but the domain is broken so cannot check quickly) https://github.com/SimplyEdit/TodoMVC/blob/1d4854c59b5bfa5069fecf72a837f30cdcc192f5/index.html#L65-L67

poef commented 1 month ago

Thank you for the test case, I can confirm the problem. So it seems that in some cases the MutationObserver is not doing its job properly and SimplyEdit still relies on the deprecated events. I will dig into that to find out where we're missing handling the mutations.

Potherca commented 1 month ago
Note to future self, to not have to look things up again later - `DOMAttrModified` [2 Found](https://github.com/search?q=repo%3ASimplyEdit%2Fsimplyedit+DOMAttrModified&type=code) - `DOMAttributeNameChanged` 0 Found - `DOMCharacterDataModified` [4 Found](https://github.com/search?q=repo%3ASimplyEdit%2Fsimplyedit+DOMCharacterDataModified&type=code) - `DOMElementNameChanged` 0 Found - `DOMNodeInserted` [6 Found](https://github.com/search?q=repo%3ASimplyEdit%2Fsimplyedit+DOMNodeInserted&type=code) - `DOMNodeInsertedIntoDocument` 0 Found - `DOMNodeRemoved` [4 Found](https://github.com/search?q=repo%3ASimplyEdit%2Fsimplyedit+DOMNodeRemoved&type=code) - `DOMNodeRemovedFromDocument` 0 Found - `DOMSubtreeModified` [2 Found](https://github.com/search?q=repo%3ASimplyEdit%2Fsimplyedit+DOMSubtreeModified&type=code) Please note, these are _occurrences_, not each occurrence is an actual _problem_ (as some of the found entries are supposed to be there with regard to backwards compatibility for older browser versions).
poef commented 1 month ago

I'm looking into it, and the DOMNodeRemoved listener on line 798 in databind.js is a clear suspect. I don't see an alternative using MutationObserver there. This also fits the test case. So I'm going to try to redesign this using MutationObserver.

Potherca commented 3 weeks ago

Intermediate update: Created locally reproducible scenario.

We have some breaking tests - databinding: databinding push 2 items, reorder items in dom (`tests/data-display/tests.js:1013:8`) - databinding: databinding in 2nd degree list, reorder 2-depth items (`tests/data-display/tests.js:1070:8`) - databinding: databinding in 2nd degree list, reorder 1-depth items (`tests/data-display/tests.js:1103:8`) - databinding: databinding in 2nd degree list, reorder 1-depth with children (`tests/data-display/tests.js:1133:8`) - lists: add list item, databinding (`tests/data-edit/tests.js:1627:8`) - lists: add list item (list item is field), databinding (`tests/data-edit/tests.js:1640:8`) - lists: add 2 list items, databinding (`tests/data-edit/tests.js:1653:8`)

@poef is working on fixes for offending parts of the code, might also need to add some more tests.

poef commented 3 weeks ago

I've made a new branch to test out my fix, you can use it to test your code by switching to this URL for simply-edit.js:

https://cdn.jsdelivr.net/gh/SimplyEdit/simplyedit@mutationobserver-switch/js/simply-edit.js

Please let me know if it works for you, not just in the codepen example.

evanebb commented 2 weeks ago

Hi! Thanks for taking a look at this.

In the CodePen @MarcHagen sent earlier, everything seems to work just fine. However, when I applied the fix to our actual application, it didn't work as intended. Specifically, in combination with the nested templates that we use, adding/removing items from a list causes the entire list to be emptied.

This can be observed in the following CodePen: https://codepen.io/evannovoserve/pen/vYqrXJr. If you try to remove any of the items from the list or add a new one, all of them will disappear. This was observed on both Firefox and Google Chrome.

Additionally, I was wondering what the proper, supported version is and where we should get it from? I can see that your branch is 49 commits ahead of master, with only the latest commit being related to this issue. It seems to be based on the gitlab branch, which contains a bunch of fixes for other stuff, but this has not made it to master or a release yet. Additionally, the latest release was made a while ago, and commits to master have been made since. Will this fix be incorporated in master/a new release/some other stable place that we can get it from?