Scope blot definitions to Quill instance instead of globally

[artaommahe]

There is some use-cases with SPA when editor is used in several places that requires different formats usage. E.x. at one place i want to disable default images format and enable custom one, and vice versa at another.
Currently all formats registered globally for each quill instance, that forces to use uglyhacks like this

// this blot has same name 'image'
import { ImageBlockBlot } from 'text-module/quill';

// before Quill component-wrapper class
const originalImageBlot = Quill.import('formats/image');

export class TextEditorComponent {

  private initFormats(): void {

    if (_get(this.config, 'formats.blockImages', false)) {

      Quill.register(ImageBlockBlot, true);
    }
    else {

      Quill.register(originalImageBlot, true);
    }
  }
}

This way i forced to change current page quill instance formats set and also i dont know how to disable any format without replacing with dummy one. Also i cannot use two different instances at one page at the same time with different formatting - they will use one global set.

Removing format button from toolbar/elsewhere or registering new formats with other names does not solve this issue cause this formats will work with copy-pasted or added via setContents/dangerouslyPasteHTML content.

[jhchen]

Why doesn't the formats config meet this use case?

[artaommahe]

This woks, partially - i cant define two formats with same name that does not override each other. E.x. i use default image format, then for some pages/editors i want to change images view, that requires image blot markup changes, so it's new blot. But already created content has this

{"insert":{"image":"image-url-here"}}

and i cant define new blot imageNew for this case, cause old images will look like before if i dont override image format with new blot.

[jhchen]

I see. Yes a current limitation is blot names need to be globally unique.

[thinksaydo]

We're running into this same problem with many editors on the page, each with their own plugins. We may try to do a PR on this to add local instance formatters alongside global formatters.

[zizzle6717]

@artaommahe Here is PR we hope will address this issue. Interested to gather some perspective on its usability. slab/parchment#41

Update
On second thought, this needs some more investigation...

[zizzle6717]

@jhchen @artaommahe Here is a working branch of Parchment matched with Quill branched off of version 2.0. I still have 44 tests failing. Could we start a conversation about merging this with branch 2.0? Any insight is much appreciated as I continue to refactor the last few tests.

Parchment - https://github.com/zdizzle6717/parchment/tree/feature/scope-registry-to-quill-instance
Quill - https://github.com/zdizzle6717/quill/tree/scope-registry-to-quill-instance

[zizzle6717]

Down to 23 failing tests, but I'm really struggling to understand how getBounds() and the code format work.

[jhchen]

What is your approach / design this time? What other designs did you consider that you decided against in favor of this?

[zizzle6717]

This time the approach follows the initial recommendation. Rather than having a singleton registry at the modular level, this branch adds a class called EditorRegistry that can be instantiated along with each instance of Quill. Quill's constructor gets a third argument that defaults to a new instance of EditorRegistry.

We gain the ability to mitigate different formats/blots/attributors among each editor. One example of this could be having an editor with a custom video blot and a secondary editor that restricts the use of that same custom video. The user can no longer copy/paste or load content which includes custom formats/blots/attributors unless both editors explicitly use the same EditorRegistry instance.

We first considered cataloguing a different Registry for each editor and setting the active registry on focus. There were too many changes occurring in the background that happened even before focus or that occurred when an editor was inactive. Having the option to share one EditorRegistry among all editors or differentiate a new EditorRegistry per editor allows for more customization without overcomplicating the usability or creating redundant overhead.

[jhchen]

What are the proposed API changes and function signature changes/additions?

[zizzle6717]

With the proposed changes, Parchment and Quill are affected in the following ways.

Parchment
Parchment's interface no longer has a create, find, query, or register method. Instead a non-default export, EditorRegistry, is added to replace these. For example...
import { EditorRegistry } from Parchment;

const editorRegistryA = new EditorRegistry();
const exampleDOMNode = createElement('strong');
...
editorRegistryA.create(exampleDOMNode);
*The function signatures remain unchanged

Quill
Quill's interface has a bit more changes, and these are mostly to ensure that the correct editorRegistry is referenced.

1. Configuration accepts a 3rd argument of editorRegistry. This defaults to a new instance of EditorRegistry
   const editor = new Quill('.editor', {}, new EditorRegistry());
   This will attach to the quill instance (ie. this.quill.editorRegistry)
2. Quill.register and Quill.import are no longer static methods. Instead call this.quill.register and this.quill.import. Calling this.quill.imports will return a full list of the current imports.
3. Quill.find now requires a second argument of the respective editorRegistry
4. Any custom formats/blots/modules will need to be aware of changes to the underlying structure. For example, formats/fonts.js includes a FontStyle class that extends a Parchment Attributor and calls super.value. In this case, value accepts a 2nd argument of editorRegistry that needs to be passed along to super.value.

...currently there are issues with the tests for test/unit/core/selection.js and test/unit/formats/code.js. getBounds() seems to check for some 'magic numbers' and I felt it would be better to gather insight from the developer that wrote the tests...the errors in code.js seem to be caused by some rework I did with the way quill overwrites formats in core and they may relate to the syntax module. The tests were structured to specifically acknowledge this, but I'm uncertain where my mistake was made. Are you able to investigate a bit? I'll have more time next week to revisit these changes and do more testing in a local environment.

[jhchen]

Implementation and trying to pass tests is premature at this stage. To me this is still in the design vetting stage.

1. What do the Quickstart instructions look like now? If there is no registry until new Quill is called, how/when will default formats be registered? What is the experience for the developer that just wants to use Quill with all default settings and get going?
2. No immediate concerns.
3. Why is this necessary?
4. In a test is assert(plus(2, 3) === 5) a use of magic numbers?

[zizzle6717]

1. The Quickstart instructions do not necessarily need to change. Since Parchment is a dependency of Quill, I imported EditorRegistry and set the 3rd argument to a new instance by default. Also, default formats/modules/etc are attached to a static variable on Quill, so using /core.js or /quill.js determines what defaults are available when the quill instance is instantiated.
2. N/A
3. This could be changed similar to the other static functions.
4. No, and maybe I'm just not familiar enough with the code. Native range and selection APIs are very complex, and I don't have enough experience to determine what the height/width of the bounding client rectangle should be. Maybe it is apparent and I need to do some more research.

[jhchen]

1 - I think what will happen with the current proposal is Quill will be initialized with no formats/blots registered new Quill('.editor', {}, new EditorRegistry()); or new Quill('.editor', {}) which is not a usable editor. If I am mistaken please point out when formats/blots are actually registered under this proposal and how it would affect the Quickstart instructions.

3 - This does not answer my question about why it is necessary to change the find function signature. Please take a look at how find works.

[zizzle6717]

My mistake...I had pasted the wrong link to the Quill branch, so I updated the comment above. Here it is again for reference.
https://github.com/zdizzle6717/quill/tree/scope-registry-to-quill-instance

1 - On lines 35 and 38 of core/quill.js, the defaults first get registered in the constructor

3 - I made a mistake here. It looks like the find function is exported separately from the registry, so I will revert that change in both repositories.