-
Notifications
You must be signed in to change notification settings - Fork 94
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
FIX Cannot link an image in TinyMCE #1571
Conversation
I copied changes from CMS 5. |
const selectionContent = selection.getContent() || ''; | ||
const editor = this.getElement().getEditor(); | ||
const selection = editor.getInstance().selection; | ||
const selectionContent = editor.getSelection(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
client/src/legacy/HtmlEditorField.js
Outdated
if (!selection && instance.selection.getSel().type === 'Range') { | ||
selection = instance.selection.getNode().outerHTML; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
}
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
cb6d5e6
to
7bc328c
Compare
I implemented previous logic from CMS5. Probably we will have to fix it in CMS5 after we merge 4.13, since in TinyMCE 6 |
There was a problem hiding this 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.
94b84cf
to
dca6dbb
Compare
dca6dbb
to
793aa74
Compare
Superceded by #1577 |
Description
We should check that user selected an element and if element doesn't have inner content return outer HTML.
Parent issue