Link Decorators: NoFollow not being added, creates 2 separate links

peppies commented 4 years ago

📝 Provide detailed reproduction steps (if any)

I'm following this demo example: https://ckeditor.com/docs/ckeditor5/latest/features/link.html#demo-2

My goal is to have the "addTargetToExternalLinks: true" for all external links and manually have the option to add "rel='nofollow'" to some links. I customized the configuration for manually adding a NoFollow attribute to some external links:

    link: {
            // Automatically add target="_blank" and rel="noopener noreferrer" to all external links.
            addTargetToExternalLinks: true,

            decorators: [
                {
                    mode: 'manual',
                    label: 'NoFollow',
                    attributes: {
                        rel: 'nofollow'
                    }
                }
            ]
        }

✔️ Expected result

<a target="_blank" rel="noopener noreferrer nofollow" href="https://www.google.com" data-aalisten="1">test</a>

❌ Actual result

<a target="_blank" rel="noopener noreferrer" href="https://www.google.com" data-aalisten="1"><a rel="nofollow" data-aalisten="1">test</a></a>

📃 Other details

Browser: Chrome 80.0.3987.132
OS: Windows 10
CKEditor version: 5
Installed CKEditor plugins: Custom Build:

{
"name": "ckeditor5-custom-build",
"author": "CKSource",
"description": "A custom CKEditor 5 build made by the CKEditor 5 online builder.",
"version": "0.0.1",
"license": "SEE LICENSE IN LICENSE.md",
"private": true,
"devDependencies": {
"@ckeditor/ckeditor5-adapter-ckfinder": "^17.0.0",
"@ckeditor/ckeditor5-alignment": "^17.0.0",
"@ckeditor/ckeditor5-autoformat": "^17.0.0",
"@ckeditor/ckeditor5-autosave": "^17.0.0",
"@ckeditor/ckeditor5-basic-styles": "^17.0.0",
"@ckeditor/ckeditor5-block-quote": "^17.0.0",
"@ckeditor/ckeditor5-ckfinder": "^17.0.0",
"@ckeditor/ckeditor5-dev-utils": "^12.0.9",
"@ckeditor/ckeditor5-dev-webpack-plugin": "^8.0.9",
"@ckeditor/ckeditor5-editor-classic": "^17.0.0",
"@ckeditor/ckeditor5-essentials": "^17.0.0",
"@ckeditor/ckeditor5-heading": "^17.0.0",
"@ckeditor/ckeditor5-image": "^17.0.0",
"@ckeditor/ckeditor5-indent": "^17.0.0",
"@ckeditor/ckeditor5-link": "^17.0.0",
"@ckeditor/ckeditor5-list": "^17.0.0",
"@ckeditor/ckeditor5-media-embed": "^17.0.0",
"@ckeditor/ckeditor5-paragraph": "^17.0.0",
"@ckeditor/ckeditor5-paste-from-office": "^17.0.0",
"@ckeditor/ckeditor5-table": "^17.0.0",
"@ckeditor/ckeditor5-theme-lark": "^17.0.0",
"@ckeditor/ckeditor5-word-count": "^17.0.0",
"postcss-loader": "^3.0.0",
"raw-loader": "^3.1.0",
"style-loader": "^1.1.3",
"terser-webpack-plugin": "^2.3.5",
"webpack": "^4.41.6",
"webpack-cli": "^3.3.11"
},
"scripts": {
"build": "webpack --mode production"
}
}

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

FilipTokarski commented 4 years ago

Hi, thanks for the report. Yes, it seems something is wrong. I checked it and I get the following results:

if I set rel="noopener noreferrer" getData() returns:
<a target="_blank" rel="noopener noreferrer" href="https://www.google.com">test link</a>
if I set rel="noopener noreferrer nofollow" I get:
<a target="_blank" rel="noopener noreferrer" href="https://www.google.com"><a rel="noopener noreferrer nofollow">test link</a></a>
if I set only rel="nofollow" I get:
<a target="_blank" rel="noopener noreferrer" href="https://www.google.com"><a rel="nofollow">test link</a></a>

cc @Reinmar

Reinmar commented 4 years ago

The issue comes from the conflict between addTargetToExternalLinks which sets rel="noopener noreferrer" and the other manual option which sets rel="nofollow". The treats rel as a string, not an array of options. It is not capable of merging multiple rel attributes. Hence, when two links with the same attribute (rel) are applied to one fragment of text, the result is completely broken. It could be better definitely, but you won't be able to achieve a correct result at the moment. We'd need to make quite big changes in how the engine treats attributes for this to work or workaround this in the link decorators implementation which would be ugly.

Reinmar commented 4 years ago

The easiest workaround I can see is doing the "add target to external links" completely manually either during the conversion (downcast) or outside the editor.

airstarh commented 4 years ago

Colleagues, sorry, but this plugin does not make image-links EXTERNAL. I see the switcher, I switch image-link to _blank, but it is not saved. I tested at least for image, which I paste from other site.

Reinmar commented 4 years ago

@airstarh, could you report a new ticket for this? This sounds like a separate bug.

airstarh commented 4 years ago

@Reinmar thank you for the response, Sir! I've just created the issue here, I create issue here at the 1st time, so, excuse me if I do something wrong. https://github.com/ckeditor/ckeditor5/issues/7519

mmichaelis commented 3 years ago

As #8230 is referring to this: We just have the very same issue and at the current point of research this is (almost) a blocker. Due to a "matured" system we must provide even several target-options to choose from like: "open in new window", "open in current window", ... and even having completely custom target attributes which can be entered by editors.

As stated in #8230 this ends up in "piling up" meaningless anchor tags (only having the target attribute set) and of course, while using toggles as given from the examples, it is bad, that you have no exclusive behavior, so that you can toggle only one of them.

Here is one output of an experiment with multiple decorators all setting the target attribute:

<a href="https://example.org" target="_blank"><a target="_top"><a target="embed">example </a></a></a>

We are in need of some solution very soon - so any hints, any workarounds would be welcome. And if possible, it would be great if to provide a workaround/solution which will work with the Links plugin rather than having an independent plugin.

piernik commented 8 months ago

4 years and still not fixed! I have to admit that HTML is not Your priority :|

ckeditor / ckeditor5