Resolve Link plugin manual decorators quirks

lamuertepeluda commented 1 year ago

Context

I (am planning to) use the CKEditor5 as the editor of a headless CMS. Content editors need to create hyperlinks setting some attributes such as target and rel, whose values are especially important for SEO optimization.

To do so, I've configured some manual decorators of the LINK plugin following the plugin guide.

        link: {
            addTargetToExternalLinks: false,
            decorators: {
                newtab: {
                    mode: 'manual',
                    label: 'Open in a new tab',
                    attributes: {
                        target: '_blank',
                        rel: 'noopener noreferrer'
                    }
                },
                nofollow: {
                    mode: 'manual',
                    label: 'Set SEO No Follow',
                    attributes: {
                        rel: 'nofollow'
                    }
                },
                sponsored: {
                    mode: 'manual',
                    label: 'Set SEO Sponsored',
                    attributes: {
                        rel: 'sponsored'
                    }
                }
            }
        }

My aim is to allow content editors to (manually) set any of the above, in any possible combination (i.e. I assume they know what they want to do and give them full control over the link).

📝 Provide detailed reproduction steps (if any)

configure a set of manual decorators such as in the example above
create a link in the editor by selecting some text, clicking on the link button in the toolbar and pasting an URL, e.g. "https://ckeditor.com/"
enable 2+ switches, e.g. 'Open in a new tab' and 'Set SEO No Follow'

✔️ Expected result

Given the html

<p>Hello I am a link</p>

to which I link https://ckeditor.com/ and use the first 2 decorators:

I want

<p>Hello <a href="https://ckeditor.com/" target="_blank" rel="noopener noreferrer nofollow">I am a link</a></p>

as the editor downcast and downcast output.

❌ Actual result

Instead I get

<p>Hello <a href="https://ckeditor.com/" target="_blank" rel="noopener noreferrer"><a rel="nofollow">I am a link</a></a></p>

view_wrong

as both the editor downcast and downcast output. Which is wrong and results in a broken link. I guess this is the result of the lack of "conflict resolution mechanism" cited in the docs.

Also the model view clearly shows something's off...

model_wrong

In the figure above, extra link attributes are treated as boolean and thus cannot be applied to the html tag. Also their names are reflecting the name of the decorator instead of the name of the attribute.

❓ Possible solution

I have noticed the following things:

decorators, in the current implementation of the plugin, maintain a true/false map that follows the switch buttons of the UI
link attributes in the model must begin with link in order to be downcasted, e.g. linkHref, linkTarget, linkRel but the fact that they are treated as boolean does not work well, resulting in them not being set.
only linkHref is actually in the list of allowedAttributes. However the full list of standard attributes for the HTML <a> tag is available here. A comprehensive list of values for the the rel attribute can be obtained from this page and from the Google SEO optimization guide docs.

My solution involves:

modify the linkediting.ts file and
1. allow at least linkRel and linkTarget (should handle also linkPing, linkDownload etc) but they're out of my scope
2. change both downcast, editingDowncast and editingUpcast to use
  1. upcast: elementToAttribute() rel -> linkRel, target -> linkTarget, ...
  2. downcast, editingDowncast: attributeToElement(): linkRel -> rel, linkTarget -> target...
modify the utils.ts file to implement:
1. a set of helpers/validator for ensuring the target or the rel is valid
2. a set of helpers for enforcing naming conversion linkAttribute <-> attribute
3. a merge function that treats differently the "multivalue" attributes such as rel (or class) differently from the single-valued ones, such as href, target
  - multivalue attributes are rendered as space-separated values, but are merged following the order of the decorators and using split()/join().
modify the linkcommand.ts file
1. change the attribute add/remove logic from boolean to a reduce which uses the merge function cited above

I have created a custom plugin myself which implements the aforementioned solution by copying the Link package code and editing the logic. While this solution works and shows it is feasible, it still needs a bit more testing, so I didn't publish it yet. I aim to submit a pull request for the official Link plugin as well, which should make the above clearer, although I might need some time since the project is not really VSCode-friendly (e.g. linters, formatters) and the whole contribution experience is a bit complex.

The following are the screenshots of the results using my version of the link plugin

view_correct

model_correct

I'd also like to have some feedbacks about this issue, i.e. if I got something wrong in the plugin configuration.

📃 Other details

Browser: any
OS: any
First affected CKEditor version: 36.0.1 up to 37.1.0
Installed CKEditor plugins: no unofficial plugins

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

lamuertepeluda commented 1 year ago

Hi there, I tried to make a PR, however:

I cannot get coverage info locally (see screenshot. I have a mac M1, don't know if it relates). This is a bit frustrating but I have not so much experience with chai/mocha
I signed the CLA, however it still results as unsigned
Travis build failed https://app.travis-ci.com/github/ckeditor/ckeditor5/jobs/601063348

My 2 cents on the dev experience, which ain't smooth compared with other projects:

please provide a Docker Container (e.g. testcontainer-node?)
move also tests to TypeScript. They are currently js
maybe Jest is more "2023" and well-known as test library and supports TS just fine
include prettier or similar tool as formatter: editorconfig sometimes conflicts with your eslint settings
remove/separate eslint formatting rules, or at least make them compatible with the formatter
be VSCode friendly: it's the most used editor nowadays and supports a lot of features including formatters, debugger, test runner and linter

That said, I hope you can give me some advices for completing the PR :)

