Fix: prevent duplicate anchors in text with styles

[KarolDawidziuk]

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

• Unit tests
• Manual tests

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

• PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4728](https://github.com/ckeditor/ckeditor4/issues/4728): Fixed: [Link](https://ckeditor.com/cke4/addon/link) Multiple anchors in one line and multi-line with text style.
* [#3863](https://github.com/ckeditor/ckeditor4/issues/3863): Fixed: [Link](https://ckeditor.com/cke4/addon/link) Multiple anchors in single word with text style.

What changes did you make?

The main problem was the text with styles. ex. bolded word.
I've created a function that search in selected range a existing or nested anchors and clean it to prevent doubled it.

Which issues does your PR resolve?

Closes #4728,
Closes #3863

[f1ames]

Looking good 👍

Since this PR fixes multiple anchor issues it would be good to provide kind of generic manual unit test where one can experiment with anchors. You could go with heavily styled content and simple steps to create an anchor over selection having style text inside.

I recall we talked F2F about issue with adding anchor to multiline selection - have you extracted this to a separate issue already because it's not covered here from what I see?

[KarolDawidziuk]

I added more unit tests and created one general manual test for more specific tests.

"I recall we talked F2F about issue with adding anchor to multiline selection - have you extracted this to a separate issue already because it's not covered here from what I see?"
Yes, but this problem is with non-standard text styles, so I figured we wouldn't need such a test. I added such a situation in one of the tests for greater coverage :)

[github-actions]

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

[sculpt0r]

Rebased on newest master.

[sculpt0r]

I resolved most of the inline comments from @f1ames since you correct them.

However, I found that still one of them has no response: #4733 (comment)

Also, I'm not sure what exactly you were talking about, but from this comment:
#4733 (comment) I concluded that we should have a separate issue for anchors applied to multiline selection? Could you explain it a little bit more? I see one unit test for that case for now.

I also was able to fail during the manual test you provided with such scenarios:

• Scenario one

  • select ading styl from the sentence Test heading styled words
  • add anchor with name e.g. 'a'
  • we have two anchors in text with the same id which is not the correct situation, but we can extract it to another issue eventually
  • also we have two anchors, while when we select any two non-styled words, it's only one anchor for both of them

• Scenario two

  • select ading styl from the sentence Test heading styled words
  • add anchor with name e.g. 'a'
  • select heading styled from the sentence Test heading styled words
  • we have three anchors: one for heading, styl and ed - we should have one anchor for heading styled. That is the behaviour while you try to do a similar thing on non-styled words

  Summary:

  • please add at least two more cases to verify that my scenarios are correct, then try to find the solution for that. Ask me anytime if you need some more explanations.
  • please try to find any non-standard selections that are working on the non-styled text, but fails with styled text. Make a test case for each unique case. If you think that some of them are not covering the original issue, we can think about extracting them as separate issue.
  • please explain to me the situation with multiline selection, because I also find a scenario when such selection fails
  • please add a test with case that is described by @f1ames previously

[KarolDawidziuk]

Thanks @sculpt0r for detailed review and bug cases.

Also, I'm not sure what exactly you were talking about, but from this comment:
#4733 (comment) I concluded that we should have a separate issue for anchors applied to multiline selection?

I mean, the problem with multi-line selection exists, but this PR may fix this without creating a new one.

This changes included:

• Adding one more test case described by @flames
• Fix: remove more nested anchors into the range.

Referring to the your failing test cases, I think we should create a new issue for this. This problem with adding anchors to partially selected styled text is a little different as the adding anchors in part does not work in the core CKEDITOR.style.applyToRange() function and I think that it could be a more complex topic.

I was find one partially solution to this problem but I want to know what you think about this.

Steps:

1. Extract actual selected range and save it to future restoring.
2. Remove the custom styles from selected range. Then the, `applyToRange()` method can correctly add the anchor to selected range without custom styles.
3. Append selected `document.fragment` version from step 1. 

This solution works for me but.. There is one bigger issue when I select a multiline text. After that, the all content is removed and moved to the end of the editable. I think I know what the problem is, but I wondering if this solution will be to hacky or maybe there is easier way to handle it.

[github-actions]

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

[github-actions]

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

[github-actions]

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

[sculpt0r]

To finalize topics with bugs, let's extract three separate issues for the anchor plugin:

• related to partially selected multi-styled, inline words. I describe it a little bit earlier, but please adjust the steps that they will be more useful in reproduction without the manual test introduced here.
• related to having two id with the same value for the separate anchors. We probably should provide uniq id in html that is generated by the editor.
• related to selection multi lines

They will be easiest to review and solve as separate issues.

[sculpt0r]

Good job 👍

Just some cleanup stuff related to extracting more issues:

• please see the previous comment: Fix: prevent duplicate anchors in text with styles #4733 (review)
• I think we can convert the "monkey-clicking" manual test into a test case that covers Anchor is duplicated when edited #3863. So please adjust the scenario and editor content. Please take a look at the indentation in html file, it looks like there are spaces instead of tabs.

Could you also rebase on master? There are some conflicts - seems that mostly with comments...

[sculpt0r]

Thank you for the corrects.

I left a few inline comments - only polishing - so 👍

As for the manual test: I somehow missed the fact that we have two cases here. I also added some inline comments there for general information, but what is more important:

• please split this test into two separate manual tests for each scenario and name those files accordingly.

It is best if our test can fail due to only one reason. It is also described in our guide.

[KarolDawidziuk]

Thanks for review!
It's ready for the another.

At the time when we decided to split it up this issue into another three, I removed the manual playground test for anchor testing and added test cases to cover issues mentioned in #4728 and #3863. Also I added custom message when test fails.

[sculpt0r]

Rebased on master

[sculpt0r]

LGTM 🎉
After rebase I cannot find what was wrong with your rebasing 🤔
We will try to investigate it in another case...

At this point it looks good :) Good job! 👍

[sculpt0r]

@f1ames we need your approve. Could you take a look?

@sculpt0r took over the review.

[f1ames]

@sculpt0r I just dismissed my review as you took it over. Feel free to proceed with merging this PR.