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

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

Closed
peppies opened this issue Mar 16, 2020 · 8 comments · Fixed by #17461
Closed

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

peppies opened this issue Mar 16, 2020 · 8 comments · Fixed by #17461
Labels
package:engine package:link squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@peppies
Copy link

peppies commented Mar 16, 2020

📝 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.

@peppies peppies added the type:bug This issue reports a buggy (incorrect) behavior. label Mar 16, 2020
@FilipTokarski
Copy link
Member

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
Copy link
Member

Reinmar commented Apr 6, 2020

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 Reinmar added this to the backlog milestone Apr 6, 2020
@Reinmar
Copy link
Member

Reinmar commented Apr 6, 2020

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
Copy link

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
Copy link
Member

Reinmar commented Jun 30, 2020

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

@airstarh
Copy link

airstarh commented Jun 30, 2020

@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. #7519

@mmichaelis
Copy link

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.

@pomek pomek removed this from the backlog milestone Feb 21, 2022
@FilipTokarski FilipTokarski added the squad:core Issue to be handled by the Core team. label Jan 25, 2023
@piernik
Copy link

piernik commented Feb 13, 2024

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

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