Fix RCE linking once and for all

closes LS-1147
flag = rce_enhancements

test plan: see the ticket acceptance criteria

Change-Id: I16cfa998b419505069556a6fc7a0e8a857b221cb
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/240738
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Alex Anderson <raanderson@instructure.com>
QA-Review: Robin Kuss <rkuss@instructure.com>
Product-Review: Ed Schiebel <eschiebel@instructure.com>
This commit is contained in:
Ed Schiebel 2020-06-19 15:57:46 -04:00
parent 372c7fb319
commit 35945d9d0e
18 changed files with 232 additions and 100 deletions

View File

@ -123,7 +123,7 @@ export default class Bridge {
return false
}
insertLink = (link, textOverride) => {
insertLink = link => {
if (this.focusedEditor) {
const {selection} = this.focusedEditor.props.tinymce.get(this.focusedEditor.props.textareaId)
link.selectionDetails = {
@ -133,7 +133,7 @@ export default class Bridge {
if (!link.text) {
link.text = link.title || link.href
}
this.focusedEditor.insertLink(link, textOverride)
this.focusedEditor.insertLink(link)
this.controller?.hideTray()
} else {
console.warn('clicked sidebar link without a focused editor')

View File

@ -102,7 +102,7 @@ describe('Editor/Sidebar bridge', () => {
it('insertLink with an active editor forwards the link to createLink', () => {
Bridge.focusEditor(editor)
Bridge.insertLink(link)
expect(editor.insertLink).toHaveBeenCalledWith(link, undefined)
expect(editor.insertLink).toHaveBeenCalledWith(link)
})
it('insertLink with no active editor is a no-op, but warns', () => {
@ -114,15 +114,12 @@ describe('Editor/Sidebar bridge', () => {
it('adds selectionDetails to links', () => {
Bridge.focusEditor(editor)
Bridge.insertLink({})
expect(editor.insertLink).toHaveBeenCalledWith(
{
selectionDetails: {
node: 'some-node',
range: 'some-range'
}
},
undefined
)
expect(editor.insertLink).toHaveBeenCalledWith({
selectionDetails: {
node: 'some-node',
range: 'some-range'
}
})
})
it('calls hideTray after inserting a link', () => {

View File

@ -426,9 +426,9 @@ class RCEWrapper extends React.Component {
}
}
insertLink(link, isNew) {
insertLink(link) {
const editor = this.mceInstance()
const element = contentInsertion.insertLink(editor, link, isNew)
const element = contentInsertion.insertLink(editor, link)
this.contentInserted(element)
}

View File

@ -38,7 +38,7 @@ describe('contentInsertion', () => {
},
selection: {
getNode: () => {
return null
return editor.selectionContent ? 'p' : null
},
getContent: () => {
return editor.selectionContent
@ -63,11 +63,13 @@ describe('contentInsertion', () => {
setAttribs: (elem, attrs) => {
if (elem?.nodeType === 1) {
// this is an HTMLElement
Object.keys(attrs).forEach(a => {
if (attrs[a]) {
elem.setAttribute(a, attrs[a])
}
})
Object.keys(attrs)
.sort()
.forEach(a => {
if (attrs[a]) {
elem.setAttribute(a, attrs[a])
}
})
}
return elem
},
@ -77,6 +79,12 @@ describe('contentInsertion', () => {
return false
}
}
},
createHTML: (tag, attrs, text) => {
const elem = document.createElement(tag)
editor.dom.setAttribs(elem, attrs)
elem.innerHTML = text
return elem.outerHTML
}
},
undoManager: {
@ -95,7 +103,11 @@ describe('contentInsertion', () => {
return {left: 0, top: 0, bottom: 0, right: 0}
}
},
execCommand: jest.fn()
execCommand: jest.fn((cmd, ui, opts) => {
if (cmd === 'mceInsertLink') {
editor.content = editor.dom.createHTML('a', opts, editor.selectionContent)
}
})
}
})
@ -123,7 +135,7 @@ describe('contentInsertion', () => {
link.embed = {type: 'image'}
contentInsertion.insertLink(editor, link)
expect(editor.content).toEqual(
'<a href="/some/path" title="Here Be Links" class="instructure_file_link instructure_image_thumbnail">Click On Me</a>'
'<a class="instructure_file_link instructure_image_thumbnail" href="/some/path" title="Here Be Links">Click On Me</a>'
)
})
@ -131,7 +143,7 @@ describe('contentInsertion', () => {
link.embed = {type: 'scribd'}
contentInsertion.insertLink(editor, link)
expect(editor.content).toEqual(
'<a href="/some/path" title="Here Be Links" class="instructure_file_link instructure_scribd_file">Click On Me</a>'
'<a class="instructure_file_link instructure_scribd_file" href="/some/path" title="Here Be Links">Click On Me</a>'
)
})
@ -140,18 +152,17 @@ describe('contentInsertion', () => {
link.class = 'instructure_file_link foo'
contentInsertion.insertLink(editor, link)
expect(editor.content).toEqual(
'<a href="/some/path" title="Here Be Links" data-canvas-previewable="true" class="instructure_file_link foo">Click On Me</a>'
'<a class="instructure_file_link foo" data-canvas-previewable="true" href="/some/path" title="Here Be Links">Click On Me</a>'
)
})
it('respects the current selection building the link by delegating to tinymce', () => {
editor.selection.setContent('link me')
contentInsertion.insertLink(editor, link)
expect(editor.content).toEqual('<a href="/some/path" title="Here Be Links">link me</a>')
expect(editor.execCommand).toHaveBeenCalledWith('mceInsertLink', false, expect.any(Object))
})
it('cleans a url with no protocol when linking the current selection', () => {
editor.execCommand = jest.fn()
editor.selection.setContent('link me')
link.href = 'www.google.com'
contentInsertion.insertLink(editor, link)

View File

@ -18,7 +18,6 @@
import classnames from 'classnames'
import {
renderLink,
renderImage,
renderLinkedImage,
renderVideo,
@ -26,7 +25,12 @@ import {
mediaIframeSrcFromFile
} from './contentRendering'
import scroll from '../common/scroll'
import {cleanUrl} from './contentInsertionUtils'
import {
cleanUrl,
getAnchorElement,
isOnlyTextSelected,
isImageFigure
} from './contentInsertionUtils'
/** * generic content insertion ** */
@ -156,24 +160,28 @@ function decorateLinkWithEmbed(link) {
}
}
export function insertLink(editor, link, textOverride) {
export function insertLink(editor, link) {
const linkAttrs = {...link}
if (linkAttrs.embed) {
decorateLinkWithEmbed(linkAttrs)
delete linkAttrs.embed
}
return insertUndecoratedLink(editor, linkAttrs, textOverride)
return insertUndecoratedLink(editor, linkAttrs)
}
// link edit/create logic based on tinymce/plugins/link/plugin.js
function insertUndecoratedLink(editor, linkProps, textOverride) {
function insertUndecoratedLink(editor, linkProps) {
const selectedElm = editor.selection.getNode()
const anchorElm = getAnchorElement(editor, selectedElm)
const selectedHtml = editor.selection.getContent({format: 'text'})
const selectedContent = editor.selection.getContent()
const selectedPlainText = editor.selection.getContent({format: 'text'})
const onlyText = isOnlyTextSelected(selectedContent)
const linkText =
(textOverride && editor.dom.encode(textOverride)) ||
selectedHtml ||
editor.dom.encode(linkProps.text)
onlyText &&
((linkProps.text && editor.dom.encode(linkProps.text)) ||
getAnchorText(editor.selection, anchorElm))
// only keep the props we want as attributes on the <a>
const linkAttrs = {
id: linkProps.id,
@ -190,39 +198,44 @@ function insertUndecoratedLink(editor, linkProps, textOverride) {
editor.focus()
if (anchorElm) {
anchorElm.innerText = linkText
editor.dom.setAttribs(anchorElm, linkAttrs)
editor.selection.select(anchorElm)
editor.undoManager.add()
updateLink(editor, anchorElm, linkText, linkAttrs)
} else if (selectedContent) {
if (linkProps.userText && selectedPlainText !== linkText) {
createLink(editor, selectedElm, linkText, linkAttrs)
} else {
createLink(editor, selectedElm, undefined, linkAttrs)
}
} else {
createLink(editor, selectedElm, linkText, linkAttrs)
}
return editor.selection.getEnd() // this will be the newly created or updated content
}
function getAnchorElement(editor, selectedElm) {
selectedElm = selectedElm || editor.selection.getNode()
if (isImageFigure(selectedElm)) {
return editor.dom.select('a[href]', selectedElm)[0]
} else {
return editor.dom.getParent(selectedElm, 'a[href]')
}
function getAnchorText(selection, anchorElm) {
return anchorElm ? anchorElm.innerText : selection.getContent({format: 'text'})
}
function isImageFigure(elm) {
return (
elm &&
((elm.nodeName === 'FIGURE' && /\bimage\b/i.test(elm.className)) || elm.nodeName === 'IMG')
)
function updateLink(editor, anchorElm, text, linkAttrs) {
if (text && anchorElm.innerText !== text) {
anchorElm.innerText = text
}
editor.dom.setAttribs(anchorElm, linkAttrs)
editor.selection.select(anchorElm)
editor.undoManager.add()
}
function createLink(editor, selectedElm, text, linkAttrs) {
if (isImageFigure(selectedElm)) {
linkImageFigure(editor, selectedElm, linkAttrs)
} else if (text) {
// create the whole wazoo
editor.insertContent(editor.dom.createHTML('a', linkAttrs, editor.dom.encode(text)))
} else {
insertContent(editor, renderLink(linkAttrs, text))
// create a link on the selected content
editor.execCommand('mceInsertLink', false, linkAttrs)
}
}
function linkImageFigure(editor, fig, attrs) {
const img = fig.tagName === 'IMG' ? fig : editor.dom.select('img', fig)[0]
if (img) {

View File

@ -47,3 +47,41 @@ export function cleanUrl(input) {
}
return url
}
// given the current selection, find the containing anchor
export function getAnchorElement(editor, selectedElm) {
selectedElm = selectedElm || editor.selection.getNode()
if (isImageFigure(selectedElm)) {
return editor.dom.select('a[href]', selectedElm)[0]
} else {
return editor.dom.getParent(selectedElm, 'a[href]')
}
}
// is the selection only text, or are other elements selected
const d = document.createElement('div')
export function isOnlyTextSelected(html) {
// this regex-based code is lifted from tinymce's link plugin, but I didn't like it.
// if (/</.test(html) && (!/^<a [^>]+>[^<]+<\/a>$/.test(html) || html.indexOf('href=') === -1)) {
// return false
// }
// return true
d.innerHTML = html
return !d.querySelector('img,iframe,video,audio')
}
export function isOKToLink(html) {
// I know, parsing html with regexp is dangerous, but this is called way too often
// from the instructure_link/plugin.js to create the nodes for a better check
if (/<(?:iframe|audio|video)/.test(html)) {
return false
}
return true
}
export function isImageFigure(elm) {
return (
elm && elm.nodeName === 'FIGURE' && /\bimage\b/i.test(elm.className)
// (elm.nodeName === 'IMG' || (elm.nodeName === 'FIGURE' && /\bimage\b/i.test(elm.className)))
)
}

View File

@ -116,7 +116,7 @@ tinymce.create('tinymce.plugins.InstructureImagePlugin', {
trayController.showTrayForEditor(editor)
},
text: formatMessage('Options'),
text: formatMessage('Image Options'),
tooltip: buttonAriaLabel
})

View File

@ -18,7 +18,7 @@
import React from 'react'
import ReactDOM from 'react-dom'
import bridge from '../../../../../bridge'
import {getContentFromEditor} from '../../../shared/ContentSelection'
import {isOnlyTextSelected, getAnchorElement} from '../../../../contentInsertionUtils'
import LinkOptionsDialog from './index'
export const CONTAINER_ID = 'instructure-link-options-tray-container'
@ -59,7 +59,7 @@ export default class LinkOptionsDialogController {
_applyLinkOptions(linkOptions) {
this._dismissDialog()
bridge.insertLink(linkOptions, linkOptions.text)
bridge.insertLink(linkOptions)
}
_dismissDialog = () => {
@ -75,8 +75,15 @@ export default class LinkOptionsDialogController {
}
_renderDialog() {
const content = getContentFromEditor(this._editor, this._op === EDIT_LINK)
let html, onlyText, anchorElm, url, text
if (this._shouldOpen) {
html = this._editor.selection.getContent()
onlyText = isOnlyTextSelected(html)
text = onlyText && this._editor.selection.getContent({format: 'text'})
anchorElm = getAnchorElement(this._editor, this._editor.selection.getNode())
url = anchorElm?.getAttribute('href')
/*
* When the dialog is being opened again, it should be rendered fresh
* (clearing the internal state) so that the currently-selected content
@ -88,8 +95,9 @@ export default class LinkOptionsDialogController {
<LinkOptionsDialog
key={this._renderId}
size="medium"
text={content.text}
url={content.url}
showText={onlyText}
text={text}
url={url}
operation={this._op}
onEntered={() => {
this._isOpen = true

View File

@ -88,7 +88,7 @@ describe('RCE "Links" Plugin > LinkOptionsDialog > LinkOptionsDialogController',
it('inserts the link', () => {
dialogController.showDialogForEditor(editor, 'create')
dialogController._applyLinkOptions({})
expect(bridge.insertLink).toHaveBeenCalledWith({}, undefined)
expect(bridge.insertLink).toHaveBeenCalledWith({})
})
})

View File

@ -41,7 +41,8 @@ export default function LinkOptionsDialog(props) {
props.onSave({
text: linkText,
target: '_blank',
href: url
href: url,
userText: props.showText
})
}
@ -79,14 +80,16 @@ export default function LinkOptionsDialog(props) {
</Modal.Header>
<Modal.Body>
<View as="div" margin="small">
<TextInput
renderLabel={formatMessage('Text')}
name="linktext"
onChange={handleTextChange}
value={text}
/>
</View>
{props.showText && (
<View as="div" margin="small">
<TextInput
renderLabel={formatMessage('Text')}
name="linktext"
onChange={handleTextChange}
value={text}
/>
</View>
)}
<View as="div" margin="small">
<TextInput
renderLabel={formatMessage('Link')}
@ -118,10 +121,12 @@ LinkOptionsDialog.propTypes = {
onExited: func,
onRequestClose: func.isRequired,
onSave: func.isRequired,
open: bool.isRequired
open: bool.isRequired,
showText: bool
}
LinkOptionsDialog.defaultProps = {
onEntered: null,
onExited: null
onExited: null,
showText: true
}

View File

@ -20,6 +20,7 @@ import React from 'react'
import ReactDOM from 'react-dom'
import bridge from '../../../../../bridge'
import {getLinkContentFromEditor} from '../../../shared/ContentSelection'
import {getAnchorElement} from '../../../../contentInsertionUtils'
import LinkOptionsTray from '.'
export const CONTAINER_ID = 'instructure-link-options-tray-container'
@ -48,6 +49,15 @@ export default class LinkOptionsTrayController {
showTrayForEditor(editor) {
this._editor = editor
this._shouldOpen = true
const selectedElm = editor.selection.getNode()
if (editor.selection.isCollapsed() && selectedElm.nodeName === 'A') {
// expand the selection to include the whole <a>
editor.selection.select(editor.selection.getNode())
} else {
const anchorElm = getAnchorElement(editor, selectedElm)
editor.selection.select(anchorElm)
}
this._renderTray()
}
@ -59,7 +69,7 @@ export default class LinkOptionsTrayController {
_applyLinkOptions(linkOptions) {
this._dismissTray()
bridge.insertLink(linkOptions, linkOptions.text)
bridge.insertLink({...linkOptions, userText: true})
}
_dismissTray() {
@ -69,8 +79,9 @@ export default class LinkOptionsTrayController {
}
_renderTray() {
const content = getLinkContentFromEditor(this._editor)
let content
if (this._shouldOpen) {
content = getLinkContentFromEditor(this._editor)
/*
* When the tray is being opened again, it should be rendered fresh
* (clearing the internal state) so that the currently-selected content

View File

@ -29,7 +29,8 @@ describe('RCE "Links" Plugin > LinkOptionsTray', () => {
displayAs: 'link',
text: 'Syllabus.doc',
url: 'http://example.instructure.com/files/3201/download',
isPreviewable: true
isPreviewable: true,
onlyTextSelected: true
},
onRequestClose: jest.fn(),
onSave: jest.fn(),
@ -59,6 +60,12 @@ describe('RCE "Links" Plugin > LinkOptionsTray', () => {
renderComponent()
expect(tray.text).toEqual(props.content.text)
})
it('does not show the text field if something other than text was selected', () => {
props.content.onlyTextSelected = false
renderComponent()
expect(tray.$textField).toBe(null)
})
})
describe('"Link" field', () => {
it('uses the value of .url in the given content', () => {

View File

@ -34,8 +34,8 @@ import {
export default function LinkOptionsTray(props) {
const content = props.content || {}
const textToLink =
(content.$element?.tagName === 'A' ? content.$element?.textContent : content.text) || ''
const textToLink = content.text || ''
const showText = content.onlyTextSelected
const [text, setText] = useState(textToLink || '')
const [url, setUrl] = useState(content.url || '')
const [autoOpenPreview, setAutoOpenPreview] = useState(content.displayAs === DISPLAY_AS_EMBED)
@ -109,13 +109,15 @@ export default function LinkOptionsTray(props) {
<Flex.Item grow padding="small" shrink>
<input type="submit" style={{display: 'none'}} />
<Flex direction="column">
<Flex.Item padding="small">
<TextInput
renderLabel={() => formatMessage('Text')}
onChange={handleTextChange}
value={text}
/>
</Flex.Item>
{showText && (
<Flex.Item padding="small">
<TextInput
renderLabel={() => formatMessage('Text')}
onChange={handleTextChange}
value={text}
/>
</Flex.Item>
)}
<Flex.Item padding="small">
<TextInput
@ -156,7 +158,7 @@ export default function LinkOptionsTray(props) {
padding="small medium"
textAlign="end"
>
<Button disabled={!text || !url} onClick={handleSave} variant="primary">
<Button disabled={(showText && !text) || !url} onClick={handleSave} variant="primary">
{formatMessage('Done')}
</Button>
</Flex.Item>

View File

@ -16,10 +16,56 @@
* with this program. If not, see <http://www.gnu.org/licenses/>.
*/
/*
* here's how linking works
* Creation:
* 1. No text is selected, user clicks create link button:
* - display the create link dialog with text and URL input
* - insert an <a> at the caret, linked to the URL with the text content
* 2. Text is selected, user clicks create link button:
* - display the create link dialog with text and URL input
* - the text input displays the plain-text content of the selection
* - on saving, if the plain-text has not changed, leave it unchanged in the RCE,
* if it has changed, replace the selection with the new plain-text.
* Wrap the text in an <a>, linked to the URL
* 3. An image + optional text is selected, user clicks create link button
* - display the create link dialog with URL input only
* - on saving, the selection is wrapped in an <a>, linked to the URL
* 4. An iframe is w/in the selection
* - disable the create link function
*
* Editing:
* 1. the caret is w/in a text link, but nothing is selected or
* some subset of the link's text is selected
* - display the link Options popup button. when clicked...
* - expand the selection to be the whole link text
* - display the tray with the link's plain-text in the text input and the href
* in the URL input
* - on saving, if the plain-text is unchanged, leave the text unchanged in the RCE,
* if it has changed, replace the link text with the new plain-text.
* Update the <a>'s href to the new URL
* 2. An image w/in a link is selected, or the caret is on the image, or the image
* plus some surrounding text that's all part of the existing link is selected, or
* the caret is w/in a link that contains an image
* a. for now: show the link and image Options buttons in a popup toolbar
* - on clicking the link Options...
* - expand the selection to be the whole link contents
* - show the link options tray, with no text input
* - on saving, update the link's href
* b. new-improved: show a single Options button, when clicked...
* - expand the selection to be the whole link contents
* - show the options tray with Image Options and Link Options sections
* the link text input is empty.
* - on saving, if the link text input is still empty, replace the link's
* href with the new URL. if the link text is updated, replace the link's
* content with the new plain text (deleting the image)
*/
import formatMessage from '../../../format-message'
import clickCallback from './clickCallback'
import bridge from '../../../bridge'
import {isFileLink, asLink} from '../shared/ContentSelection'
import {getAnchorElement, isOKToLink} from '../../contentInsertionUtils'
import LinkOptionsTrayController from './components/LinkOptionsTray/LinkOptionsTrayController'
import {CREATE_LINK, EDIT_LINK} from './components/LinkOptionsDialog/LinkOptionsDialogController'
@ -27,15 +73,6 @@ const trayController = new LinkOptionsTrayController()
const PLUGIN_KEY = 'links'
const getLink = function(editor, elm) {
return editor.dom.getParent(elm, 'a[href]')
}
const getAnchorElement = function(editor, node) {
const link = node.nodeName.toLowerCase() === 'a' ? node : getLink(editor, node)
return link && link.href ? link : null
}
tinymce.create('tinymce.plugins.InstructureLinksPlugin', {
init(ed) {
const contextType = ed.settings.canvas_rce_user_context.type
@ -94,6 +131,9 @@ tinymce.create('tinymce.plugins.InstructureLinksPlugin', {
type: 'menuitem',
text: formatMessage('Remove Link'),
onAction() {
const selectedElm = ed.selection.getNode()
const anchorElm = getAnchorElement(ed, selectedElm)
ed.selection.select(anchorElm)
ed.execCommand('unlink')
}
}
@ -125,6 +165,7 @@ tinymce.create('tinymce.plugins.InstructureLinksPlugin', {
onSetup(api) {
function handleNodeChange(e) {
api.setActive(!!getAnchorElement(ed, e.element))
api.setDisabled(!isOKToLink(ed.selection.getContent()))
}
ed.on('NodeChange', handleNodeChange)
@ -143,7 +184,7 @@ tinymce.create('tinymce.plugins.InstructureLinksPlugin', {
ed.execCommand('instructureTrayToEditLink', false, ed)
},
text: formatMessage('Options'),
text: formatMessage('Link Options'),
tooltip: buttonAriaLabel
})

View File

@ -118,7 +118,7 @@ tinymce.create('tinymce.plugins.InstructureRecord', {
trayController.showTrayForEditor(ed)
},
text: formatMessage('Options'),
text: formatMessage('Video Options'),
tooltip: buttonAriaLabel
})

View File

@ -17,6 +17,7 @@
*/
import {fromImageEmbed, fromVideoEmbed} from '../instructure_image/ImageEmbedOptions'
import {isOnlyTextSelected} from '../../contentInsertionUtils'
const FILE_DOWNLOAD_PATH_REGEX = /^\/(courses\/\d+\/)?files\/\d+\/download$/
@ -71,7 +72,8 @@ export function asLink($element, editor) {
return {
$element: $link,
displayAs,
text: editor.selection.getContent() || $link.textContent,
text: $link.textContent,
onlyTextSelected: isOnlyTextSelected(editor.selection.getContent()),
type,
isPreviewable,
url: $link.href

View File

@ -68,11 +68,8 @@ describe('RCE > Plugins > Shared > Content Selection', () => {
})
it('finds the selected text', () => {
// a bit contrived, but proves we find the selected text
// if there is some w/in the link
editor.selection.getContent = () => 'foo'
const content = getContentFromElement($element, editor)
expect(content.text).toEqual('foo')
expect(content.text).toEqual('Syllabus.doc')
})
it('includes the url of the link', () => {

View File

@ -43,7 +43,7 @@ describe "Wiki pages and Tiny WYSIWYG editor Files" do
@course.wiki_pages.first.publish!
create_session(@student.pseudonym)
get "/courses/#{@course.id}/pages/front-page"
expect(f('a[title="Link"]')).to include_text("text_file.txt")
expect(fj('a:contains("text_file.txt")')).to be_displayed
end
end