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

Empty <a> tags removed automatically despite CKEDITOR.dtd settings #3267

Open
arnisjuraga opened this issue Jul 15, 2019 · 7 comments
Open
Assignees
Labels
core The issue is caused by the editor core code. status:confirmed An issue confirmed by the development team. type:bug A bug.

Comments

@arnisjuraga
Copy link

Type of report

I have searched for solution, but seems there are more users having hard time to use empty tags correctly: https://stackoverflow.com/questions/18250404/ckeditor-strips-i-tag - there are at lest 3 different solutions -

//`
CKEDITOR.dtd.$removeEmpty['a'] = false;
CKEDITOR.dtd.$removeEmpty.a = false;

//2
config.protectedSource.push(/<a[^>]*><\/a>/g);

//3
config.allowedContent = true;
config.extraAllowedContent = 'p(*)[*]{*};div(*)[*]{*};li(*)[*]{*};ul(*)[*]{*}';
CKEDITOR.dtd.$removeEmpty.i = 0;

DTD class alone does not help: https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_dtd.html#property-S-removeEmpty

Provide detailed reproduction steps (if any)

  1. Configure CKEDITOR to allow empty A tags.
  customConfig: '',
  allowedContent: true,    
  fillEmptyBlocks: true,
   [...]
  //CKEDITOR.dtd.$removeEmpty.a = 1;
  CKEDITOR.dtd.$removeEmpty.a = 0; 
  1. Paste HTML:
<div> 
<a class="empty-tag" href="#link1" style="background-image:url(image.jpg);"> 
</a> 
<a href="#link2"><span>TEXT</span></a> 
<span>TITLE</span> 
</div> 
  1. Switch between Source-Html; Empty A tag with href #link1 is removed together with div wrapper (which becomes empty, of course, if A is removed).

Codepen with tests: https://codepen.io/arnis-juraga/pen/NZZxPX

Expected result

A tag should either stay, or it should innerHTML replaced with &nbsp;

Actual result

See Codepen. Empty A tag removed.

IMPORTANT!

If you enter ANYTHING into first <div> tag, like X symbol in example - empty tag is not removed!

This works fine:

<div> X
<a class="empty-tag" href="#link1" style="background-image:url(image.jpg);"> 
</a> 
<a href="#link2"><span>TEXT</span></a> 
<span>TITLE</span> 
</div> 

If you rename this #link1 tag to i tag, and add

CKEDITOR.dtd.$removeEmpty.i = 0;

then the same HTML works fine (with i tag instead of a), check codepen:
https://codepen.io/arnis-juraga/pen/yddOgN

  • CKEditor version: 4.8, 4.12 tested
  • Installed CKEditor plugins: none

Some other resources, having difficulties to use empty tags:

https://stackoverflow.com/questions/18261198/ckeditor-removing-empty-span-automatically
https://stackoverflow.com/questions/44417887/how-to-make-ckeditor-stop-removing-empty-divs
https://stackoverflow.com/questions/11782236/ckeditor-removes-empty-tags

@arnisjuraga arnisjuraga added the type:bug A bug. label Jul 15, 2019
@engineering-this
Copy link
Contributor

I can confirm this bug.

Currently there is no way to preserve empty links without hacks.

https://github.com/ckeditor/ckeditor-dev/blob/9cb74b35db7082189088f47326eb05b02c947f8d/core/htmlparser/fragment.js#L64-L65

The source of this comes from line above. It looks like, it was to cover some IE upstream issue. Instead of removing empty links they should be filled with zero with space, eventually space can be removed when retrieving editor data.

@engineering-this engineering-this added core The issue is caused by the editor core code. status:confirmed An issue confirmed by the development team. labels Jul 16, 2019
@arnisjuraga
Copy link
Author

I am using workaround by adding dummy data-cke-survive="1" attribute to a tag. But still - very annoying

@arnisjuraga
Copy link
Author

arnisjuraga commented Nov 2, 2020

