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

Links with class attributes can create unexpected duplicate elements in regular editing #15051

Closed
paulkozik opened this issue Sep 22, 2023 · 5 comments · Fixed by #15373
Closed
Assignees
Labels
package:html-support package:link squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@paulkozik
Copy link

paulkozik commented Sep 22, 2023

I work with many anchor links with classes applied to them to give them the styling of buttons or other visual flair. These links have CSS classes applied to them to get this styling. I have found that in CKEditor5 that clicking at the rightmost or leftmost edge of these links creates a second link element in the editor, where it should be setting the cursor to the end of the link and adding any typed content into the existing link element. This can create a situation where the editor, under normal use, creates multiple different link elements, and can produce text that can appear as one link, but can in fact be 2 or 3 links with different URLs.

I have noticed that this issue does not apply to link without a class attribute. In the case of those the editor merely creates a text node outside of the link content, but does not duplicate the link element.

It's also of note that any links that have borders or padding applied to them are particularly affected by this issue.

Video of this occurring on a CKEditor demo page:
https://github.com/ckeditor/ckeditor5/assets/106848219/4aa7d723-7d56-42f4-a221-7ae7ff2348fd

📝 Provide detailed reproduction steps (if any)

  1. Create a link in the editor
  2. Go into source view and add a class attribute to it, any classes can be applied including just class=""
  3. Close the source view
  4. Click in the editor at the rightmost or leftmost edge of the link text
  5. Type anything into the editor

✔️ Expected result

Text to the right of link continues to be added to the current link element.

❌ Actual result

A second link element is created before or after the current link element and the new text is added to that element

📃 Other details

  • Browser: Firefox 117.0.1 and Chrome 117.0.59~
  • OS: MacOS Ventura 13.5.2
  • First affected CKEditor version: CKEditor 5
  • Installed CKEditor plugins: General HTML Support, Link, Source Code View
  • I've attached a short video of this being replicated on the stock CKEditor HTML support demo found here: https://ckeditor.com/ckeditor-5/demo/html-support/
  • A temporary "workaround" would be to have the editors always select the middle of the text and use keyboard keys to go to the beginning or end of the text, which does not trigger this issue. This dependence on keyboard may violate WCAG guidelines though and is definitely a far from ideal UX experience.
@paulkozik paulkozik added the type:bug This issue reports a buggy (incorrect) behavior. label Sep 22, 2023
@Witoso
Copy link
Member

Witoso commented Oct 10, 2023

Hi @paulkozik, sorry for the late reply, and thanks for the detailed report.

The demo you're using has a full GHS, when you add a class to the link we have two attributes on the text. The engine takes our internal data model and decides how to merge them into one tag. (I highly recommend the inspector)

The very interesting problem is that it only happens on click, the keyboard movements work ok. 🤔 We would need to investigate this. @niegowski do you know is there an open issue for this?

@niegowski
Copy link
Contributor

The very interesting problem is that it only happens on click, the keyboard movements work ok. 🤔 We would need to investigate this. @niegowski do you know is there an open issue for this?

I think this is caused by a custom click handler on the edge of the link:

model.change( writer => {
removeLinkAttributesFromSelection( writer, getLinkAttributesAllowedOnText( model.schema ) );
} );

It does not recognize GHS htmlA attribute and does not remove it from the selection.

On the left side, the 2-step caret movement is not integrated with GHS attributes.

@Witoso
Copy link
Member

Witoso commented Oct 11, 2023

@niegowski similar for backspace? #14683 (comment)

@wimleers
Copy link

Per @Witoso's comment at #14683 (comment), it looks like this may be part of the root cause for https://www.drupal.org/project/drupal/issues/3376167

@paulkozik
Copy link
Author

paulkozik commented Oct 13, 2023

Taking a closer look, this can be triggered in two different ways, either by clicking on the left or rightmost edge of the link, OR by clicking inside the link and hitting the left button until you reach the leftmost edge and then hitting left one more time.

I've attempted to try out the workaround from #14683 (comment) but it has not made a difference for this case in my testing. I can also note that in my local environment (using CKEditor V35, so admittedly a bit out of date), this issue occurs with target and rel attributes as well on links, but defining convertors for those attributes as so fixes that up:


editor.model.schema.extend( '$text', { allowAttributes: ['linkHref', 'linkId', 'linkClass', 'linkTarget', 'linkRel'] } );
editor.conversion.for( 'downcast' ).attributeToElement( {
        model: 'linkTarget',
        view: ( attributeValue, conversionApi ) => {
            return conversionApi.writer.createAttributeElement( 'a', { target: attributeValue }, { priority: 5 } );
        },
        converterPriority: 'low'
    } );

    editor.conversion.for( 'upcast' ).attributeToAttribute( {
        view: {
            name: 'a',
            key: 'target'
        },
        model: 'linkTarget',
        converterPriority: 'low'
    } );

However, attempting to setup a conversion for the class attribute does not resolve the issue:


editor.model.schema.extend( '$text', { allowAttributes: ['linkHref', 'linkId', 'linkClass', 'linkTarget', 'linkRel'] } );
editor.conversion.for( 'downcast' ).attributeToElement( {
        model: 'linkClass',
        view: ( attributeValue, conversionApi ) => {
            return conversionApi.writer.createAttributeElement( 'a', { class: attributeValue }, { priority: 5 } );
        },
        converterPriority: 'low'
    } );

    editor.conversion.for( 'upcast' ).attributeToAttribute( {
        view: {
            name: 'a',
            key: 'class'
        },
        model: {
          key: 'linkClass',
          value: viewElement => {
            return viewElement.getAttribute('class')
          }
        },
        converterPriority: 'low'
    } );

(Apologies for accidentally closing and reopening this issue, clicked the wrong button)

@paulkozik paulkozik reopened this Oct 13, 2023
@niegowski niegowski self-assigned this Nov 18, 2023
@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Nov 18, 2023
@niegowski niegowski added the squad:core Issue to be handled by the Core team. label Nov 20, 2023
arkflpc added a commit that referenced this issue Nov 22, 2023
Fix (html-support): The `DocumentSelection` should not store the GHS `linkA` attribute if the `linkHref` attribute was removed by the two-step caret movement feature. Closes #15051.

Other (link, typing): The logic behind the two-step caret movement extracted to the common code in the two-step caret movement feature.

Other (typing): Unified behavior of the `insertText` command for cases using the `DocumentSelection` and `Selection` as applied attributes behaved differently in those cases.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Nov 22, 2023
@CKEditorBot CKEditorBot added this to the iteration 69 milestone Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:html-support package:link squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants