Singleton for styles processor was a bad idea

[Reinmar]

📝 Provide a description of the improvement

So, when making a decision that we can go with a styles processor singleton I was only thinking how one editor can affect another. That was a bit fishy, but acceptable.

However, then, thank god, I realized that test files will affect one another and that's just too ugly. There's no reasonable way to clean this up other than just stop using singleton.

Why didn't we use a singleton in the first place? Because we need an access to a view document from all elements. Through that document we ca access a styles processor of that particular view document.

Interestingly, I've just checked that view nodes have the #document property but it's going to work only for elements that are in the document already. This is not going to work for document fragments and detached nodes unfortunately.

I can see two options here:

• Make the current #document return the document based on a writer which created it. Both writers (upcast and downcast) will have to have the #document property too. The #document could only be changed when a node is inserted to an element/docfrag from a different #document.
  • This way of tracking the document may actually be faster than the current recursive root lookup.
  • It's also quite clear in terms of when the parent document may change.
  • We could also throw in case of documents mismatch.
• Introduce a new property which will work like the #document that I proposed in the former point.

cc @scofalik @jodator

Steps

• Add real #document property to view.Node and view.DocumentFragment. This property will be passed to those nodes/dfs through the first param of their constructors. This requires passing document to UpcastWriter (DowncastWriter already knows about the document).
  • Do not forget about a specific case when a node is create in document A and then it's added to document B. At this moment, the document property needs to be updated. However, it would be useful to add a debug console log (CK_DEBUG_ENGINE?) so we can find out about this weird scenario when one node is being moved from one document to another.
• Change implementation of the style processor rules registration so it's bound with a specific document instance. Use those rules in a view.Element#_styles map.

---

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

[Reinmar]

We've got 👍 from @scofalik for the first option, so let's do this :)

[jodator]

Yes, singletons are always bad :D The other idea was to not allow extend the style processing rules. This is not feasible as we want to have this style processing rules added only when needed.

Taken this into consideration I don't see any other "clean" way of doing this. The style processing rules should be added per document context so the #document option seems fine for me.

[pomek]

After adding the #document property to view.Node and view.DocumentFragment as the first argument of their constructors, I spotted some problems:

• A lot of tests started failing because view.Document is now the first parameter in a constructor for the above classes. After spending some time on adjusting API, those tests are green again.
• Getter view.Node#document should be removed but there is at least one piece of the code where we base on the #document value (needsPlaceholder() function). The question is: how to check whether an element was removed? As for know, I decided to check whether the passed element is an instance of view.DocumentFragment. After debugging this test case I noticed that the element is a child of DF after being removed from the document. However, I am not sure about the solution I did.
• view.Document is a heavy object and we should avoid creating it. It means that DomConverter.domToView() function and DomConverter.domChildrenToView() generator require an instance of view.Document as the first parameter.
• Since the above, XmlDataProcessort.toView() and HtmlDataProcessor.toView() must create an own instance of the document which will be passed to DomConverter.
• Also, the view.Renderer class needs the view.Document instance but it can be extracted from the view.View class where rendered is created. I decided to simplify the Renderer's constructor. Instead of passing two/three parameters, it accepts the view.View instance and we extract required classes from it.
• At the beginning, we thought that adding nodes from one document to nodes in another document shouldn't be possible. It's not true because we must support it because of things like XmlDataProcessor or HtmlDataProcessor which have no access to the target document.

I am still trying to understand one test that fails:

FAILED TESTS:
  downcast selection converters
    clean-up
      clearAttributes
        ✖ should do nothing if the attribute element had been already removed
          Chrome 80.0.3987 (Mac OS X 10.14.6)
        TypeError: Cannot read property '_removeChildren' of null
            at AttributeElement._remove (webpack:///./packages/ckeditor5-engine/src/view/node.js?:278:15)
            at DowncastWriter.mergeAttributes (webpack:///./packages/ckeditor5-engine/src/view/downcastwriter.js?:553:19)
            at DowncastDispatcher.eval (webpack:///./packages/ckeditor5-engine/src/conversion/downcasthelpers.js?:567:27)
            at DowncastDispatcher.fire (webpack:///./packages/ckeditor5-utils/src/emittermixin.js?:218:30)
            at DowncastDispatcher.convertSelection (webpack:///./packages/ckeditor5-engine/src/conversion/downcastdispatcher.js?:279:8)
            at eval (webpack:///./packages/ckeditor5-engine/tests/conversion/downcasthelpers.js?:2245:17)
            at View.change (webpack:///./packages/ckeditor5-engine/src/view/view.js?:491:27)
            at Context.eval (webpack:///./packages/ckeditor5-engine/tests/conversion/downcasthelpers.js?:2236:10)

After fixing that, I'll be ready for closing the first part of the issue.

[pomek]

Ok, I found it. https://github.com/ckeditor/ckeditor5-engine/blob/7ab19cff179b375e3bd1fb7e5413529423a473c6/src/conversion/downcasthelpers.js#L546 will be always true.
I replaced the line with the snippet below:

if ( range.end.parent.parent ) {

and all the tests are green.

[pomek]

Change implementation of the style processor rules registration so it's bound with a specific document instance. Use those rules in a view.Element#_styles map.

During this point in the issue, we decided to create a single instance of the StyleProcessor in the core Editor class.

Why? The result of the downcast and the upcast conversions must be the same. It's also safer and easier to add processing rules in a single place instead of creating a few instances of SP.

It means, we need to pass the instance of SP to:

• view.Document (through the view.View and EditingController classes),
• (Html|Xml)DataProcessor through constructors (because #toView() creates an instance of view.Document),
• DataController - the same reason as above.

All I need to do now is fix all failing tests because (again) we changed APIs of many classes. But I guess, we achieved the goal.

cc: @jodator

[jodator]

Yes. Looking at where it is passed we need to have a single instance injected as a dependency on all of those classes so the behavior is consistent in all of the pipelines.

Another thing is that to allow extensibility of this system we have editor.eding.view.document.addStyleProcessorRules() which after these changes will be set in the wrong place. However setting this on editor.addStyleProcessorRules() looks also not so nice.

The other way around this I see is to have editor.stylesProcessor.addRules() or something similar. (BREAKING CHANGE 😞. Buuuut as we didn't release the editor yet we can:

• make current addStyleProcessorRules() private/protected or
• move it as proposed to editor.stylesProcesor.addRules() [name might be better - I'm concerned about its location).