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

Fix "Blank Space" When Editing Text With Floating Image at Beginning of Text #175

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mmichaelis
Copy link
Member

@mmichaelis mmichaelis commented Jan 8, 2024

The Issue

We identified an issue with our image integration, which restricts images to be only represented as "inline images": Given a structure like:

<p><img class="float-left"/></p>
<p>Text</p>

It gets rendered with a virtual "paragraph" initially by CKEditor, causing a "surprising blank space" to be shown above the "Text":

<p><img class="float-left"/></p>
<p></p>
<p>Text</p>

This issue got reproduced:

The issue extends to even more problems, like not being able to add text before the floating image.

Suggested Solution

The suggested solution is to also support block images, and get closer to the well-paved path in CKEditor 5, where both types are supported.

This again requires some deep changes, as we currently have a strong dependency on ImageInline and also break with the (undocumented) CKEditor 5 contracts, that each and every image must have a src attribute set in model layer (see ckeditor/ckeditor5#15653).

More to come, if we follow this track, like adapting our data-processing. The suggested approach is, to identify an image being the only element within a <p> tag as "block image" (toView) and later again transform it in toData mapping to a structure <p><img/></p>.

To-Do

  • Remove src attribute already in dataDowncast

  • Add toData data-processing from incoming structure for block-level images.

    Possibly not even required, if we ensure, that on dataDowncast the data-xlink-href is stored at the image and not at the figure.

  • Add toView data-processing to prepare some <figure> element.

    We may need to learn, which attributes (also) go to the <figure> element. Current observation is, that some class attribute values (here: alignment) are duplicated.

Example representation of a block-level image in data view layer:

<figure class="image float--left" data-xlink-href="content/900#properties.data">
  <img class="float--left"
       src="data:image/png;base64,iVBORw0KGg..."
       alt="Floating Image">
</figure><p>Test</p>

where the data only represent the data BLOB, that is irrelevant in data layer.

mmichaelis and others added 9 commits January 8, 2024 11:54
This is a preliminary commit showing some sketches to also support
the ImageBlock plugin. Much more is to be done (like data-processing),
but we are also blocked by our way of stripping the "src" attribute from
the model, which blocks switching the image type from ImageInline to
ImageBlock and vice versa.

Either, we keep some artificial non-empty src attribute in model, or
move the removal of the artificial attribute to the dataDowncast layer,
or, best option, CKEditor 5 opens the ImageTypeCommand to accept more
attributes than just `src` and `uploadId` as denoting a
"valid non-broken image".
There is no need, that we remove the `src` attribute from `<img>` during
upcast. It is enough, that we have (at least) a late processing in
data-processing, that ensures corresponding `src` attribute to be
stripped.

Nevertheless, it may make sense removing the `src` attribute already
earlier in `dataDowncast`, so that there is no need for the
data-processor to take core of it. Leaving the new state as is would
cause some subtle breaking change for anyone who wrote a custom
rich text data-processor.
Continued sketch how to possibly split up "depending" customizations,
i.e., have a dedicated plugin that integrates with the corresponding
plugins for ImageBlock- and ImageInline-editing.

Unused for now. Just exists as a rough sketch for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant