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

t/85: Linking an image should not be possible #132

Merged
merged 3 commits into from
Jul 6, 2017
Merged

t/85: Linking an image should not be possible #132

merged 3 commits into from
Jul 6, 2017

Conversation

oleq
Copy link
Member

@oleq oleq commented Jul 6, 2017

Suggested merge commit message (convention)

Fix: Linking an entire image should not be possible. Closes ckeditor/ckeditor5#4771.

@oleq oleq requested a review from oskarwrobel July 6, 2017 10:28
/* global document */

import Link from '../src/link';
import Image from '@ckeditor/ckeditor5-image/src/image';
Copy link
Contributor

Choose a reason for hiding this comment

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

Image should be added as a devDependency.

@oskarwrobel oskarwrobel merged commit 1a4e110 into master Jul 6, 2017
@oskarwrobel oskarwrobel deleted the t/85 branch July 6, 2017 12:07
@Reinmar
Copy link
Member

Reinmar commented Jul 13, 2017

This change is bad because it couples image and link features. This should not happen this way. This should be managed through schema and checkAttributeInSelection(). So, unless there's some problem to control this through schema (in which case it must be mentioned in https://github.com/ckeditor/ckeditor5-engine/issues/532) this PR should be reverted.

@oleq
Copy link
Member Author

oleq commented Jul 13, 2017

When the ImageCaption is on, linking doesn't work because doc.schema.checkAttributeInSelection( doc.selection, 'linkHref' ) indeed returns false in a such situation like [<image></image>].

However, when ImageCaption is on, the following rules are added to the schema:

schema.registerItem( 'caption', '$block' );
schema.allow( { name: '$inline', inside: 'caption' } );
schema.allow( { name: 'caption', inside: 'image' } );

which make checkAttributeInSelection return true when

[<image><caption>a</caption></image>]

and false when

[<image><caption></caption></image>]

When there's a text in <caption>, it automatically means the linking is possible. I have no idea what to use to check if an attribute is allowed <image>here<caption>...</caption></image> instead of <image><caption>there</caption></image> for any selection. The first, shallow check would certainly solve the issue.

@Reinmar
Copy link
Member

Reinmar commented Jul 13, 2017

OK, I know what's wrong here, actually. It's checkAttributeInSelection(). And the issue is strongly related to https://github.com/ckeditor/ckeditor5-engine/issues/986. The same kind of fix is needed in checkAttributeInSelection(), but... this will disable not only link but also bold and italic. So, I'm not sure if we want to do that.

Let's revert this change and get back to the original ticket.

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.

You shouldn't be able to link an image (just yet)
3 participants