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

You shouldn't be able to link an image (just yet) #4771

Closed
Reinmar opened this issue Mar 10, 2017 · 5 comments · Fixed by ckeditor/ckeditor5-link#132
Closed

You shouldn't be able to link an image (just yet) #4771

Reinmar opened this issue Mar 10, 2017 · 5 comments · Fixed by ckeditor/ckeditor5-link#132
Labels
package:link type:bug This issue reports a buggy (incorrect) behavior. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@Reinmar
Copy link
Member

Reinmar commented Mar 10, 2017

The link feature should be disabled when an image is selected. Most likely some state check is missing.

In the future, we'll add the possibility to link images, but so far we don't have converters for that.

Alternatively, instead of fixing the state check now, we can add a converter allowing linking images. But the state check needs to be fixed at some point anyway.

@Reinmar Reinmar changed the title You can link an image You shouldn't be able to link an image (just yet) Mar 10, 2017
@Reinmar
Copy link
Member Author

Reinmar commented Mar 10, 2017

To clarify why images can't be linked yet – the problem here is that so far we support linking text, so we can convert text attribute to a <a> element.

With images we need to convert an element attribute to an <a> element. Also, the <a> itself should land inside the <figure> element:

<figure class="image">
  <a href="..."><img src="..." alt="..."></a>
  <figcaption>Caption.</figcaption>
</figure>

The model would look like this:

<image linkHref="..." src="..." alt="...">
   <caption>Caption.</caption>
</image>

@oleq oleq self-assigned this Jul 5, 2017
oskarwrobel referenced this issue in ckeditor/ckeditor5-link Jul 6, 2017
Fix: Linking an entire image should not be possible. Closes #85.
@Reinmar
Copy link
Member Author

Reinmar commented Jul 13, 2017

Please see the discussion in ckeditor/ckeditor5-link#132 (comment).

The quick fix was too brutal. A slightly better fix would be to change this behaviour in Schema#checkAttributeInSelection() but that means affecting also bold and italic and we decided no to disable them on images (at least not yet). See https://github.com/ckeditor/ckeditor5-core/issues/73#issuecomment-311602619.

So, we need to figure out the right solution and as this issues is not annoying or something (the editor doesn't blow up) this can be postponed. I guess that at some point we'll simply implement the ability to link images.

@Reinmar Reinmar reopened this Jul 13, 2017
@oleq
Copy link
Member

oleq commented Jul 14, 2017

So, we need to figure out the right solution and as this issues is not annoying or something (the editor doesn't blow up) this can be postponed. I guess that at some point we'll simply implement the ability to link images.

I wonder if the issue may re–appear in other features (also in the future). Essentially the case is that some inline attribute can be applied in such situation

ab<element><child>[c]</child></element>de

but cannot be applied to the element as a whole in

ab[<element><child>c</child></element>]de

It might be a very common case in different kind of widgets, e.g. if we ever decided blockquote should be one, etc.. We might need to figure out the solution anyway then.

@Reinmar
Copy link
Member Author

Reinmar commented Jul 14, 2017

I'm not sure what you mean. The other cases which I mentioned are bold and italic. They are a slightly different case than link, but the effect is identical – if you apply bold to a selected image that bold applies in the caption. While it's hard to tell whether this makes sense with an image, that would certainly be the expected result if blockquote was some kind of widget.

The editor will work in these cases as long as schema is well defined. However, it's a UX question whether people want to apply inline styles to block widgets at all. And which of these inline styles should rather apply to the whole element (like we want the link to do).

@Reinmar
Copy link
Member Author

Reinmar commented Apr 5, 2018

I'm not sure why we kept this issue open. The only one which makes sense now is: #702 (allow linking images).

@Reinmar Reinmar closed this as completed Apr 5, 2018
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-link Oct 9, 2019
@mlewand mlewand added resolution:solved type:bug This issue reports a buggy (incorrect) behavior. type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:link labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:link type:bug This issue reports a buggy (incorrect) behavior. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants