Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attributes are set to all the element's children during upcast #4366

Closed
scofalik opened this issue Jun 25, 2018 · 1 comment · Fixed by ckeditor/ckeditor5-engine#1444
Closed
Assignees
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@scofalik
Copy link
Contributor

Originally reported on SO: https://stackoverflow.com/questions/50992491/ckeditor5-prevent-nested-same-type-elements-from-automatically-inheriting-paren

The issue is that during upcast we use data.modelRange which holds the range over the just-inserted elements so the converter knows where to operate. In attribute upcast though, the range that is set there is incorrectly used/interpreted.

function _setAttributeOn( modelRange, modelAttribute, conversionApi ) {
	let result = false;

	// Set attribute on each item in range according to Schema.
	for ( const node of Array.from( modelRange.getItems() ) ) {
		if ( conversionApi.schema.checkAttribute( node, modelAttribute.key ) ) {
			conversionApi.writer.setAttribute( modelAttribute.key, modelAttribute.value, node );

			result = true;
		}
	}

	return result;
}

This function sets attributes on passed modelRange. The thing is that for elements, the range is like this: [<div>...</div>] not like this: [<div>]...</div>. So, in the given example, the range to set attribute border on is [0] - [1] instead of [0] - [0,0]. This means that all elements inside will also have that attribute set.

There are two solutions here: either we change _setAttributeOn function to work correctly for such ranges (changing in the loop to .getItems( { shallow: true } ) should be enough). Or we change what data.modelRange is generated for elements during conversion. I think that the first fix is probably safer.

BTW. we didn't notice that bug before because we don't have any element that can contain itself. So, whenever we set an attribute on an element it was not allowed on any of its children, so it worked.

@scofalik scofalik self-assigned this Jun 25, 2018
@scofalik
Copy link
Contributor Author

This was generated with shallow: true:

image

Seems like that fix would work.

Reinmar referenced this issue in ckeditor/ckeditor5-engine Jun 25, 2018
Fix: Attributes were incorrectly set on an element's children during upcast. Closes #1443.
Reinmar referenced this issue in ckeditor/ckeditor5-engine Jun 27, 2018
Fix: Element to attribute upcast should set an attribute on all the elements inside the converted element. See #1443.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 19 milestone Oct 9, 2019
@mlewand mlewand added module:conversion type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants