If I insert a document then disable rich text this is the output.

<span class="file media-element file-default" data-file_info="%7B%22fid%22:%22299%22,%22view_mode%22:%22default%22,%22type%22:%22media%22%7D">
<img alt="" class="file-icon" src="/modules/file/icons/x-office-document.png" title="application/msword" /> 
<a href="http://example.com/files/example/xxxx.doc" type="application/msword; length=58368">xxxx.doc</a>
</span>

If I insert an image I get the following
[[{"fid":"307","view_mode":"default","type":"media","attributes":{"height":200,"width":400,"class":"media-element file-default"}}]]

I'd guess the system is set up correctly for images to work correctly, but there is something non standard about document file types?

CommentFileSizeAuthor
#22 media-wysiwyg-document-support-1942688-22.patch2.95 KBDavid_Rothstein
#21 media-wysiwyg-document-support-1942688-19.patch2.95 KBdalin
#18 media-wysiwyg-document-support-1942688-17.patch717 bytesPaul Lomax
#7 media-wysiwyg-document-support-1942688-7.patch2.6 KBDavid_Rothstein
#1 media-wysiwyg-document-support-1942688.patch718 bytesPaul Lomax

Comments

Paul Lomax’s picture

Paul Lomax commented
Status: Active » Needs review
StatusFileSize
new media-wysiwyg-document-support-1942688.patch718 bytes

Patch is attached which changes the regex so it supports 'placeholder' span containers.

This probably isn't foolproof as documents aren't really placeholders, they can be a mixture of markup. But if someone modifies the markup output by the module they probably don't want to use the file templates anyway? In theory.

This does work anyway, not sure if there are any other cases where this wouldn't. This is a hard one to spot really, as Media inserts the correct markup, so on the face of it the insertion works perfectly. But the data that gets saved is a hard coded path to the file, so it can cause problems when migrating from dev to live URLs.

Thanks to Owain Cleaver for the regex.

ParisLiakos’s picture

ParisLiakos commented

i am in favor of this change, but might be bad for UX, eg altering the linked filename hit save and see that your changes are not there..maybe we could store the altered filename in the json macro attributes?

Paul Lomax’s picture

Paul Lomax commented

I see what you are saying, but it would be more than just the file label (which should be changed on the file entity). The user could change the icon for example. Really it shouldn't be editable at all? All the layout and contents should be loaded fresh from the file entity display every time. In my mind that is one of the most powerful parts of Media.

We could add some CSS to the editor to stop users editing it?

.media-element * {
  -webkit-touch-callout: none;
  -webkit-user-select: none;
  -khtml-user-select: none;
  -moz-user-select: none;
  -ms-user-select: none;
  user-select: none;
}

This would stop the contents of the element from being selected, but should still allow placeholder images to be selected / dragged.

I'm not sure if this is possible to do in the context of a WYSIWYG plugin though?

sheldonkreger’s picture

sheldonkreger commented
Status: Needs review » Needs work

I have tested patch in 1. The output inside of the WYSIWYG is changed as predicted. However, I'm still getting an absolute link to the document I attach.

Is the goal here to eliminate the absolute path? That would be a big improvement for moving content from a development to live server.

annikaC’s picture

annikaC commented

I've tested this patch too, thanks for this, really helped!

johnennew’s picture

johnennew commented
Status: Needs work » Reviewed & tested by the community

Been using this on a number of sites for a while now.

Personally I think this patch in #1 fixes the issue described so should be committed. Any user interface improvements could be filed as separate bug.

Thanks!

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new media-wysiwyg-document-support-1942688-7.patch2.6 KB

I ran into this issue as well, but I agree with @ParisLiakos that it's important to deal with the case of someone trying to change the link text. That is not merely a user experience "improvement" - it is actually existing functionality that the above patch breaks... It's very common to want to insert a link to a file and change the link text to be something other than the file name.

Here is an updated patch that gets that working. It is kind of hacky, though. Also, it only deals with changing the link text... As mentioned in #3, there are other ways you can edit the inserted HTML in the WYSIWYG (for example, deleting or changing the file icon) and this patch will neither make those work nor prevent them from happening.

David_Rothstein’s picture

David_Rothstein commented

I have tested patch in 1. The output inside of the WYSIWYG is changed as predicted. However, I'm still getting an absolute link to the document I attach.

Is the goal here to eliminate the absolute path? That would be a big improvement for moving content from a development to live server.

What these patches do is remove all absolute URLs from the text that gets stored in the database. This allows the content to be deployed between servers without any links breaking.

Each server might still generate an absolute URL (in the final HTML) when the content is viewed, of course... but that shouldn't cause any problems, should it?

sheldonkreger’s picture

sheldonkreger commented

What these patches do is remove all absolute URLs from the text that gets stored in the database. This allows the content to be deployed between servers without any links breaking.

Each server might still generate an absolute URL (in the final HTML) when the content is viewed, of course... but that shouldn't cause any problems, should it?

Yes, the link is not saved in the DB. I was just inspecting the field after saving the node in the browser and saw that the absolute URL is still shown in the

The patch in 7 applies cleanly and works as described. Based on the comments though, I'd say there are more kinks to work out here.

