#8659: Implemented pasting/dropping heuristics to better recognize user's intention #9351

oleq · 2021-03-24T12:28:58Z

Suggested merge commit message (convention)

Internal (image): Implemented pasting/dropping heuristics to better recognize the user's intention. Closes #8659. Closes #9344.

In this branch, I propose the following heuristics for both internal and external paste/drop:

Pasting an inline image into an empty block or over an existing block (widget)

I assume the intent is to make the image block if pasted/dropped in this place and the image type is changed.

Pasting a captionless block image into a paragraph (block)

I see no good reason why the image should break the paragraph. For one thing, it is surprising. Also, if the paragraph was broken, this would be a costly operation from the UX standpoint to put things back together (undo or manual backspacing). IMO in my approach, the user gets better UX at virtually no cost.

Pasting a block image with a caption into a paragraph (block)

I didn't change anything in this scenario because:

if there is a caption, they probably meant to have it, and converting into an inline image would mean getting rid of it,
- that would be a surprise,
if we really got rid of the caption, we'd need some indicator, for instance, a hint pinned to the "toggle caption" button and/or a toast notification to let the user know they can see the caption and it wasn't lost,
- at this moment we don't have resources to implement either of the systems,
- we'd also need to implement the view figcaption -> view img attribute transition (for instance cke-data-caption) during paste to preserve the caption content in the upcast and store it in the caption model attribute (that we already have) so it can be reverted on demand,
  - that's another layer of complexity we don't need right now

…ge style manual test.

…tention.

…ImageBlockEditing.

Reinmar · 2021-03-24T17:02:48Z

if there is a caption, they probably meant to have it, and converting into an inline image would mean getting rid of it,

that would be a surprise,

👍 That was my thought as well.

I agree with the other scenarios too.

…id of repeating code.

…eality the cases are handled as a paste.

…Editing plugin.

…diting plugin.

oleq · 2021-03-30T11:16:11Z

TODO: https://github.com/ckeditor/ckeditor5/pull/9352/files could benefit from isBlockViewImage introduced in this PR. If that PR lands first, let's align the code to new helper.

niegowski · 2021-03-31T14:40:45Z

packages/ckeditor5-image/src/image/utils.js

+export function isInlineViewImage( element ) {
+	return !!element && element.is( 'element', 'img' );
+}


This util is very generic so I can image cases when someone would use it on the img element inside a figure and it would give an invalid result. Maybe we should check if this element's parent is not a figure?

Also, I'm not sure about the name of this util, maybe isInlineImageView() would be better? Same for isBlockViewImage() -> isBlockImageView().

This util is very generic so I can image cases when someone would use it on the img element inside a figure and it would give an invalid result. Maybe we should check if this element's parent is not a figure?

TBH I don't see anything wrong with using it for the inner structure of a block widget; it already happens in this PR in getViewImageFromWidget().

niegowski · 2021-03-31T14:48:57Z

packages/ckeditor5-image/src/image/utils.js