This is still a problem. Now, notices, that even having data-cke-survive="1" does not protect empty tags is removed if they contain containing "Zero-width join" (&zwj;).

I can't correctly reproduce it, because it seems to be dependent from some specific tag structure, but I have an exact HTML code which does reproduce it, if anyone is interested to fix it.

@cengizilhan
Copy link

This is still a problem. :(

@KarolDawidziuk
Copy link
Contributor

Thoughts after delving into this issue.

Overall, this is more complex than I initially assumed.
It involves filtering, selection, and changing the editor mode.

  1. The problems with selection.
    When we enable adding empty links to the editor, there is a problem with a selection in wysywig mode. When we try to select everything by using ctrl+a/command+a the empty links will be removed.

  2. Changing the editor mode.
    After I allow pass empty links, there was another problem with changing the editor mode from wysywig to sourcemode. Empty links have been removed even though changing from sourcemode to wysywig works fine, so we would have to spend extra time checking the differences in switching between modes.

  3. It's hacky.
    To fix the problem from the previous point, I added a zero width space or &nbsp; into empty links, but...
    Even if we have an empty link or link with &zwj; or &nbsp; there is no way to put caret into this link so that would be another problem to handle.

To sum up: After allowing the addition of empty links without additional &nbsp; or &zwj; there will be a problem with selection and putting caret inside empty links in wysywig mode.

@KarolDawidziuk KarolDawidziuk removed their assignment Jan 17, 2022
@sculpt0r sculpt0r assigned sculpt0r and unassigned sculpt0r Mar 31, 2022
@sculpt0r
Copy link
Contributor

sculpt0r commented Apr 1, 2022

So, we have a few places that are responsible for keeping/removing empty a element:

  • ckeditor4/core/filter.js

    Lines 1746 to 1752 in 3566f47

    function validateElement( element ) {
    switch ( element.name ) {
    case 'a':
    // Code borrowed from htmlDataProcessor, so ACF does the same clean up.
    if ( !( element.children.length || element.attributes.name || element.attributes.id ) )
    return false;
    break;
  • // Remove empty link but not empty anchor. (https://dev.ckeditor.com/ticket/3829, https://dev.ckeditor.com/ticket/13516)
    a: function( element ) {
    var attrs = element.attributes;
    if ( !( element.children.length || attrs.name || attrs.id || element.attributes[ 'data-cke-saved-name' ] ) )
    return false;
    }
  • function isRemoveEmpty( node ) {
    // Keep marked element event if it is empty.
    if ( node.attributes[ 'data-cke-survive' ] )
    return false;
    // Empty link is to be removed when empty but not anchor. (https://dev.ckeditor.com/ticket/7894)
    return node.name == 'a' && node.attributes.href || CKEDITOR.dtd.$removeEmpty[ node.name ];
    }

Those code fragments are connected with the old issues - mentioned in the comments.

I didn't encounter any issues with selection - which is a thing mentioned earlier by @KarolDawidziuk

TBC...

@sculpt0r
Copy link
Contributor

sculpt0r commented Apr 1, 2022

So we are touching core filter logic with this issue. Those cases intentionally ignore the $removeEmpty for some 'higher' purposes. It is possible to adjust this code to preserve the empty a element in the wysiwyg/source, but there are unit tests that are failing due to those initial changes.

Also, that might require setting the a element as removable by default in the dtd - which should be fine according to those places, but it is a breaking change if you want to verify the plain value of CKEDITOR.dtd.$removeEmpty[ 'a' ].

This will also require handling those old issues in this new reality. So probably we might ignore $removeEmpty anyway for the old IE issues.

After that, there will be still failing tests according to the 'anchor' feature. So my recommendation is also size: 'M' or size: 'XL' without the guarantee that we will be able to perfectly preserve backward compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core The issue is caused by the editor core code. status:confirmed An issue confirmed by the development team. type:bug A bug.
Projects
None yet
Development

No branches or pull requests

6 participants