Paul Lomax’s picture

Paul Lomax commented

Surely you change the 'link text' by changing the label/title of the file? This way if a user has embedded 20 of the same file in various places across the site and want to change the label, they do it in one place rather than 20.

David_Rothstein’s picture

David_Rothstein commented

I don't think so; frequently when I link to something in multiple places (whether a file or anything else) I use different link text depending on the context of the words around it.

The other option (automatically having the link text inherit from the title) would definitely be nice in certain situations, and maybe there's a way to make this patch smart enough to handle both (e.g., inherit the title whenever the link text isn't specifically overridden?), but editing the link text is what Media supports now and it seems to me like the most important case to continue supporting.

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented

First some questions about the regex:

[\'"]
[^"']
Sometimes we escape the single quote, sometimes not. But there is no need to in this context.

([^"']+ )?
Is the space necessary? I think we might be able to simplify this to:
([^"']+)?

media-element[^>]*>
media-element[^>]*?>
An inconsistency here, but I think the latter is more appropriate.

([\S\s]+?)
This must be some sort of trick that I don't understand, because I think this is the same as:
(.+?)

A more thorough review forthcoming...

bneil’s picture

bneil commented

The patch in #7 works great. The placeholder markup is available in the WYSIWYG and is output correctly when viewing the node.

As for the link text discussion, I also think it makes sense to be able to change the link text on a per use basis, and I did not have any issues doing so with this patch.

The other option (automatically having the link text inherit from the title) would definitely be nice in certain situations, and maybe there's a way to make this patch smart enough to handle both (e.g., inherit the title whenever the link text isn't specifically overridden?), but editing the link text is what Media supports now and it seems to me like the most important case to continue supporting.

This sounds nice, but maybe that functionality could be a feature request and we could get this current bug fix committed.

I'm not great with regex so I didn't RTBC since I couldn't provide any clarity on #12.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Needs work

i am not a regex expert as well, fully agreed with #13 and marking needs work per #12

bneil’s picture

bneil commented
Issue tags: +7.x-2.0 alpha blocker

I think it's a good idea to shore this up before releasing the alpha. I'll create a feature request for inheriting the title.

bneil’s picture

bneil commented

Created followup feature request based on #11 per #14/#15: #1984454: Automatically have link text inherit from the title

Paul Lomax’s picture

Paul Lomax commented

([^"']+ )?
Is the space necessary? I think we might be able to simplify this to:
([^"']+)?

This is to avoid double spaces in the output, the only benefit of removing it would be to get a match even if it is the last attribute in the tag. But as the data-file_info attribute is last its not an issue. Better to not have double spaces.

media-element[^>]*>
media-element[^>]*?>
An inconsistency here, but I think the latter is more appropriate.

The ? is faster, as it makes it non greedy.

([\S\s]+?)
This must be some sort of trick that I don't understand, because I think this is the same as:
(.+?)

. Matches any character, except for line breaks if dotall is false.
\s Matches any whitespace character (spaces, tabs, line breaks).
\S Matches any character that is not a whitespace character (spaces, tabs, line breaks).

\S\s covers all bases really.

Paul Lomax’s picture

Paul Lomax commented
StatusFileSize
new media-wysiwyg-document-support-1942688-17.patch717 bytes

Slightly revised regex from comments in #12

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review

@Paul Lomax: it would be great to have a comment like that in the js file, explaining the regex:)
would you add one, please? :)

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented

I broke the long regex into pieces and added some comments explaining it.

This is to avoid double spaces in the output,

This regex is only used for finding the whole token, not for replacing/extracting any parts of it, so we don't even need groups. Taking those out helps readability.

I did a bit more banging away at this and I think this is pretty solid.

I agree that we could have a follow-up patch to handle the case where link_text matches the original filename.

I also agree that trying to deal with any edits beyond just the link_text is outside of scope. IMO if the user is changing it to that degree then they do in fact want something completely custom and we don't need to try to maintain the link to the media library.

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
StatusFileSize
new media-wysiwyg-document-support-1942688-19.patch2.95 KB

Whoops, here's the patch.

David_Rothstein’s picture

David_Rothstein commented
StatusFileSize
new media-wysiwyg-document-support-1942688-22.patch2.95 KB 
+    var classRegex = 'class=[\'"][^\'"]+?media-element';

This will fail for the basic case where there are no characters between the quote mark and class name (e.g., class="media-element").

I discovered that because I noticed things weren't working correctly for images anymore with this patch applied (documents did work correctly, because they happened to have another class name in the list which appeared before "media-element").

Switching the + to * seems to fix it, as in the attached patch.

thekevinday’s picture

thekevinday commented
Status: Needs review » Reviewed & tested by the community

Don't forge to turn off js aggregation (like me).
I thought it wasn't working at first..

The patch from #22 works for me.

aaron’s picture

aaron commented
Status: Reviewed & tested by the community » Fixed

Thanks for the great work everyone. I have committed this patch in #22.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

David_Rothstein commented

Interesting followup issue: #2028231: Embedded documents in the WYSIWYG can be very hard to delete

David_Rothstein’s picture

David_Rothstein commented

Another followup: #2034387: Link text of embedded documents is double encoded