#8959 DowncastWriter should handle UIElements consistently while wrapping and inserting

[niegowski]

Suggested merge commit message (convention)

Fix (engine): DowncastWriter should handle UIElements consistently while wrapping and inserting. Closes #8959.

Feature (engine): ContainerElement could be marked as isAllowedInsideAttributeElement and get wrapped with an AttributeElement. Closes #1633.

---

Additional information

This change introduces a view Element property isAllowedInsideAttributeElement (in the same fashion as priority on AttributeElement) to specify the element behavior while being wrapped with an AttributeElement. Also, it does not break attributes while inserting elements that are not present in the model (UIElement, probably should handle RawElement in the same way).

[Reinmar]

Out of curiosity: can we make the bold feature work in this case https://ckeditor.com/docs/ckeditor5/latest/framework/guides/tutorials/implementing-an-inline-widget.html#demo?

I'd like it to generate (i.e. the strong is added outside the placeholder):

<p>xxx <strong><placeholder>text</placeholder></strong></p>

I think it comes from: #1633.

[niegowski]

Out of curiosity: can we make the bold feature work in this case https://ckeditor.com/docs/ckeditor5/latest/framework/guides/tutorials/implementing-an-inline-widget.html#demo?

I'd like it to generate (i.e. the strong is added outside the placeholder):

<p>xxx <strong><placeholder>text</placeholder></strong></p>

I think it comes from: #1633.

I updated the PR to properly handle this case. Steps that are required:

• inline widget schema should add allowAttributesOf: '$text'
• downcast conversion should provide the option to use inline ContainerElement (so it could be wrapped by an AttributeElement):

const widgetElement = writer.createContainerElement( 'placeholder', null, { isInline: true } );

• widget inserting command should provide selection attributes to the inserted model element (to enable downcasting of a model attribute to an AttributeElement in the same way as InputCommand does for text nodes):

const attributes = model.document.selection.getAttributes();

const placeholder = writer.createElement( 'placeholder', {
	...Object.fromEntries( attributes ),
	type: 'placeholder'
} );

Also, I noticed that the only inline element that we currently support (softBreak) is breaking the AttributeElement and I'm not sure if this is the desired behavior.

[niegowski]

I'm not sure if this should not include rawElement also.

[niegowski]

But OTOH we could use RawElement (instead of UIElement) for suggestions in TC.

[Reinmar]

Notes from a call with @scofalik and @niegowski :

---

• Custom property vs private property?
  • Custom props is more for integrations
  • So, let's go with priv prop, just like the priority of attr elements
  • But, does it clone right now? TODO
• Do we need this property at all?
  • bold - no difference. Image can be inside <strong>
  • linkHref – is handled in a custom way on <img> (could be handled by converters)
  • Idea: The first common container element ancestor is the limit of attribute operations.
    • <attrelem:foo>
      • <containerelem:bar>
        • ... operations on attr elements here are limitted by the <containerelem:bar>
  • PK: For me, differentiating inline and block container elements might be useful.
  • SC: Model is controllable. View is generated based on this model and should implement the least possible restriction.
  • PK: For us, restrictions mean problems. For others: could be the other way around as restrictions give directions on how to do things.
  • Position mapping could be a problem. If you'd have <strong><p>xx</p></strong> the position after </p> will map to something weird in the model.
  • Cases:
    • Backwards compat
    • Future safety
    • View source
    • Weird data structure
• isInline is not a good name for this property
  • Why are UI/Raw/Empty elements "inline"? It's more about that attribute element should wrap this element.
  • isAllowedInAttributeElement.
• The current PR changes the behavior of suggestions
  • Previously: a suggestion markers splits attribute elements. Now: it lands inside.
  • Is it an important breaking change?
    • What if someone extracts content of suggestions by cutting out a piece between two suggestion markers?
    • There's a plugin for post-processing suggestions so we shouldn't need to worry.

[Reinmar]

• But, does it clone right now? TODO

If we go with the property – is it automatically copied when you clone an element?

And the second thing – what should be the default value of this property for UI/Raw/Empty elements? "allow in attr elements" seem safe and fine in this case.

[niegowski]

If we go with the property – is it automatically copied when you clone an element?

I missed that part - I'll add it. Also, I should update isSimilar()

And the second thing – what should be the default value of this property for UI/Raw/Empty elements? "allow in attr elements" seem safe and fine in this case.

I think that it should trigger the same behavior as the current implementation to replace isEmpty || isUI || isRaw while wrapping nodes. But changing the property name to allowedInline or allowedInAttributeElement would solve the confusion.

[niegowski]

I think that we should use some property to indicate allowedInline for the cases when we are mapping a single element in the model to multiple elements in the view. For example an image with an linkHref attribute is downcasted to <figure><a><img/></a></figure> and in this case, we don't want to end up with <a><figure><img/></figure></a>

[Reinmar]

After changing the opinion many times already, I'd go with the most specific isAllowedInsideAttributeElement.

If we'll ever have to have a more general isInline or allowInline we can set isAllowedInsideAttributeElement based on this more generic property. Let's just make sure we spot this moment when it's not only about attribute elements.

Also, please make sure to mention that property in DW#wrap() and DW#createAttributeElement() and AttributeElement itself.

Finally, we may consider allowing passing isAllowedInsideAttributeElement to createUI/Raw/EmptyElement too. It would make sense to make this controllable, especially for UIElements as they do not map to anything in the model so you cannot control that via model's schema or converters.

[niegowski]

Ready for another review round.

[Reinmar]

I'm missing some tests for the element classes themselves. There are test for create* methods in the DW, but the changes actually affected the element classes and it'd be good to cover that too.

Also, @scofalik could you review changes in the downcast writer? I checked all other files.

[niegowski]

I'm missing some tests for the element classes themselves. There are test for create* methods in the DW, but the changes actually affected the element classes and it'd be good to cover that too.

I added more tests.