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

LinkCommand should provide extension point for custom text attributes (linkTarget or similar) #9730

Open
mmichaelis opened this issue May 19, 2021 · 9 comments
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:integration-dx This issue reports a problem with the developer experience when integrating CKEditor into a system. package:link type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@mmichaelis
Copy link

📝 Provide a description of the improvement

The only extension point of LinkCommand up to now for custom attributes are manual and automatic decorators. But they only provide boolean logic rather than the possibility to add for example text attributes.

What is required is an extension point to string attributes stored in model, like for example an attribute linkTarget which may take any values like (you guessed it) _top, _blank or any string for named targets.

Trying to implement such a feature is not really feasible, especially as LinkCommand may change the document selection during execution.

Efforts so far

  • I managed to add a LabeledFieldView and append it after urlInputView.
  • For UnlinkCommand I added a post-fixer listening to removal of linkHref attribute, which automatically applies removing linkTarget attribute. This works fine and does not provide any hassle with undo/redo.
  • For LinkCommand I tried a post-fixer approach as well, by having a "fake" command bound to the submit-button, which stores the selected target attribute from UI and forwards it to a post-fixer.
  • The post-fixer will then see, if there got any new linkHref attribute got added, where a linkTarget attribute should be added as well.

This is feasible as long as the linkHref attribute got changed. But it fails for example, as soon as only the target attribute needs to be changed. In this case the setAttribute triggered within LinkCommand does not record any changes (as there are none, obviously). So, there is nothing to fix afterwards.

I skipped the idea to listen to LinkCommand.execute as LinkCommand itself may change the document selection (it jumps to the end on collapsed selection for example). And it most likely would create additional undo-records where should be only one.

For now I will continue this path, which may require to duplicate some code from LinkCommand for certain scenarios.

Suggested API: Events

⚡ set:attributes

Plural, as also the manual decorators may have set attributes.

Event Data:

  • writer - used to apply additional changes
  • range - used to adapt the correct range as for linkHref; possibly an array of ranges, so that we don't need to fire events with the for-loop applying linkHref to several ranges.
  • attributes (optional) - not required for my use-case, but it may come handy to have access to the attributes just set

This would be fired, for uncollapsed selection or for collapsed selection, if the current selection has a linkHref set, which is about to be changed.

There is one special case, which needs extra effort: If selection is collapsed, but has no linkHref set, the linkHref will be written as text to the model. In this case, we either need a way of adding attributes to the selection or the event needs to be fired as well, but a range has to be explicitly created. The latter one may be the best choice, as the existing listener could just be re-used.

It is important, that this event is fired right before the selection adjustment triggered in some paths within LinkCommand.

Implementors then only should take care in the selection.collapsed state, to also call writer.removeSelectionAttribute, so that when continuing typing, this custom attribute is not added to the following text.

Possible Clash with Manual Decorators

At least in the given example, the approach may clash with manual decorators adding a target attribute as well. For my approach I will just document, that you must not use them.

📃 Other details

  • Browser: any
  • OS: any
  • CKEditor version: 24.0.0
  • Installed CKEditor plugins: Link

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@mmichaelis mmichaelis added the type:improvement This issue reports a possible enhancement of an existing feature. label May 19, 2021
@mmichaelis
Copy link
Author

Just to sketch my solution which seems to work for now:

  • I created a new LinkTargetCommand.
  • I registered LinkTargetCommand as listener to submit event with high priority, so that it runs prior to LinkCommand.

And implemented LinkTargetCommand as follows:

  • I copied, pasted and adapted refresh from LinkCommand to deal with new linkTarget attribute.
  • I copied, pasted and adapted execute from LinkCommand with similar calculation of ranges, but instead modifying the model, I just store ranges and target to be set in a variable.
  • Special case: if href is to be set to a non-empty value in a collapsed selection (thus, a text will be written), I just set the target attribute to be set as selection attribute. This will be recognized by LinkCommand automatically and applied to the newly created text.
  • In a registered post-fixer I see, if there are any changes to apply. If yes, I add (or remove) linkTarget and clear the state variable.
  • I added a listener to DocumentSelection for change:attribute and in case the linkHref attribute gets removed, I also ensure to remove linkTarget attribute. Otherwise, after LinkCommand moved the selection to the end of the link, linkTarget would have been applied when continuing typing.

This seems to do the trick and undo/redo works as expected. Nevertheless, I think it is a cumbersome approach. Any better extension point to LinkCommand would be appreciated.

@mmichaelis
Copy link
Author

mmichaelis commented Jun 17, 2021

