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

Inline widget styling #1633

Closed
dkrahn opened this issue Mar 15, 2019 · 15 comments · Fixed by #8998
Closed

Inline widget styling #1633

dkrahn opened this issue Mar 15, 2019 · 15 comments · Fixed by #8998
Assignees
Labels
package:engine support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@dkrahn
Copy link
Contributor

dkrahn commented Mar 15, 2019

Is this a bug report or feature request? (choose one)

🐞 Bug report or documentation improvement

💻 Version of CKEditor

12.0.0

📋 Steps to reproduce

  1. Follow the steps from the Inline Widget docs (https://ckeditor.com/docs/ckeditor5/latest/framework/guides/tutorials/implementing-an-inline-widget.html) or (https://ckeditor5.github.io/misc/placeholder-demo/index.html)
  2. Try to style all the text

✅ Expected result

From my perspective, it would be desired of the inline widget to be treated as part of the text when it comes to styling(bold, italic, font...).

❎ Actual result

The text around the inline widget get styled but the widget itself does not.

📃 Other details that might be useful

I was able to get around this issue with changing the createContainerElement with createAttributeElement, adding a random id and priority 20.
And then adding the text using the private _insertChild converting elementToElement. But this does not seam right.


If you'd like to see this feature implemented, add 👍 to this post.

@Mgsy
Copy link
Member

Mgsy commented Mar 18, 2019

cc @jodator

@jodator
Copy link
Contributor

jodator commented Mar 18, 2019

Right now we do not support this out of the box. I think that view writers should be updated to support isInline=true elements in wrapping in attributes elements but I think that's a broader topic.

//cc @Reinmar & @scofalik.

Edit: I've managed to crate a <strong> inside a placeholder in the view but using private API isn't a good way to do it.
outcome:

<p>
	<strong>Bold </strong>
	<placeholder><strong>{place}</strong></placeholder>
	<strong>.</strong>
</p>

but shouldn't it be?

<p>
	<strong>Bold 
	<placeholder>{place}</placeholder>
	.</strong>
</p>

@dkrahn
Copy link
Contributor Author

dkrahn commented Mar 18, 2019

Hi @jodator
IMHO the outcome should be se second one:

<p>
	<strong>Bold 
	<placeholder>{place}</placeholder>
	.</strong>
</p>

And about using the private API, that is exactly why I opened this issue, it doesn't look right.

@Reinmar
Copy link
Member

Reinmar commented Mar 18, 2019

<strong> should definitely be outside <placeholder> in the view. It needs to be done in three steps:

  1. First (simplest), you need to use allowAttributesOf: '$text' in your widget's schema definition.
  2. Then, we need to make sure that basic styles correctly apply to such elements. Right now, they may be implemented in a way that they only look for text nodes. This needs to be generalised so they look for all inline nodes.
  3. Finally, and this is the biggest challenge, if we'll want to render <strong><placeholder/></strong> in the view we need to render a ContainerElement inside an AttributeElement which isn't allowed today. Today, all the code assumes that there's only text or UIElement (and perhaps EmptyElement) in AttributeElements. We may need to either remove that limitation or introduce a new element type.

Anyway, tl;dr is that unfortunately this isn't supported yet and it may not be that easy to introduce (in a stable way).

@Reinmar Reinmar added status:confirmed package:engine type:feature This issue reports a feature request (an idea for a new functionality or a missing option). labels Mar 18, 2019
@Reinmar Reinmar added this to the next milestone Mar 18, 2019
@lslowikowska lslowikowska added the support:1 An issue reported by a commercially licensed client. label Apr 23, 2019
@pragmaticus
Copy link

pragmaticus commented Jun 8, 2019

Any progress on this?

@jodator, @dkrahn I couldn't reproduce the workaround by using the private API. Can you post an example or guide me in the right direction?

@remigasas
Copy link

Hello, any update on this ?

Placeholders needs to be styled with font, bold, italic, etc. Their purpose is to provide a block that will be replaced with actual text. Without correct style they are not usable.

@Reinmar Reinmar modified the milestones: nice-to-have, next Aug 16, 2019
@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2019

Not yet. It will be a pretty big change, unfortunately, so we need to find space for it in our roadmap. At the moment, I can't promise anything, but it's high on our wishlist too.

@lslowikowska lslowikowska added support:2 An issue reported by a commercially licensed client. and removed support:1 An issue reported by a commercially licensed client. labels Jan 9, 2020
@nikunjtrapasiya
Copy link

Any progress on this?

@crypto-dump
Copy link

Is there any update on this feature?

@Xusifob
Copy link

Xusifob commented Jun 9, 2020

Hello,

Any information on this feature ? :)

@Xusifob
Copy link

Xusifob commented Jun 16, 2020

I created a workaround for this. It's not optimal but instead of creating <strong> or <em> HTML elements inside the inline widget, I add a strong="true", italic="true" attributes in the widget element. Does the work for me.
You can check it out inside my fork

@dkrahn
Copy link
Contributor Author

dkrahn commented Aug 28, 2020

Any updates with the new version?

@QuantumGlitch
Copy link

Hi! I will be really interested in this feature too!

@dkrahn
Copy link
Contributor Author

dkrahn commented Oct 1, 2020

Hi,
Any estimates or updates on when this issue might get in the roadmap?
Thanks for the great work you all are doing!

@DaveOrDead
Copy link

DaveOrDead commented Oct 22, 2020

Hey @dkrahn, hit the exact same situation following the tutorial. Are you able to elaborate on how you achieved your workaround as I haven't been able to figure out how to get it working.

I was able to get around this issue with changing the createContainerElement with createAttributeElement, adding a random id and priority 20.
made sense - updated to:

const placeholderView = viewWriter.createAttributeElement(
	"span",
	{class: "placeholder"},
	{ id: "placeholder:my", priority: 20 }
);

but this part:

And then adding the text using the private _insertChild converting elementToElement.

I tried switching

conversion.for("dataDowncast").elementToElement({

in the tutorial for

conversion.for("dataDowncast")._insertChild({

but I get the error
t.for(...)._insertChild is not a function

Any guidance would be great

@pomek pomek added this to the nice-to-have milestone Nov 10, 2020
@niegowski niegowski modified the milestones: nice-to-have, iteration 40 Feb 10, 2021
@niegowski niegowski self-assigned this Feb 10, 2021
Reinmar added a commit that referenced this issue Feb 22, 2021
Fix (engine): `DowncastWriter` should handle `UIElements` consistently while wrapping with and inserting them into attribute elements. Closes #8959.

Feature (engine): `ContainerElement` can be marked as `isAllowedInsideAttributeElement` in order to allow wrapping it with attribute elements. Useful for instance for inline widgets. Other element types (UI, Raw, Empty) have this flag on by default but it can be changed via `options.isAllowedInsideAttributeElement` to `false`. Read more in `DowncastWriter#create*()` methods documentation. Closes #1633.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet