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

[GHS] HTML elements wrapped in <a>s are deleted #13803

Closed
Witoso opened this issue Mar 31, 2023 · 9 comments · Fixed by #14410
Closed

[GHS] HTML elements wrapped in <a>s are deleted #13803

Witoso opened this issue Mar 31, 2023 · 9 comments · Fixed by #14410
Assignees
Labels
package:html-support squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Witoso
Copy link
Member

Witoso commented Mar 31, 2023

📝 Provide detailed reproduction steps (if any)

Reported by Drupal community: https://www.drupal.org/project/drupal/issues/3349893

<div class="adblock">
  <a href="/link">
    <picture>
      <source media>
    </picture>
  </a>
</div>
  1. Copy/paste that first snippet into https://ckeditor.com/docs/ckeditor5/latest/features/html/general-html-su...'s source view
  2. Turn off the source view.
  3. Return to the source view.

✔️ Expected result

There's no data loss.

❌ Actual result

The wrapping <a> is lost!


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@Witoso Witoso added the type:bug This issue reports a buggy (incorrect) behavior. label Mar 31, 2023
@Witoso Witoso added squad:core Issue to be handled by the Core team. package:html-support labels Mar 31, 2023
@Witoso Witoso changed the title HTML elements wrapped in <a>s are deleted [GHS] [GHS] HTML elements wrapped in <a>s are deleted May 4, 2023
@RaspberryBlack
Copy link

RaspberryBlack commented May 23, 2023

I believe the following is the same issue:
Drupal issue: https://www.drupal.org/project/drupal/issues/3362002

  1. Add the following snippet via source (https://ckeditor.com/ckeditor-5/demo/html-support/):
<h2>
    <div>Test</div>
</h2>
  1. Turn off the source view.
  2. Return to the source view.

✔️ Expected result

The html stays the same.

❌ Actual result

The html changes to the following, stripping the h2

<div>
    Test
</div>

@Witoso
Copy link
Member Author

Witoso commented May 25, 2023

Thanks @RaspberryBlack, added a new issue for this case: #14249.

@Witoso
Copy link
Member Author

Witoso commented Jun 5, 2023

  1. To figure out if we need to fix only a wrapping picture or a wrapping any block element.
  2. a wrapping any block element is a very complex topic.

@neclimdul
Copy link

Using div inside a a is semantically correct in HTML 5 so I'd say that's the correct solution at the end of the day. If you don't, its going to be just one element after another that need to be able to be wrapped (foreshadowing).

Also the more general solution is probably what I and Drupal actually need as well. In Drupal, we have a media embed tool that allows embedding complex media inside ckeditor. Using this the user might insert all sorts of complicated html depending on what the site developer implements. As an example, I have a site with an image embed that uses a figure and a video embed that wraps with a div. Other sites might use picture like the original poster, have a div around that image to create a card, etc,

Despite being a figure, that embed image feels like a normal image and because Drupal's widget integration they can generally interact with it like one. They want to just click the link tool and wrap it in a link. All our technology, browsers, styles, etc are setup to work with it wrapped as a link but because of this bug it doesn't work for them. They're frustrated because they can't "just link and image in ckeditor"

@Witoso
Copy link
Member Author

Witoso commented Jun 5, 2023

Thanks @neclimdul for more context. The complexity we are facing is that CKEditor 4 has a totally different implementation than CKEditor 5 and different mechanisms of extensibility. CKE5 operates on the internal model and needs to know what to accept to it, and how to represent it in HTML. The model gives us a "pure" abstraction layer, a safer development, etc., in return for some cost of agility and freedom.

Using this the user might insert all sorts of complicated html depending on what the site developer implements.

And this is also possible for CKE5 but inserting this complex HTML needs to be aligned with the way plugin development works. In fact, in most complex representations, you will not be inserting HTML but predefined models' elements, and conversion will handle the HTML output for editing and data view.

That said, we are aware of the migration of content from CKEditor 4 (as well as advanced Drupal cases), and we need to prepare for some cases with special ways of handling them.

Despite being a figure, that embed image feels like a normal image and because Drupal's widget integration they can generally interact with it like one. They want to just click the link tool and wrap it in a link.

And actually, they can, as we have integration of picture with a link as well as figure (through our model's imageBlock|Inline). This can be tested for example on our CKBox page. Upload an image, click it, press link, and add an URL. That's why I have a few comments on the reported problem, and (I think) faulty HTML representation.

As for other elements more research needs to be done on our side.

@Witoso
Copy link
Member Author

Witoso commented Jun 5, 2023

As for the OP, I ran some additional tests, and in fact, the picture is integrated with the link. You can test it with the content:

<div class="adblock">
    <a href="/link">
      <picture>
        <source media="">
        <img src="image">
      </picture>
    </a>
</div>

on the GHS demo.
I pasted such content onto the Drupal 10 (Full HTML), and it was preserved as well.

The problem appears, due to the wrong usage of a picture element. Per specification:

Zero or more source elements, followed by one img element, optionally intermixed with script-supporting elements.

We store linkHref on the model's imageBlock|Inline, the lack of img element, prevents the integration with the linking to kick in.

To be discussed how to approach a fix in the picture area (should we even fix something here?) and the relation to the more general problem of HTML elements wrapped in a's.

@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Jun 15, 2023
@Witoso
Copy link
Member Author

Witoso commented Jun 16, 2023

The fix most likely just requires two actions:

packages/ckeditor5-html-support/src/schemadefinitions.ts

add in inline objects group for picture

{
model: 'htmlPicture',
view: 'picture',
isObject: true,
modelSchema: {
  inheritAllFrom: '$inlineObject'
  }
},

Extend htmlCustomElement definition to include in model schema:

allowAttributesOf: '$inlineObject',

this way we will handle elements that are not known to GHS.

@Witoso
Copy link
Member Author

Witoso commented Jun 16, 2023

An important point to mention - we will support a wrapping blocks the same way CKE4 did it. This means although there won't be a data loss, there may be a data change. If the data change is not acceptable, there needs to be more direct integration with the linking provided by the tools that insert such content (e.g. entity embeds).

Examples below:

Before

<a href="https://ckeditor.com">
  <div>CKEDITOR</div>
</a>

After

<div><a href="https://ckeditor.com">CKEDITOR</a></div>

Before

<a href="https://ckeditor.com">

<figure>
<blockquote cite="https://www.huxley.net/bnw/four.html">
<p>Words can be like X-rays, if you use them properly—they’ll go through anything. You read and you’re pierced.</p>
</blockquote>

<figcaption>—Aldous Huxley, <cite>Brave New World</cite></figcaption>
</figure>

</a>

After

<figure>
<blockquote cite="https://www.huxley.net/bnw/four.html">
<p><a href="https://ckeditor.com">Words can be like X-rays, if you use them properly—they’ll go through anything. You read and you’re pierced.</a></p>
</blockquote>

<figcaption><a href="https://ckeditor.com">—Aldous Huxley, <cite>Brave New World</cite></a></figcaption>
</figure>

@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Jun 19, 2023
@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Jun 19, 2023
arkflpc added a commit that referenced this issue Jun 20, 2023
Fix (html-support): GHS should allow linking of custom elements. Closes #13803 .
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jun 20, 2023
@CKEditorBot CKEditorBot added this to the iteration 64 milestone Jun 20, 2023
@wimleers
Copy link

🥳 Reported the progress at https://www.drupal.org/project/drupal/issues/3349893#comment-15113038 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:html-support squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants