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

Fix TinyMCE edit link when link contains html #2993

Closed
wants to merge 2 commits into from

Conversation

satrun77
Copy link

@satrun77 satrun77 commented Aug 20, 2024

Description

If you have a link in tinymce like <a href=""><span>Title</span></a>, then you can edit the link. The HTML inside the link tag can be any tag.

The issue is that the selected node is the inside HTML tag instead of the link tag.

I'm not 100% keen on the solution using while but this is legacy code, so might be ok! Alternative code is welcomed :)

This PR is needed for silverstripe/silverstripe-admin#1814

Manual testing steps

  1. Install fresh CMS
  2. Edit a page
  3. Add a link (internal or external)
  4. Edit HTML code in TinyMCE, add any HTML tags inside the link
  5. Close the HTML code view in TInyMCE
  6. Try to edit the link, it won't show the popup form.

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@satrun77 satrun77 changed the title Pull/edit link tinymce Fix TinyMCE edit link when link contains html Aug 20, 2024
@satrun77 satrun77 marked this pull request as ready for review August 20, 2024 08:49
@satrun77 satrun77 changed the base branch from 5 to 5.2 August 20, 2024 08:52
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this - unfortunately there's some problems with the approach.

Comment on lines +125 to +131
if (linkNode.nodeName !== 'A') {
let count = 0;
while (count < 10 && linkNode.parentNode && linkNode.nodeName !== 'A') {
linkNode = linkNode.parentNode;
count +=1;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use a while loop - especially with a completely arbitrary number of iterations. If there's a function available to recursively check the ancestors of the element, use that. Otherwise, this might need to go back to the drawing board.

@@ -119,7 +119,17 @@ jQuery.entwine('ss', ($) => {

getOriginalAttributes() {
const editor = this.getElement().getEditor();
const node = $(editor.getSelectedNode());

// Find "a" node, issue https://github.com/silverstripe/silverstripe-cms/issues/2439
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Find "a" node, issue https://github.com/silverstripe/silverstripe-cms/issues/2439
// Try to find a parent <a> tag in case the user clicks on some child node inside a link

@GuySartorelli GuySartorelli self-assigned this Aug 21, 2024
@satrun77
Copy link
Author

satrun77 commented Aug 21, 2024

No longer needed! The fix in silverstripe/silverstripe-admin#1814 fixes the issue

@satrun77 satrun77 closed this Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants