ID's are stripped from links

Created on 26 April 2024, 7 months ago
Updated 9 September 2024, 2 months ago

Problem/Motivation

When the Anchor button is enabled on our CKEditor5 editor, IDs disappear from links when editing a node.

Steps to reproduce

Note that at no point in discovering and reproducing this issue were we actually *using* the anchor button -- it was simply enabled on the editor's toolbar.

I created a basic page node with simple text reading "This is a test." I tried two different ways to create the desired link -- using the source editor, and using the Advanced Link functionality with the link button to add the URL and an ID. Both ways, I ended up with simply:

<a href="https://drupal.org" id="link-id-test">test</a>

I saved the link and confirmed that it was working and had the ID on it.

I then edited the node again. Checking either the source editor or the link settings in the link dialog both revealed that the ID was gone -- not just empty, the attribute had been removed entirely. The src attribute was still present.

The problem occurs while loading the editor -- if I don't save the node, the ID attribute is still present. If I re-add it and save, it's present until I edit again.

If I remove the Anchor icon from the toolbar, this problem stops happening. (Doesn't matter if I actually uninstall the module.)

Proposed resolution

Remaining tasks

  • ✅ File an issue
  • ➖ Addition/Change/Update/Fix
  • ➖ Testing to ensure no regression
  • ➖ Automated unit testing coverage
  • ➖ Automated functional testing coverage
  • ➖ UX/UI designer responsibilities
  • ➖ Readability
  • ➖ Accessibility
  • ➖ Performance
  • ➖ Security
  • ➖ Documentation
  • ➖ Code review by maintainers
  • ➖ Full testing and approval
  • ➖ Credit contributors
  • ➖ Review with the product owner
  • ➖ Release notes snippet
  • ❌ Release

API changes

  • N/A

Data model changes

  • N/A

Release notes snippet

  • N/A
🐛 Bug report
Status

Needs work

Version

3.0

Component

Code

Created by

🇺🇸United States wrd-oaitsd

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @wrd-oaitsd
  • 🇺🇸United States justcaldwell Austin, Texas

    I can confirm this behavior. We use Editor Advanced Link (2.2.4) to allow editors to add ARIA-label, classes and ID to links.

    After Anchor Link is added to the toolbar, any previously-added ID attributes are removed from link tags when the entity is next edited. As noted, no actual use of Anchor Link is necessary. ARIA-label and class attributes are not affected.

  • 🇺🇸United States justcaldwell Austin, Texas

    This would need to be resolved upstream on northernco/ckeditor5-anchor-drupal, but there's nowhere to open an issue there.

  • 🇺🇸United States justcaldwell Austin, Texas

    Here's an example where I created a link (with an href), then selected the entire link and made it into an anchor, which sets the ID attribute and adds the ck-anchor class. You can see below that the UI indicates that both are in effect (both the link and anchor button are "active").

    On initial save, the ID remains intact in the rendered page. If I make any subsequent change to the page and save it, the ID is stripped and the anchor is silently broken.

    To add to potential confusion, if I edit the page again, the anchor flag icon still appears (because the ck-anchor class was not stripped) so the anchor still appears to be in tact.

  • 🇺🇸United States justcaldwell Austin, Texas

    I think this is at least major, as we're losing user input/data.

  • 🇪🇨Ecuador jwilson3

    I would say this is actually more like Critical since it results in data loss.

    https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

  • Hi,
    Below lines of code($plugin_definition['ckeditor5']['config']['htmlSupport']['disallow'][] ) stripping the link and other attributes.

    function anchor_link_ckeditor5_plugin_info_alter(array &$plugin_definitions): void {
      $plugins_to_override = [
        'ckeditor5_arbitraryHtmlSupport',
      ];
      foreach ($plugins_to_override as $plugin_id) {
        if (!isset($plugin_definitions[$plugin_id])) {
          return;
        }
        $plugin_definition = $plugin_definitions[$plugin_id]->toArray();
        // Make plugin-specific alterations. Disallow the General HTML Support
        // plugin from controlling links with the attributes handled by the
        // Anchor plugin.
        $plugin_definition['ckeditor5']['config']['htmlSupport']['disallow'][] = [
          'name' => 'a',
          'attributes' => [
            'id',
            'name',
          ],
          'classes' => [
            'ck-anchor',
          ],
        ];
        // Update plugin definitions.
        $plugin_definitions[$plugin_id] = new CKEditor5PluginDefinition($plugin_definition);
      }
    }
    
  • Assigned to tagpy
  • First commit to issue fork.
  • 🇪🇨Ecuador jwilson3

    I'm confused whether @tagpy should still be assigned given @bedlam created the issue fork.

    Am anxious to review this if possible, I see a first commit, but don't see any MR yet.

    @bedlam what is the status here?

  • 🇺🇸United States justcaldwell Austin, Texas

    FWIW, I don't think the code referenced in #7 plays a role in this issue. Per the comment in that code...

        // Make plugin-specific alterations. Disallow the General HTML Support
        // plugin from controlling links with the attributes handled by the
        // Anchor plugin.
    

    ...it's just preventing ckeditor's own General HTML Support plugin from acting on links that anchor_link wants to handle. That is, <a> elements with id and/or name attributes and the ck-anchor class. It's not actually stripping any attributes.

  • 🇺🇸United States justcaldwell Austin, Texas

    Okay, I think this is where at least part of the problem begins in the ckeditor5-anchor-drupal plugin. In that upcast function, if the <a> tag has an href, the upcast bails out early (lines 134-136):

    if (viewElement.hasAttribute('href')) {
        return;
    }

    (The code above was explicitly added to the drupal version of the anchor plugin in this commit.)

    When I comment out those lines and rebuild the plugin, IDs are no longer stripped from existing links.

    BUT, that introduces a new issue. Since anchor_link is handling any <a> with the ck-anchor class, when you place the cursor in the link text, the anchor_text UI popup appears — the link UI popup is no longer available.

    So editors would no longer be able to edit the href in the usual way, only the anchor (i.e. the ID).

    At this point I'm not sure if anchor link can be made to work on links with hrefs, but it seems like it needs some logic to prevent editors from turning said links into anchors.

    And, obviously, it shouldn't strip IDs from links in exisiting content.

  • 🇺🇸United States justcaldwell Austin, Texas

    Last observation for today: I noticed IDs were striped in my clean install (Drupal 10.2.6) even after I removed anchor_link from my text format/editor. So, I tried to add ID support by adding <a hreflang id> to Source Editing, and Drupal gave me an error telling me to enable anchor link to support IDs!

    Error Message: The following attribute(s) are already supported by available plugins and should not be added to the Source Editing "Manually editable HTML tags" field. Instead, enable the following plugins to support these attributes: Anchor link (<a id>).

    So this likely also related to 🐛 [10.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified Needs review .

    Basically, anchor_link is "adding support" for IDs in <a> tags (IDs aren't allowed by default), but it bails out on processing when it finds an href (see above), so the ID never makes it back into the content. AND, as long as anchor_link is installed/enabled in the project at all, you can't add support for IDs via Source Editing. Yeesh. 🤦🏻‍♂️

  • 🇺🇸United States sacevedo87

    Following up in regards to the ID stripping issue. Will there be a patch soon? The issue did not seem to exist in the prior version (2.7) but this version is not compatible with CKEditor 5 only with CKEditor 4. Hoping there is a fix soon.

  • Assigned to justcaldwell
  • 🇺🇸United States justcaldwell Austin, Texas

    Taking another crack at this...

  • 🇯🇴Jordan Rajab Natshah Jordan

    Facing similar issues with other modules and CKEditor 5 under Drupal 10.3

    Do you have Limit allowed HTML tags and correct faulty HTML enabled in the text format?

    And Allowed HTML tags

    With CKEditor 5 this is a read-only field. The allowed HTML tags and attributes are determined by the CKEditor 5 configuration. Manually removing tags would break enabled functionality, and any manually added tags would be removed by CKEditor 5 on render.

    I see <a id target rel class="ck-anchor" href title data-entity-type data-entity-uuid data-entity-substitution> in my Allowed HTML tags

    NOTE: I do not use the Limit allowed HTML tags and correct faulty HTML in my case.

  • 🇺🇸United States justcaldwell Austin, Texas

    Hi @rajab natshah. I've been testing with CKEditor 5 and a clean instance of Drupal 10.3.2, with the default text formats Basic HTML and Full HTML. The only change I've made to either is to enable anchor_link (i.e., add the anchor button to the toolbar).

    Basic HTML has Limit allowed HTML tags and correct faulty HTML enabled, and has <a id target rel class="ck-anchor" href title data-entity-type data-entity-uuid data-entity-substitution> in Allowed HTML tags (same as yours). Limit allowed HTML is not enabled for Full HTML.

    In both cases, IDs survive the initial node creation, but are stripped on subsequent edits.

  • 🇺🇸United States justcaldwell Austin, Texas

    The Problem

    I think I fully understand what's happening now:

    1) anchor_link adds support for anchor id attributes (among others) to Allowed HTML tags in anchor_link.ckeditor5.yml.

    2) anchor_link_ckeditor5_plugin_info_alter() tells CKEditor's core HTMLSupport to NOT handle any <a> with an id or name attribute, because anchor_link will act on these elements. (See @tagpy's comment #7)

    3) BUT the anchor_link plugin was modified with the code below to stop processing if the <a> has an href attribute. (See comment #12)

    if (viewElement.hasAttribute('href')) {
        return;
    }

    If you remove that code, id attributes are no longer stripped, but you get the suboptimal UI that doesn't allow for editing the link in context (see screenshot from #12 ).

    That's where I gave up before.

    Proposed Solution

    We should remove the code above to re-enable processing of anchors with href attributes AND add the linkUI button to the anchor_link balloon toolbar IF the anchor also has an href.

    Here's a quick demo of what that looks like, with a standard link and a plain anchor (no href) for comparison:

    I forked @northernco/ckeditor5-anchor-drupal, and the implementation for this is in the link-id-support branch.

    Review and Testing

    There won't be an MR or patch here, as all the changes need to occur in the @northernco/ckeditor5-anchor-drupal plugin. I'll submit a pull request there soon, and I'll set this issue to Needs Review when that's done.

    If you're in a position to build/compile ckeditor 5 plugins (with npm/yarn/webpack etc.), then feel free to clone my issue fork to your /libraries directory and give it a try.

    I may publish a package to make this easier to test.

  • 🇺🇸United States justcaldwell Austin, Texas

    Here's the pull request.

  • Issue was unassigned.
  • Status changed to Needs review 3 months ago
  • 🇺🇸United States justcaldwell Austin, Texas
  • 🇺🇸United States justcaldwell Austin, Texas

    A package with the changes described in #18 has been published to https://asset-packagist.org to make it easier to test.

    Just follow the OPTION #2 instructions for installing the library on the project page , being sure to replace npm-asset/northernco--ckeditor5-anchor-drupal anywhere you see it with npm-asset/justcaldwell--ckeditor5-anchor-drupal.

  • 🇯🇴Jordan Rajab Natshah Jordan

    Nice work, Michael.

    Thank you :)
    You have full credit for this fix!

  • 🇯🇴Jordan Rajab Natshah Jordan

    This is a good way of not attaching a static library to a module
    Allowing developers to bring any library or any forked improved library, allows the module to do more.

  • 🇺🇸United States justcaldwell Austin, Texas

    Thanks, Rajab! And agreed -- I really like the asset packagist approach. Your edits to the project page make the process much more reliable.

  • Status changed to Needs work 3 months ago
  • 🇺🇸United States justcaldwell Austin, Texas

    I discovered a minor (IMO) bug in this implementation. If you follow these steps:

    1. Select some text
    2. Click the anchor button and add the anchor id
    3. Click the link button and add a link href
    4. Click in the text and click the 'unanchor' button

    The anchor/id is removed as expected, but subsequent clicks on the link text will continue to show the anchor actions balloon toolbar. It should display the link toolbar, since the href is still present.

    The order matters. If you add the link/href before you make it an anchor, everything performs as expected. This only occurs before the content is saved. Editing existing content doesn't trigger the issue, unless you're adding a new linked anchor.

    I'll try to dig into this next week.

  • 🇫🇮Finland masipila

    Thank you so much @justcaldwell, you made my day!

    Here are some remarks for other community members who have previously used the npm-asset method to manage their dependencies and are using `npm-asset/northernco--ckeditor5-anchor-drupal`

    1. composer.json has two references that need to be updated

    • `require` section
    • `extra / installer paths` section

    2. Note that the versions of `npm-asset/justcaldwell--ckeditor5-anchor-drupal` are not matching to the version numbers of `npm-asset/northernco--ckeditor5-anchor-drupal`. So make sure to check the latest version from https://www.npmjs.com/package/@justcaldwell/ckeditor5-anchor-drupal

    3. If you modify composer.json manually to swap `npm-asset/northernco--ckeditor5-anchor-drupal` to `npm-asset/justcaldwell--ckeditor5-anchor-drupal`, you need to run `composer update drupal/anchor_link --with-all-dependencies` because your composer.lock still has reference to `npm-asset/northernco--ckeditor5-anchor-drupal`. The composer update command will remove the `npm-asset/northernco--ckeditor5-anchor-drupal` and download `npm-asset/justcaldwell--ckeditor5-anchor-drupal`

    Cheers,
    Markus

  • 🇺🇸United States justcaldwell Austin, Texas

    Glad to help @masiplia, and thanks for the notes!

    I haven't been able to work on the bug I identified in #25 🐛 ID's are stripped from links Needs work , but I wanted to mention to everyone that I just found out that anchor support is finally coming to core CKEditor.

    Per this comment on the github issue, work began a couple weeks ago. You can comment on the issue to cast a vote on the UI. While you're there add a 👍 to the issue summary to express your support for the feature request.

  • 🇺🇸United States justcaldwell Austin, Texas

    I've had no luck addressing #25. Hopefully this won't be an issue when "anchors" land in a ckeditor 5 core package. Here's my best guess at what is happening should someone else take this up.

    The element created by createAnchorElement() is assigned a priority of 5 — the same priority used by CKEditor5's link plugin. Since both plugins operate on <a> elements, I assume the priority must match to prevent creation of additional elements. Indeed, if you change the priority value used in createAnchorElement to e.g. 4, the resulting markup will have two <a> tags when both id and href are added in the editor — one with the id, one with the href.

    My guess is since both plugins share the same priority, the first plugin invoked wins when setting a custom property on the element ('link' or 'anchor'). These custom properties play a role in deciding what toolbar/UI to display. So, we get different behavior depending on whether a range of text was made into a link or an anchor first.

  • 🇺🇸United States justcaldwell Austin, Texas

    Just noting that the CK Editor developers have opened an issue to track initial development of native anchor support — called "Bookmarks" — at Bookmarks: part 1 · Issue #17063 · ckeditor/ckeditor5.

    Sounds like, at least initially, the UI will only create what they call "point-only-bookmarks", e.g. <a id="xyz"></a>. So the native plugin may not allow for actual links that have an href and link text to be edited using the new Bookmark UI.

  • 🇺🇸United States brockfanning

    We're suffering from this exact problem, and I'm hoping to get in a quick fix to stem the bleeding of the data loss. Does anyone have any guidance on how to apply justcaldwell's fix (thank you!!) for existing sites that went with "Option 1" on the install instructions for this project?

  • 🇺🇸United States justcaldwell Austin, Texas

    I haven't actually tried this, but I think a "quick-and-dirty" approach would be to just manually replace the northernco version of the plugin in your /libraries folder:

    - Visit https://asset-packagist.org/package/npm-asset/justcaldwell--ckeditor5-an..., and click 'Get Zip'
    - Decompress the zip file, and rename the resulting 'package' directory to 'ckeditor5-anchor-drupal'
    - Replace /libraries/ckeditor5-anchor-drupal with the directory above

    I assume this would get overwritten next time you update anchor_link with composer, so you'd probably want to re-install anchor_link using the modified "Option 2" outlined in #21 and #26 above when possible.

  • 🇺🇸United States justcaldwell Austin, Texas

    Just wanted to note that, when last I checked, ckeditor's upcoming 'Bookmarks' plugin will not support editing/creating bookmarks (anchors) on links.

    I also don't see a way to resolve the edge case in #25. I think the changes so far are an improvement on stripping ids from links, so setting back to NR.

Production build 0.71.5 2024