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 Cannot link an image in TinyMCE #1571

Conversation

sabina-talipova
Copy link
Contributor

Description

We should check that user selected an element and if element doesn't have inner content return outer HTML.

Parent issue

@sabina-talipova
Copy link
Contributor Author

I copied changes from CMS 5.

const selectionContent = selection.getContent() || '';
const editor = this.getElement().getEditor();
const selection = editor.getInstance().selection;
const selectionContent = editor.getSelection();
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
const selectionContent = editor.getSelection();
let selectionContent = selection.getContent() || selection.getNode() || '';
if (typeof selectionContent === 'object') {
selectionContent = selectionContent.outerHTML || '';
}

Try to get just the text, if there is just text.
If there's no text, try to get the node and grab its outer HTML.

Note that I haven't tested this with anything other than an image so far. Make sure to double check how this interacts with things like iframes (when explicitly allowed), embeds, etc - make sure we're still only allowing links to be added to things that should be allowed.

Copy link
Contributor Author

@sabina-talipova sabina-talipova Sep 4, 2023

Choose a reason for hiding this comment

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

This solution will not work in the opposite direction if the user has not selected anything, since getNode returns the parent element and, accordingly, true.

Comment on lines 193 to 195
if (!selection && instance.selection.getSel().type === 'Range') {
selection = instance.selection.getNode().outerHTML;
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be correct. In CMS 4 this still results in the buggy behaviour. When I debug this with my image selected, instance.selection.getSel() returns:

Selection {
  anchorNode: img.leftAlone.ss-htmleditorfield-file.image,
  anchorOffset: 0,
  focusNode: img.leftAlone.ss-htmleditorfield-file.image,
  focusOffset: 0,
  isCollapsed: true,
  rangeCount: 1,
  type: "Caret",
  caretBidiLevel: 0
}

Calling toString() on this results in an empty string, and because the type isn't Range it returns that empty string.

If this works in CMS 5 it is likely due to the changes in tinymce - we will need to do a CMS 4 specific fix first, then merge up, then if it's still broken in CMS 5 we'll need to tackle that separately.

I've suggested a change below that seems to work fine, and doesn't require this new function at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still cannot reproduce this issue with CMS 4.13. And when I check value of instance.selection.getSel() on selected image I have the following:

Selection {
  anchorNode: p
  anchorOffset: 0
  baseNode: p
  baseOffset: 0
  extentNode: p
  extentOffset: 1
  focusNode: p
  focusOffset: 1
  isCollapsed: false
  rangeCount: 1
  ----> type: "Range"
}

Copy link
Member

@GuySartorelli GuySartorelli Sep 4, 2023

Choose a reason for hiding this comment

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

Looks like you're somehow selecting a paragraph element instead of the image... how are you selecting the image? For me I click on the image - it briefly selects and then deselects it, so I have to click it a second time to select it. Video to show that in action:
screen recording of the issue happening

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is record what happens in my local environment for clean install(silverstripe-installer) CMS 4.13

@sabina-talipova
Copy link
Contributor Author

I implemented previous logic from CMS5. Probably we will have to fix it in CMS5 after we merge 4.13, since in TinyMCE 6 getContent() doesn't work with html entities.

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.

Link to external URL is just completely broken now. I get this error:

Uncaught TypeError: a.getSelection is not a function

Given you can't see the actual broken behaviour, it might be best if you let someone who can see the broken behaviour fix it for CMS 4, and then you can look at it in CMS 5 after the merge up.

@sabina-talipova sabina-talipova force-pushed the pulls/1.13/fix-link-on-img branch 2 times, most recently from 94b84cf to dca6dbb Compare September 5, 2023 04:15
@GuySartorelli
Copy link
Member

Superceded by #1577

@GuySartorelli GuySartorelli deleted the pulls/1.13/fix-link-on-img branch September 6, 2023 00:32
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