+export function determineImageTypeForInsertionAtSelection( editor, selection ) {
+	const schema = editor.model.schema;


Maybe we could pass schema in params instead of the whole editor?

Also, this is missing the editor.config.get( 'image.insert.type' ) to allow forcing default image type on insertion.

Also, this is missing the editor.config.get( 'image.insert.type' ) to allow forcing default image type on insertion.

I'm not sure whether this tool should respect this configuration.

When used in insertImage(), some other logic handles this configuration and that's OK.

If we're going to use it in the clipboard pipeline, though, this would be partially against the module:image/imageinsert~ImageInsertConfig#type documentation which states it works for new images only. And since we have no means to discover whether a pasted image is returning back to the same editor or comes from an external data source, I'd rather not use it here. This way copy/paste inside the editor will be at least more predictable.

packages/ckeditor5-image/src/image/imageblockediting.js

packages/ckeditor5-image/tests/image/imageinlineediting.js

packages/ckeditor5-image/tests/image/utils.js

Co-authored-by: Kuba Niegowski <1232187+niegowski@users.noreply.github.com>

niegowski

👍 LGTM except for the failing tests in the link package.

oleq · 2021-04-06T15:52:37Z

👍 LGTM except for the failing tests in the link package.

Fixed.

oleq added 3 commits March 24, 2021 12:12

Tests: Make sure the inspector attaches in the right order in the ima…

b04c180

…ge style manual test.

Implemented pasting/dropping heuristics to better recognize user's in…

9e5f324

…tention.

Added missing ClipboardPipeline dependency to ImageInlineEditing and …

065eea7

…ImageBlockEditing.

oleq added 6 commits March 26, 2021 09:39

Implemented isInlineViewImage() and isBlockViewImage() utils to get r…

0c366ba

…id of repeating code.

Tests: Fixed image upload tests that wrongly simulated drop when in r…

698c3c5

…eality the cases are handled as a paste.

Tests and docs.

8831989

Tests: Added clipboard pipeline integration tests for the ImageInline…

121551f

…Editing plugin.

Tests: Added clipboard pipeline integration tests for the ImageBlockE…

cb2f8cf

…diting plugin.

Docs [skip ci].

d09d35f

oleq marked this pull request as ready for review March 26, 2021 12:54

oleq requested a review from niegowski March 26, 2021 12:54

oleq mentioned this pull request Mar 30, 2021

I/9117 inline images in tables #9352

Merged

niegowski requested changes Mar 31, 2021

View reviewed changes

oleq and others added 15 commits April 1, 2021 11:49

Merge branch 'i/2052-inline-images' into i/8659-clipboard-improvements

c0118ad

Docs.

6df6538

Renamed image helpers.

71ecf71

Simplified arguments of determineImageTypeForInsertionAtSelection().

c48ecc1

Code refactoring.

0835f06

Code refactoring.

f4c372b

Code refactoring.

87227a9

Update packages/ckeditor5-image/tests/image/imageblockediting.js

ef9cc6b

Co-authored-by: Kuba Niegowski <1232187+niegowski@users.noreply.github.com>

Update packages/ckeditor5-image/tests/image/imageinlineediting.js

32132ba

Co-authored-by: Kuba Niegowski <1232187+niegowski@users.noreply.github.com>

Update packages/ckeditor5-image/tests/image/imageinlineediting.js

f6dfd83

Co-authored-by: Kuba Niegowski <1232187+niegowski@users.noreply.github.com>

Update packages/ckeditor5-image/tests/image/imageinlineediting.js

ddd35a6

Co-authored-by: Kuba Niegowski <1232187+niegowski@users.noreply.github.com>

Update packages/ckeditor5-image/tests/image/imageinlineediting.js

86dd038

Co-authored-by: Kuba Niegowski <1232187+niegowski@users.noreply.github.com>

Apply suggestions from code review

0911e78

Co-authored-by: Kuba Niegowski <1232187+niegowski@users.noreply.github.com>

Code refactoring.

a9c08a7

Code refactoring.

f7e65ee

Used the isBlockImageView() helper in the getImageTypeMatcher().

902be9e

oleq requested a review from niegowski April 6, 2021 13:40

niegowski approved these changes Apr 6, 2021

View reviewed changes

Fixed broken LinkImage integration.

5ab8180

oleq merged commit df3cff5 into i/2052-inline-images Apr 7, 2021

oleq deleted the i/8659-clipboard-improvements branch April 7, 2021 08:30

This was referenced Apr 7, 2021

Inline images - Support for clipboard #8659

Closed

Improve the UX of pasting block images into paragraphs #9344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#8659: Implemented pasting/dropping heuristics to better recognize user's intention #9351

#8659: Implemented pasting/dropping heuristics to better recognize user's intention #9351

oleq commented Mar 24, 2021 •

edited

Loading

Reinmar commented Mar 24, 2021

oleq commented Mar 30, 2021

niegowski Mar 31, 2021

niegowski Mar 31, 2021

oleq Apr 1, 2021

niegowski Apr 6, 2021

niegowski Mar 31, 2021

niegowski Mar 31, 2021

oleq Apr 1, 2021

niegowski Apr 6, 2021

niegowski left a comment

oleq commented Apr 6, 2021

		export function determineImageTypeForInsertionAtSelection( editor, selection ) {
		const schema = editor.model.schema;

#8659: Implemented pasting/dropping heuristics to better recognize user's intention #9351

#8659: Implemented pasting/dropping heuristics to better recognize user's intention #9351

Conversation

oleq commented Mar 24, 2021 • edited Loading

Suggested merge commit message (convention)

Pasting an inline image into an empty block or over an existing block (widget)

Pasting a captionless block image into a paragraph (block)

Pasting a block image with a caption into a paragraph (block)

Reinmar commented Mar 24, 2021

oleq commented Mar 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niegowski left a comment

Choose a reason for hiding this comment

oleq commented Apr 6, 2021

oleq commented Mar 24, 2021 •

edited

Loading