Thanks for raising this!
Some steps to reproduce would be helpful here. Also, do we need an update/post update hook to clean up possible improperly stored values?
Previous patch looks like pretty obvious credit mining and @Pracheth never returned to correct the format. No credit.
Makes sense. Merged with a new release coming shortly.
Thanks for contributing!
justcaldwell โ made their first commit to this issueโs fork.
No problems with manual testing. Merged the minimal change and will tag a new release soon.
justcaldwell โ made their first commit to this issueโs fork.
Having a hard time replicating the scenario, but I can see how it happens. I suppose we could query for distinct values and populate the bundle filter with the results. Couldn't this be an issue for other fields as well?
This is a good idea. Are you thinking layout id should be on the current report somewhere, or would this be a new report?
justcaldwell โ created an issue.
Did some manual testing on D11. All seems fine. Merged to dev, release to follow.
justcaldwell โ made their first commit to this issueโs fork.
Thanks for the contribution! In general, I like the changes proposed here. I think there are some minor issues that need discussion/work:
- In the filters, what was labeled 'Block Type' is now 'Inline Block Type' and 'Block Type' now refers to other or reusable blocks, is that correct? This was confusing to me. Maybe 'Other Block Type' or 'Reusable Block Type' would be a better label here? I'm not sure.
- If I'm correct above, then those two filters are mutually exclusive, right? That can lead to further confusion. Maybe throw an error on validation, or add some help text or additional organization to the filter area?
- The 'Reset' button doesn't clear Inline Block Type if a value has been set.
Static patch of the current MR.
I opened PR 14 on northernco/ckeditor5-anchor-drupal in support of the MR here.
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.
As I indicate in the MR, this still requires minimal changes in northernco/ckeditor5-anchor-drupal plugin. I can open a PR there if there's any interest in this.
At this point, we're just using a custom build of the plugin that resolves this and a few other issues in the queue to try to ensure a smooth transition from cke4 -> cke5.
MR created as promised ๐
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.
Adding CKEditor Anchor Link โ , where it's currently a dependency of the 8.x-2.x branch, and might be added back to the 3.x branch (see: ๐ Support concurrent use of CKEditor 4 and 5 Active ).
Given no other input, I'm going to close this one. Thanks!
Here to eat my words ๐ฌ.
Given that there's still a large number of sites that haven't moved from CKEditor 4 (1.x and 2.x branch usage is still at ~18K), there may be a case for supporting an improved upgrade path that allows testing both CKE4 and CKE5 concurrently (see ๐ Support concurrent use of CKEditor 4 and 5 Active ).
This would require allowing the name
to continue to appear in the final source code. As I wrote in that issue:
The plugin currently upcasts anchors with
name
attributes, but does not include thename
attribute in the resulting page source. As a result, anchor links that have been edited in CKEditor 5 will no longer be present if the text format is subsequently switched back to one using CKEditor 4.
Even though name
is deprecated, it might be preferable to continue full support for now, and remove it in a subsequent release or branch.
I'm attaching a patch with the necessary changes. I'll open an issue branch/MR soon.
I opted to not reintroduce the hard dependency on the CKEditor FakeObjects โ module now, but to use these changes you would need to include it in your project:
composer require 'drupal/fakeobjects:^1.2'
As I said above, this is probably a good idea anyway if you're updating from 8.x-2.x, as composer may well remove the module (if it's not required elsewhere) without uninstalling it. FakeObjects can be uninstalled and removed when it's no longer needed.
The patch also corrects broken paths to the CKEditor 4 toolbar icons.
justcaldwell โ created an issue.
Seems reasonable to add support for other entities, but the current MR adds quite a bit of duplicate code. We should refactor to avoid that.
I'd call it minimally maintained. I somehow missed a couple of the recent issues entirely.
I'll plan to make a Drupal 11 release in the next few days, and probably include the RTBC issue.
@rob230 - this fix is actually included in the current release (3.0.0-beta1). The maintainers just haven't updated the issue status to Fixed.
Drupal.t()
will pass strings through \Drupal\Component\Utility\Html::escape(). Not sure if there's an easy way to allow HTML in javascript like this.
justcaldwell โ created an issue.
justcaldwell โ created an issue.
Agreed. No concerns from me.
justcaldwell โ changed the visibility of the branch 3477418-add-project-browser to hidden.
justcaldwell โ created an issue.
Manually tested with Drupal 11 โ works with all toolbar options.
justcaldwell โ made their first commit to this issueโs fork.
If I remember correctly, the CKEditor 4 version of anchor link adds both an id and name attribute to all anchors. Maybe you could select <a>
elements that have both attributes? For example:
a[id][name] {
/* your styles here */
}
Depending on other styles in play, you might have to use !important
to get them to work.
This assumes your text format/filter is allowing both attributes through to the rendered content
Yes, switching to 'Create label before first save' worked for us ๐ Pattern not being respected Active .
No time to debug right now, but my assumption is this stems from changes in ๐ Set Label runs two times on node creation Needs review .
We're seeing the same issue. Our pattern uses a single token that points to a text field on the same entity. That field allows some minimal formatting, but the generated labels are as described above (e.g. %AutoEntityLabel: 7a075eef-dde7-4c04-8f99-8f6a3f6e6177%
) whether or not the field values contains formatting.
Re-saving the node corrects the generated label. Switching to the new 'Create label before first save' setting also works in our case.
Since updating to 3.3, we're experiencing the issue described in ๐ Pattern not being respected Active . I haven't had time to confirm, but is suspect it might also be related to the changes made here (?).
A simple fix would be to remove the `id` attributes from the elements in the `icons/anchor.svg` file if they are not required for CSS or JavaScript.
They aren't needed for CSS or js. Absolutely a simple fix, but will have to happen in the plugin at northernco/ckeditor5-anchor-drupal. I opened a PR there:
3426731 remove duplicate ids by justcaldwell ยท Pull Request #13
justcaldwell โ created an issue.
I was able to test the PR and it corrects the problem. It's literally a one-character change, merging it feels almost like a no-brainer ๐ซฃ.
Agree with @vordude above โ it would be useful to chart a course to a stable release.
I'm also curious about plans for future development given that the CKE5 maintainers are now working on their own anchor ("Bookmarks") plugin.
Turns out there's already a PR on the plugin repo for this - https://github.com/northernco/ckeditor5-anchor-drupal/pull/11 from @james.williams โ ! Setting to NR based on that.
The change is minimal and makes sense to me, but I haven't tested it yet.
The overall utility of this functionality is still an open question (IMO), but maybe that belongs in a separate issue.
justcaldwell โ created an issue.
Just wanted to chime in to say that the 3.0 branch of anchor_links (specifically for CKEditor 5) only provides a UI for creating anchors โ i.e. <a>
elements with an id attribute.
Unlike previous branches that were for CKEditor 4, there is no UI for linking to anchors on the page, and there's no current indication that feature will be added to CKEditor plugin used by the 3.0 branch.
Editors should create links that target the anchors as described in step 2 of the issue summary.
(2) select the navigation text, click the standard 'link' icon, and manually enter '#myAnchor' as the link.
It's worth noting that the CKEditor 5 maintainers have started work on a native "Bookmark" plugin. Part 1 of that work is described in this issue on github. As noted there, and in this comment on a related issue, part 1 will add bookmark creation/editing UI โ the "bookmark linking experience" will occur in a subsequent development effort.
So it seems like the native plugin will provide that functionality at some point.
I am still unable to replicate this issue with a clean install of Drupal (now 10.3.5), BUT I did notice it was happening in our production site. I haven't figured out why โ maybe there's an interaction with some other plugin/config that's not present in the clean install (?).
At any rate, I tested with the change from the MR and the issues are resolved when creating new content (thank you!). Note that any existing content that was saved with the ck-anchor
class will still exhibit the problem, so that would still need to be sorted out.
Below are recordings of the before-and-after behavior. The issues are numbered according to the Observed Issues list in the issue summary. I should also note that I've only been able to trigger "issue #1" when the anchor is the first child of a given DOM node.
Before FixAfter Fix
Yeah, that seems reasonable. We're basically relying on Drupal's Html::cleanCssIdentifier to validate class names, but surprise (to me at least), it filters underscores by default.
Should be easy enough to refactor. I'll try to get something up soonish. Thanks for opening this!
This makes a ton of sense to me. ๐
Seems like it would help with some sticky issues in
๐
ID's are stripped from links
Needs work
, many of which arise from conflating "links" (<a>
with href) with "anchors" (<a>
with id).
The ckeditor maintainers have started working on a native editor plugin for anchors (they're calling them "Bookmarks"). Maybe consider proposing your idea in the related github issue?
Well, when you put it that way, it absolutely makes sense!
I updated the MR and did a bit of testing on my end. Definitely no detrimental effects, but the view returned appropriate results with or without getCacheTags. (I toggled the "Is microcontent" setting of an entity to test.)
Could be our view isn't getting cached in any case for some other reason.
I'll have to defer to stronger minds on that one. The API docs for getCacheTags()
read:
The cache tags associated with this object.
When this object is modified, these cache tags will be invalidated [emphasis mine].
I guess it's not clear to me what role a filter should have in invaliding cache tags. In this case, the list of node types.
Confirming that I tested in our environment and the fix restored previous performance. +1 RTBC.
LOL! All's well that ends well.
I just created ๐ Degraded query performance after "Is Micro-content" filter change Active . I'll create an MR using Dan's code in a bit.
justcaldwell โ created an issue.
That did the trick. I applied the patch and re-enabled the filter โ performance is back on par with the previous release. Thanks for the quick response @danflanagan8! ๐
This change seems to have significantly degraded view performance on our site with 14K+ content items.
After this update, the render time for our content admin view (100 items/page) went from a little over a second to over two minutes. Removing the "Is Micro-content" filter immediately restored previous performance.
I'm no expert in SQL queries, but maybe the updated where clause leads to issues at scale? I'll try to investigate more as time allows.
justcaldwell โ created an issue.
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.
The MR throws an error if the response is other than 'ok' and adds a .catch()
to alert the user. Static patch attached.
I tried to use ckeditor's Notification plugin, but doesn't seem like it was included Drupal's build of the editor. If this comment is correct, Notification doesn't provide any special UI and falls back to window.alert() itself.
Confirming this issue.
I agree that Option 1 would be the best UX, but Option 2 would be acceptable if implementation of Option 1 is overly difficult.
The above is based on the patch from #48, LinkIt 6.1.4 and Drupal 10.3.2.
Retracting my comment #49. Sorry for the confusion.
The LinkIt issue โจ Add better support for linking to anchors Postponed: needs info I referenced is only about ensuring LinkIt will work correctly when a fragment is appended to a link to another node (e.g. /node/5#foo).
This issue is augmenting the LinkIt dialog with a list of on-page anchors via the custom CKEditor Anchor link matcher (which would be nice to have!).
Unfortunately, in a quick test it doesn't quite seem to be working. When I begin a link with #, the 'Anchor links (within the same page)' list appears, but no anchors appear โ just a single blank entry. Screenshot below.
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.
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.
+1. Adding a static patch from the MR. Thanks!
I discovered a minor (IMO) bug in this implementation. If you follow these steps:
- Select some text
- Click the anchor button and add the anchor id
- Click the link button and add a link href
- 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.
It looks like โจ Add better support for linking to anchors Postponed: needs info in LinkIt's issue queue is getting close. Should this be closed as "won't fix" in favor of focusing efforts and testing there?
Hello! I'm unable to replicate this and don't see anything in anchor_link that would change the case of user-entered id values.
Your screenshot ( this.png โ ) shows how LinkIt is displaying the anchor ID. If you inspect the page source, has the id actually been converted to lower-case there?
This seems to be working well โ setting to RTBC. The library change for this issue actually shipped in the latest tagged release (3.0.0-beta1). So should it actually be marked as Fixed?
I also tweaked the issue summary to further clarify that name attributes are supported, but won't appear in the markup for new or edited content.
Regarding legacy content with only name attributes...If you have existing content with name attribute in the text, and you want to avoid big update path processing these texts, that needs to be converted to id attribute.
Though name is deprecated, browsers tend to support obsolete markup for a very long time. I suspect that browsers will support "named anchors" into the foreseeable future. If that's the case, I'm not sure there's any requirement to update older content to convert names to ids (at least not for the purposes of this issue).
Thanks, Rajab! And agreed -- I really like the asset packagist approach. Your edits to the project page make the process much more reliable.
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
.
Here's the pull request.
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.
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.
Taking another crack at this...
Thank you!
14 days have passed with no response.
MR prevents a fatal error in these edge cases. Static patch attached.
I didn't know there could be a difference between the module name and info file, but that seems to be a thing. In general, Composer Deploy should probably just bail out in edge cases like this (and maybe log the exception). Something like:
try {
$projectData = \Drupal::service('extension.list.' . $project_type)->getExtensionInfo($variables['project']['name']);
} catch (Exception $e) {
// Log the exception somewhere?
return;
}
We've run into a related issue with CKEditor 5 Plugin Pack โ . The Plugin Pack depends on code from CKEditor 5 Premium Features โ , which must be present in the codebase, but isn't (usually) enabled/installed as far as Drupal is concerned.
In this setup CKEditor 5 Premium Features actually appears in the list of installed modules on the Update Report (even though it's not installed โ weird, I know). When Composer Deploy comes along, it fails because the call to $projectData = \Drupal::service('extension.list.' . $project_type)->getExtensionInfo($variables['project']['name']);
throws and exception for ckeditor5_premium_features, since it's not actually installed.