Listening to "open form view": In order to provide a "clean fields" behavior like for fixing ckeditor/ckeditor5-link#78 (now: #4765) and ckeditor/ckeditor5-link#123 (now: #4793) as noted in LinkUI#_addFormView for any custom fields, you have to deeply integrate at various locations:

  • You have to listen to the edit-event in action view.
  • You have to listen to the toolbar button being clicked.
  • You have to listen to the keystroke event Ctrl+K.

Doing so will be very error-prone with any CKEditor update.

Again, a common extension point would be helpful here, at least providing some central event to listen to.

The only feasible option we found in the end, was listening to private API:

linkUI.decorate("_addFormView");
linkUI.on("_addFormView", () => {
  console.error("_addFormView triggered");
});

mmichaelis added a commit to CoreMedia/ckeditor-plugins that referenced this issue Jun 17, 2021
Similar to `LinkUI` from CKEditor's Link Feature we must ensure, that
target input field and behavior selector are reset each time the form
view is opened.

The previous approach only covered the use-case of doing so from action
view by listening to edit-button. But, this neither deals with Ctrl+K
keystroke nor with toolbar action.

The only feasible workaround we found, was adding a decorator to the
private method `LinkUI#_addFormView`, and thus refers to missing
extension points as reported at ckeditor/ckeditor5#9730.

See-also: ckeditor/ckeditor5#4765
See-also: ckeditor/ckeditor5#4793
See-also: ckeditor/ckeditor5#9730
@quicksketch
Copy link

I'm surprised this hasn't gotten more traction. Like @mmichaelis, I'm trying to set more attributes on <a> tags than just href or boolean true/false properties through decorators. Some attribute need string values, such as title, id, or class, and decorators' boolean mechanism is not sufficient. I would l like to set these attributes through a non-CKEditor UI such as a dialog (provided by a CMS). I've figured out opening a dialog through #4836, but actually setting attributes other than href seems extraordinarily difficult. Copy/pasting the entirety of the LinkCommand.exec() seems to be the only option.

@mmichaelis
Copy link
Author

@quicksketch, you also may want to take a look at #13594, where I sketched some required workaround to correctly handle custom attributes on cursor movement, unlinking, etc. Also, we learned late, that integrating these custom attributes into TwoStepCaretMovement is an important point not to forget. Regarding this "behind the doors" (thus, non-UI) part, you may find some insights here: LinkAttributes.ts.

It also documents an important finding, that all attributes, that are meant to "belong to a link" should be prefixed with link within the model representation, as you will then benefit from some auto-processing of the link feature.

@Witoso
Copy link
Member

Witoso commented Jun 21, 2023

Thanks for the comments, I will try to take a deeper look at this topic. We also have a recent community PR (#13985) that informs about similar decorating issues and mentions the UI side as well. cc @niegowski.

@Witoso Witoso added package:link domain:dx This issue reports a developer experience problem or possible improvement. domain:integration-dx This issue reports a problem with the developer experience when integrating CKEditor into a system. labels Jun 21, 2023
@quicksketch
Copy link

Thanks @mmichaelis, looks like you've really gone down the rabbit hole on CoreMedia. That will probably be a valuable resource for me.

In the issue summary @mmichaelis suggests adding an event for set:attributes. I think making the LinkCommand.execute() method able to set specific attributes through decorators may do the job. Something like this:

const linkCommand = editor.commands.get( 'link' );
linkCommand.execute( 'http://example.com', {
	linkTarget: '_blank',
} );

Or add a 3rd parameter to set arbitrary attributes and avoid over-complicating decorator configuration:

const linkCommand = editor.commands.get( 'link' );
linkCommand.execute( 'http://example.com', {}, {
	linkTarget: '_blank',
} );

It would be nice if the attribute could be configured to handle automatic upcast/downcast with a 1-to-1 value, but that's not too difficult to do through the existing conversion APIs so I'd consider that optional.

@quicksketch
Copy link

It seems a Drupal contrib module actually implemented almost exactly my second suggestion, extending the link execute event to accept a 3rd argument of arbitrary attributes: https://git.drupalcode.org/project/editor_advanced_link/-/blob/2.x/js/ckeditor5_plugins/editorAdvancedLink/src/editoradvancedlinkediting.js

image

Although they added all the additional attributes directly to the balloon and I plan to use a CMS-provided modal, the solution there works just as well for making it so that additional attributes can be tracked and updated.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@quicksketch
Copy link

This is still a desired API feature. Both Drupal and Backdrop are re-implementing a large swath of the LinkCommand, while implementing this in LinkCommand would take substantially less work comparatively, help other projects that need the same thing, and save us from unexpected API breaks caused by overriding such a large segment of code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:integration-dx This issue reports a problem with the developer experience when integrating CKEditor into a system. package:link type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

4 participants