T/404: Better handling of   block filler.

[jodator]

Suggested merge commit message (convention)

Fix: Remove only real block fillers on DOM to view conversion. Closes ckeditor/ckeditor5#3704.

---

Additional information

ATM this PR fixes all the issues addressed with handling   in the data. I'm not however 100% satisfied with this solution because of a need to handle <br data-cke-filler="true"> in editing differently than   in data pipeline.

For editing pipeline the isBlockFiller() is pretty straight forward as we check node.isEqualNode() wchich compares two DOM nodes by theirs attributes. However for   we compare Text elements by string equality. In   case we need to detect if the block filler belongs to data or is it negligible in given context.

The other solution that came to my mind is to allow passing a isBlockFiller() check to DOMConverter along with blockFiller option thus explicitly changing the DOMConverter for editing and data pipelines. The drawback is that you have to remember to pass this method when not using deafult block filler (<br>).

• After some testing (including PFO compatibility issues) I think that the current solution is OK.
• I've also fixed some tests that were incorrectly implemented (didn't test what they claimed in the description)

• I've hardcoded a short list of inline elements that are recognized in <b>&npbsp</b> and <b>foo</b>&nbsp;<i>bar</i> cases but this is to be extended before merging.

ps.: Some tests in PFO are failing - I'll be investigating this.

I've added a sub-pr for PFO: ckeditor/ckeditor5-paste-from-office#81 (branch t/ckeditor5-engine/404.

Requires: ckeditor/ckeditor5-utils#302.

[coveralls]

Coverage remained the same at 100.0% when pulling fa8ccb6 on t/404 into f073ad5 on master.

[jodator]

So for PFO some tests fail because:

<p>
<span><img src="..."></span><span style='mso-spacerun:yes'>&nbsp;</span>foo 
</p>

is expected to be:

<image src="foo.jpg"></image>
<paragraph>foo</paragraph>

which is IMO wrong as this should have space inserted before "foo" and with previous logic, the   was cut off as it was considered a block filler.

[Reinmar]

I left a comment in #404. I think that this algorithm must work differently.

[jodator]

@Reinmar OK the code is fixed and we should be able to:

editor.setData( '<p><strong>a</strong>&nbsp;<i>f</i></p>' );

The PR in PFO was also updated to reflect latest findings about tests and the PFO internals. In one test I had to add space between blocks (<table> and <paragraph>) because this is what DOM->View conversion will produce and since we allow spaces between blocks in clipboard holder then the model fixture must have this set as it represents model data produced by the PFO.

[Reinmar]

As commented.

[jodator]

@Reinmar I don't like that this duplicates the code from a DomConverter - maybe we should intorduce isBlockElement() in ckeditor5-utisl/dom? That this could be unified there.

[Reinmar]

We should use the same block elements array which is defined in DomConverter. The idea is that it can be extended by someone else if needed. So, we need to find a way to make filler.js us that property.

[Reinmar]

One option would be to have a module here (in the engine) which exports a table.

This module would be imported by both domconverter.js and filler.js. The instance of this array would be shared by those modules. So, if you'd extend DomConverter.blockElementNames that'd also extend the value used by the filler.js module.

The downside of this would be that this would become a singleton. All editor instances would share this table. In fact, that'd mean that if you'd like to extend this array in your plugin's code (cause it adds some new element support to the editor), if you'd run 10 editors on one page, this element would be added 10 times. That's quite ugly.

So, I'd rather go in a different direction – e.g. move isBlockFiller() to DomConverter. Perhaps it doesn't make sense to have this as contextless util. Maybe it needs to be a part of bigger context.

[jodator]

@Reinmar I've rewritten the checks as we talked.

Requires:

• changes in PFO: T/ckeditor5 engine/404: Handle spaces as output by new DOM > View conversion. ckeditor5-paste-from-office#81 (branch t/ckeditor5-engine/404).
• Util assertEqualMarkup() it is used in PFO tests to nicely format error messages: Introduce assertEqualMarkup() test util ckeditor5-utils#302.

[Reinmar]

The solution looks fine. The only place where we need to find some solution is how we share the block element names with DomConverter. Unfortunately, we need some central, publicly available place for that.

[jodator]

It makes sense to tie the isBlockFiller() to the DomConverter instance as it was done anyway in this proposal.

I've refactored the code so now you can check if a node is a block filler on the converter instance. Similarly, other utility methods are there (isElement, isComment, etc). So instead of isBlockFiller( domNode, domConverter.blockFillerMode ) you have domConverter.isBlockFiller( domNode ) which makes more sense.

[Reinmar]

FYI: I've just removed entire hasBlockParent( domNode, blockElements ) from code and all the tests still pass. I also realised that you didn't add any tests for isBlockFiller() which would cover those scenarios.

[Reinmar]

Added them in #1808.