Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Element to attribute upcast should set attribute on all the elements inside the converted element #1449

Merged
merged 2 commits into from
Jun 27, 2018

Conversation

scofalik
Copy link
Contributor

@scofalik scofalik commented Jun 27, 2018

Suggested merge commit message (convention)

Fix: Element to attribute upcast should set attribute on all the elements inside the converted element.


Additional information

This PR provides a fix after tests started to fail in ckedior5-basic-styles after merging ckeditor/ckeditor5#4366. As of now, element to attribute upcast will set attribute on all children of the element, while attribute to attribute upcast will set the attribute only on the element.

So:

<strong><p>Foo</p></strong> will convert to <paragraph><$text bold="true">Foo</$text></paragraph>

while (assuming a plugin handling class attribute):

<div class="dark"><div>Foo</div></div> will convert to <div class="dark"><div>Foo</div></div>

even though inside <div> could accept class attribute dark.

@scofalik scofalik requested a review from Reinmar June 27, 2018 10:57
@scofalik
Copy link
Contributor Author

To clarify:

In upcasting we use data.modeRange which describes on which part of the model we operate. When upcasting to attribute we use that range to set the model attribute "on that range".

Before any fix, the model attribute was applied to all elements in that range. So this incorrect situation happened:

<div class="xyz"><div>Foo</div></div> -> <div class="xyz"><div class="xyz">Foo</div></div>

So, in the fix, I changed it the way that only top-level elements receive the attribute:

<div class="xyz"><div>Foo</div></div> -> <div class="xyz"><div>Foo</div></div>

This caused a regression in following scenario in autoparagraphing:

<strong>Foo</strong> -> <strong><p>Foo</p></strong> -> <paragraph>Foo</paragraph>

because bold attribute has been tried to be set only on <paragraph> not on the text.

So I made another fix. Now, if element-to-attribute upcast is used, it works as earlier, so everything in the range gets the attribute. It makes sense. After all, all the elements were inside the upcasted element in the original view.

However, if there is attribute-to-attribute conversion, it is assumed, that the view attribute is connected only with its element, so the attribute is set only on top-level node.

@Reinmar Reinmar merged commit 26673a0 into master Jun 27, 2018
@Reinmar Reinmar deleted the t/1443-2 branch June 27, 2018 16:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants