ckeditor / ckeditor5

Powerful rich text editor framework with a modular architecture, modern integrations, and features like collaborative editing.
https://ckeditor.com/ckeditor-5
Other
8.27k stars 3.59k forks source link

Inline objects/widgets should be wrapped by a single span by `markerToHighlight` downcast helpers #16287

Open niegowski opened 2 weeks ago

niegowski commented 2 weeks ago

📝 Provide a description of the improvement

Follow-up of https://github.com/ckeditor/ckeditor5/pull/16275.

While debugging the restricted editing marker-to-highlight bug (https://github.com/ckeditor/ckeditor5/issues/16218) we realized that it ignores widgets in the data-pipeline downcast. It's happening because the data pipeline does not wrap objects with widget helpers and the addHighligh custom property is not available. In the PR we decided to use custom downcast converter for such a case but it looks like it would be better to integrate a solution with the markerToHighlight downcast helper.

The current implementation limits wrapping inline nodes to text only:

https://github.com/ckeditor/ckeditor5/blob/8e36734df880804e2fbcef3c10eac0888a3d9ce8/packages/ckeditor5-engine/src/conversion/downcasthelpers.ts#L1755-L1757

With the following visual effect (example in TC with an inline image):

Screenshot 2024-04-25 at 15 23 22 The image is not inside a common span element. Additional class is added to the image widget.

After changing the above mentioned lines of code in such a way:

if (
    !( data.item instanceof ModelSelection || data.item instanceof ModelDocumentSelection ) &&
    !conversionApi.schema.isInline( data.item )
) {
    return;
}

The visual result: Screenshot 2024-04-25 at 15 28 22 The image is included inside a single span element together with text nodes.

The main concern is the visual aspect for widgets higher than text but it could be beneficial for widgets that match the text size.


If you'd like to see this improvement implemented, add a 👍 reaction to this post.

scofalik commented 2 weeks ago

In general, I am a fan of this idea. If something is an inline element, it makes sense to keep it inside a <span> like we do with text.

But we would need a design discussion how we want to handle inline elements that are way bigger than line height, as in the example, it doesn't look that great. OTOH, it really looks much better with smaller inline elements. One thing to consider is that if such inline element (especially a big one) is the only element in the line (no text), you can barely see the marker with the proposed solution.