slab / parchment

Generalized state model for rich-text editors to interface with browser DOM
BSD 3-Clause "New" or "Revised" License
628 stars 143 forks source link

Scroll#find() should not return blots in child scrolls #107

Closed luin closed 2 years ago

luin commented 2 years ago

Previously, Scroll#find() can return blots inside child scrolls. This is actually not expected as child scrolls should be managed by embed blots that render them.

scottsidwell commented 2 years ago

Hey @luin 👋 Consider this a drive-by, but I've been looking into nesting Quill-in-Quill recently as well- and ran into this same issue and fixed it on our fork by instead filtering out all mutations that originated in another scroll, e.g.

    // Handle the case where mutations have originated from inside another Quill, i.e. nested Quill instances.
    // Special handling will just ignore any mutations that originate inside other ScrollBlots.
    mutations = mutations.filter(mutation => {
      const blot = this.registry.find(mutation.target, true);
      return blot && blot.scroll === this;
    });

Is there a reason you opted to preserve passing around mutations that originated from other Scrolls (e.g. to update), and to just ensure they (correctly) don't resolve to a blot?

These seem like they may be equivalent approaches, but curious if I missed anything around the mechanics of parchment that means I should have preserved the mutations here

luin commented 2 years ago

Hey @scottsidwell 👋

Quill actually handles this similarly. I haven't given it much thought, but it sounds like it makes sense to handle this at the Parchment level.

scottsidwell commented 2 years ago

It does too! I must have missed that; glad we arrived at a similar result 😄 Looks like I've got some rebasing to do on our fork! Thanks for the fast reply :)