Witoso commented 1 year ago

Hey @lamuertepeluda, thanks for the contribution, and the feedback. We will get back to you as soon as possible.

Reinmar commented 1 year ago

I signed the CLA, however it still results as unsigned

@vokiel, could you take a look (#13988)?

I cannot get coverage info locally (see screenshot. I have a mac M1, don't know if it relates). This is a bit frustrating but I have not so much experience with chai/mocha

We use all kinds of setups and many of us use MacBooks with M1/M2. Did you try the steps described in https://ckeditor.com/docs/ckeditor5/latest/framework/contributing/development-environment.html? I've just checked them and they work fine:

Travis build failed https://app.travis-ci.com/github/ckeditor/ckeditor5/jobs/601063348

It failed because of the missing coverage in the link package. Once you'll get your coverage running you'll most likely know what's missing.

As for the feedback regarding the dev env: there's for sure room for improvement. Some smaller comments on unfeasible things:

move also tests to TypeScript. They are currently js

It's 300.000 SLOC in just this repo itself. We've got a massive test suit. Migration would take years.

What we want to enable is writing new tests in TS, though. But there's also a risk of the dev env getting slower due to that. We observed an over 2x increase in dev env compilation times when migrating to TypeScript (which we completed recently after 7 months of work).

maybe Jest is more "2023" and well-known as test library and supports TS just fine

Jest runs in Node, not in a browser (unless something has changed there). We rely on the DOM to an extent where moving out of the browser would weaken the test suit. We'd need to look for shifting bits of that to E2E tests or keeping some in the current form and some in Jest... in other words – that'd cause more harm than good for the project's learning curve.

As for the other things: definitely, something that we should look into.

vokiel commented 1 year ago

CLA confirmed, status updated.

Witoso commented 1 year ago

Curious also what you mean by:

be VSCode friendly: it's the most used editor nowadays and supports a lot of features including formatters, debugger, test runner and linter

almost the entire team works on VSCode 🤔

lamuertepeluda commented 1 year ago

Curious also what you mean by:

be VSCode friendly: it's the most used editor nowadays and supports a lot of features including formatters, debugger, test runner and linter

almost the entire team works on VSCode 🤔

ok that's nice to hear 🤗 then maybe sharing the workspace settings of .vscode/extensions.json and .vscode/settings.json could help. I mean, I had several conflicts between linter and formatter while editing the code, which is a bit annoying

lamuertepeluda commented 1 year ago

Jest runs in Node, not in a browser (unless something has changed there). We rely on the DOM to an extent where moving out of the browser would weaken the test suit. We'd need to look for shifting bits of that to E2E tests or keeping some in the current form and some in Jest... in other words – that'd cause more harm than good for the project's learning curve.

As for the other things: definitely, something that we should look into.

I've used this library https://www.npmjs.com/package/@testing-library/jest-dom in the past.
I didn't have to deal with such a massive test amount, and I acknowledge that achieving good test performances ain't easy and I am not able to estimate the impact of such a change on this and on your team (although several test function API are similar), nor the effort and priorities on this.

But I really appreciated moving the sources to TS, so having TS also for the tests would be just great.

lamuertepeluda commented 1 year ago

We use all kinds of setups and many of us use MacBooks with M1/M2. Did you try the steps described in https://ckeditor.com/docs/ckeditor5/latest/framework/contributing/development-environment.html? I've just checked them and they work fine:

Same command, different output (tried also without --production flag, I've added it just to get rid of the warning)

I have this setup:

Node: v18.12.1
Yarn: 1.22.19
Macbook Pro with M1 Pro and macOS Ventura 13.3.1

I noticed that the yarn.lock file is not in git tracking. This means everyone can potentially get different libraries when installing libraries using yarn install, doesn't it?

Reinmar commented 1 year ago

Could you try to rename the directory in which you are from ckeditor5-fork to ckeditor5? I remember this was significant at some point and it's the only bigger difference in the setup that I see now.

lamuertepeluda commented 1 year ago

Could you try to rename the directory in which you are from ckeditor5-fork to ckeditor5? I remember this was significant at some point and it's the only bigger difference in the setup that I see now.

yup! that worked :) thanks! so maybe there is some hardcoded path somewhere. Maybe it's good to add this in the dev/test setup docs

Reinmar commented 1 year ago

yup! that worked :) thanks! so maybe there is some hardcoded path somewhere. Maybe it's good to add this in the dev/test setup docs

@pomek, this sounds good, wdyt?

pomek commented 1 year ago

I believe the issue with paths and non-working the code coverage tool is already fixed. Please, see https://github.com/ckeditor/ckeditor5-dev/releases/tag/v38.2.0.

ckeditor / ckeditor5