Editor: linking to a file overwrites selection

[lsinger]

Steps to reproduce

1. Edit a post or page
2. Select some text that you want to link to something
3. Press the (+) button to insert / link to a file
4. Select a file to link to
5. Choose "insert"

What I expected

I expected my selected text to be preserved, linking to the file I just selected.

What happened instead

Instead, the selected text was removed and replaced with the filename of the file I inserted.

Screenshot / Video

[adnanhb]

Is anybody working on this issue? If not, I would like to claim it.

[unDemian]

I'm not sure if persisting the text should be expected behavior here, because the file selector allows the selection of multiple files, which makes it difficult to have a single word pointing at multiple file's url.

[lsinger]

difficult to have a single word pointing at multiple file's url

Good point. What if we only treat the one-file-case in this special manner?

[unDemian]

I guess we could treat it as special case, but that might require quite a big change in the code. Because right now all assets from the + Insert Content are appended to the content instead of updating it.

If we are going to keep selected text while inserting new media content, for consistency's sake we should do it for all media types not just files. And if that is the case we should figure out how it behaves in scenarios like:

• Inserting multiple items while having selected text
• Inserting images while having selected text
• Should this also affect forms and google content ?
• What happens to the selected text when we insert shortcodes for audio and video?

[lsinger]

I still think that there's a special case here.

Pseudocode:

if "text selected and inserting a single non-embedabble item (that will be linked to instead of shown inline)"
    use selected text as link text
else
    remove selected text and insert items (as it is now)

If the effort is really high, though, it might make sense to not invest that right now and wait till Gutenberg has landed. Thoughts?

[unDemian]

That sounds good but from my brief research seems like quite an update and my Calypso class is almost finished so I'll definitely not be able to properly estimate or do it.

[lsinger]

Ah. I wasn't aware that you wanted to do this as part of a class.

[lsinger]

@adnanhb sorry for not replying earlier. If a ticket is unassigned I think you can just start working on it. Maybe add a comment so people know you are. Or self-assign.

[jdlrobson]

^ No tests (can add some if you think solution works), but I think this is what @lsinger is requesting...?

I'm not sure it's worth the effort, given this may produce inconsistent behaviour elsewhere in similar workflows. Also it's not clear what should happen if text is highlighted and two images are selected. Currently the text gets replaced with the two images in that case.

Given @unDemian wanting to rewrite this and the above is this really a "good first change" ?

[lsinger]

Thanks @jdlrobson, this works well for me.

not sure it's worth the effort

I'm happy to go with a solution for just this special case for now, or with forgoing this entirely — especially since Gutenberg is on the horizon.

really a "good first change"

I'm not sure about the specific heuristics here. Given that this issue will be closed soonish either because you supply a fix or because we close it as a wontfix — what would removing the label change, though?

[jdlrobson]

@lsinger is there any way to add tests for tinymce plugins?

[lsinger]

@jdlrobson none that I'm aware of off the bat (which doesn't mean they don't exist).

[lsinger]

@jdlrobson I looked around a bit and found a few TinyMCE plugins in the wp-calypso repository that come with tests:

• contact-form
• embed
• media/restrict-size
• wpcom
• wpcom-sourcecode

Do these examples help?

[jdlrobson]

Hi @lsinger, thanks for those examples. Sadly don't quite help me as much as I was hoping.
The functions I want to test are not accessible and the mediaButton is quite large (probably could do with a bit of refactoring), so I need to pull the bits I want to test out into submodules (I don't have time right now to test the whole thing).

Ideally, I would pull out the entire onInsertMedia method and export it as a function that can be bound to the editor... but that's a lot of work given it has lots of dependencies. The easiest thing to do is just pull out and test insertMedia and insertMediaAsLink but that doesn't provide coverage for the if/else switch (which I don't think is vital), on the other hand it does allow us to guard against certain attacks. I've submitted a new patch to hopefully demonstrate this better...

Alternative approach (not covered in current patch) would be something like this:

import getInsertMediaCommand from './commands'
...
<EditorMediaModal {...props} onInsertMedia={getInsertMediaCommand(editor)}
...

commands.js would look something like this:

export function ( editor, renderModal ) {
		return  ( markup, media ) => {
					const selectedText = editor.selection.getContent( { format: 'text' } );
					if ( media.length === 1 && selectedText ) {
						insertMediaAsLink( editor, media[ 0 ] );
					} else {
						insertMedia( editor, markup );
					}
					renderModal( { visible: false } );
				} };
		};
};

I don't really like the feel of doing all that given the rewrite happening.. as @unDemian eluded to it would need some big changes to the code..

[lsinger]

Indeed, that sounds like more effort than would currently be warranted.

Given that a) the rewrite is happening and b) many other TinyMCE plugins in the Calypso codebase also don't have any tests at all — I wouldn't be too concerned if this small change shipped without a test ... unless some sort of attack vector could be argued for, which I don't think is the case here — malicious code could wreck the attacker's own editor instance, but I don't currently see any way for this to affect other users.

[rachelmcr]

I confirmed this issue still exists in Calypso, and tested it in Gutenberg.

Gutenberg doesn't yet support this kind of action (you have to know the file URL to link to it, unless using a separate File block) but it's being explored in WordPress/gutenberg#8322. I'm going to close this issue since the Calypso fix is more complex than expected and the issue doesn't exist in the same way in Gutenberg.