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

Add insertObject() function. #11425

Merged
merged 29 commits into from
Apr 1, 2022

Conversation

CatStrategist
Copy link
Contributor

@CatStrategist CatStrategist commented Mar 7, 2022

Suggested merge commit message (convention)

Feature (engine): Added a new function insertObject() to the Model for inserting elements defined as objects by schema into a model.

Feature (engine): Added a new function setAllowedAttributes() to the Schema that validates attributes if they are allowed on given element before setting them.

Feature (engine): Added a new function getAttributesWithProperty() to the Schema that retrieves attributes from a node which have given property.

Feature (media-embed): Added optional findOptimalPosition parameter to insertMedia() function that allows for inserting media element without breaking content.

Feature (paragraph): Added optional options.attributes parameter to InsertParagraph command that allows setting attributes on created paragraph.

Fix (list): Fixed fixing indent of inserted content into document list in documentlistediting fixer.

Internal (engine): findOptimalPosition() is now also included in engine for internal use.

Internal (table, page-break, horizontal-line, media-embed, html-embed, image): table, pageBreak, horizontalLine, media, imageBlock,imageInline elements are now inserted with insertObject() function instead of insertContent(). Closes #11198.


Additional information

Changes to deletecontent, widgettypearound that handles copyOnReplace attributes' property are not described above. Changes to moving findOptimalPosition to engine not described yet.

} else {
insertionContext = model.document.selection.anchor.parent;
}

Copy link
Member

@Reinmar Reinmar Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imageType is calculated below, so the order should be different.

// This applies only when we don't have the selectable specified (i.e., we insert multiple block images at once).
findOptimalPosition: !selectable && imageType != 'imageInline'
}
);

// Inserting an image might've failed due to schema regulations.
if ( imageElement.parent ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this condition be also included in setSelection inside insertObject?

const firstSelectedBlock = selection.getSelectedBlocks().next().value;
const attributesToCopy = model.schema.getAttributesWithProperty( firstSelectedBlock, 'copyOnReplace', true );

writer.setAttributes( attributesToCopy, object );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be set on the block element (so for an inline image that requires auto paragraphing those attributes should be set on the paragraph that wrapped an inline image.

Also attributes should be verified if are applicable to a specified object (according to the schema).

model.deleteContent( insertionSelection, { doNotAutoparagraph: true } );
}

writer.insert( paragraph, insertionSelection.anchor );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would wrap the object with this paragraph and use the paragraph with the inline object as an object to be inserted. On the new object (the paragraph) we need to set those attributes that should persist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember that final selection should be set relative to the object, not the paragraph (set selection on an object, not on a paragraph).

insertionSelection = writer.createPositionAt( paragraph, 0 );
}

const affectedRange = model.insertContent( object, insertionSelection, offset, options );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const affectedRange = model.insertContent( object, insertionSelection, offset, options );
const affectedRange = model.insertContent( object, insertionSelection, offset );


// If the element is missing, but a paragraph could be inserted next to the element, let's add it.
if ( !canSetSelection && model.schema.checkChild( contextElement.parent, 'paragraph' ) ) {
nextElement = writer.createElement( 'paragraph' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably should also apply those attributes that should persist.

@niegowski niegowski changed the base branch from ck/10812-document-list-editing to ck/epic/2973-document-lists March 22, 2022 15:09
@CatStrategist CatStrategist marked this pull request as ready for review March 31, 2022 14:42
@oleq
Copy link
Member

oleq commented Mar 31, 2022

I guess pasting a widget on a fake typearound selection caret should also respect list attributes. The paste should be in the same list item. Same with pasting text on a fake caret.

@@ -281,6 +282,8 @@ export function toWidgetEditable( editable, writer ) {
}

/**
* TODO this is only wrapping the engine util for the cases where selection is tested for possibility to insert some object.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO.

} );
}

// TODO findOptimalInsertionRange should be exported or exposed in some reasonable place to be used in the widget util of the same name.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO.

packages/ckeditor5-engine/src/model/utils/insertobject.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/utils/insertobject.js Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/utils/insertobject.js Outdated Show resolved Hide resolved
@CatStrategist CatStrategist changed the title WIP: Initial version of fix handling insertion of blocks to document … Add insertObject() function. Mar 31, 2022
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍. Waiting for feedback from the QA team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants