From 65481858c2bb6e12275b5b1389bb4b922addb86d Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 23 Jan 2020 09:41:39 +1000 Subject: [PATCH] FEATURE: Use upload:// short URL for videos and audio in composer (#8760) For consistency this PR introduces using custom markdown and short upload:// URLs for video and audio uploads, rather than just treating them as links and relying on the oneboxer. The markdown syntax for videos is ![file text|video](upload://123456.mp4) and for audio it is ![file text|audio](upload://123456.mp3). This is achieved in discourse-markdown-it by modifying the rules for images in mardown-it via md.renderer.rules.image. We return HTML instead of the token when we encounter audio or video after | and the preview renders that HTML. Also when uploading an audio or video file we insert the relevant markdown into the composer. --- .../javascripts/discourse/lib/uploads.js.es6 | 66 +++++++++-------- .../engines/discourse-markdown-it.js.es6 | 40 +++++++++-- .../discourse-markdown/upload-protocol.js.es6 | 13 +++- .../pretty-text/upload-short-url.js.es6 | 70 ++++++++++++------- .../pretty-text/white-lister.js.es6 | 9 +++ lib/oneboxer.rb | 5 +- test/javascripts/lib/pretty-text-test.js.es6 | 22 ++++++ .../lib/upload-short-url-test.js.es6 | 27 ++++++- test/javascripts/lib/uploads-test.js.es6 | 14 ++-- 9 files changed, 190 insertions(+), 76 deletions(-) diff --git a/app/assets/javascripts/discourse/lib/uploads.js.es6 b/app/assets/javascripts/discourse/lib/uploads.js.es6 index 5a0e81779b0..0f12b3aa497 100644 --- a/app/assets/javascripts/discourse/lib/uploads.js.es6 +++ b/app/assets/javascripts/discourse/lib/uploads.js.es6 @@ -6,7 +6,7 @@ function isGUID(value) { ); } -export function imageNameFromFileName(fileName) { +export function markdownNameFromFileName(fileName) { let name = fileName.substr(0, fileName.lastIndexOf(".")); if (isAppleDevice() && isGUID(name)) { @@ -68,7 +68,7 @@ function validateUploadedFile(file, opts) { } if (opts.imagesOnly) { - if (!isAnImage(name) && !isAuthorizedImage(name, staff)) { + if (!isImage(name) && !isAuthorizedImage(name, staff)) { bootbox.alert( I18n.t("post.errors.upload_not_authorized", { authorized_extensions: authorizedImagesExtensions(staff) @@ -193,12 +193,20 @@ export function authorizesOneOrMoreImageExtensions(staff) { return imagesExtensions(staff).length > 0; } -export function isAnImage(path) { +export function isImage(path) { return /\.(png|jpe?g|gif|svg|ico)$/i.test(path); } +export function isVideo(path) { + return /\.(mov|mp4|webm|m4v|3gp|ogv|avi|mpeg|ogv)$/i.test(path); +} + +export function isAudio(path) { + return /\.(mp3|og[ga]|opus|wav|m4[abpr]|aac|flac)$/i.test(path); +} + function uploadTypeFromFileName(fileName) { - return isAnImage(fileName) ? "image" : "attachment"; + return isImage(fileName) ? "image" : "attachment"; } export function allowsImages(staff) { @@ -220,37 +228,33 @@ export function uploadIcon(staff) { return allowsAttachments(staff) ? "upload" : "far-image"; } -function uploadLocation(url) { - if (Discourse.CDN) { - url = Discourse.getURLWithCDN(url); - return /^\/\//.test(url) ? "http:" + url : url; - } else if (Discourse.S3BaseUrl) { - if (url.indexOf("secure-media-uploads") === -1) { - return "https:" + url; - } - return window.location.protocol + url; - } else { - var protocol = window.location.protocol + "//", - hostname = window.location.hostname, - port = window.location.port ? ":" + window.location.port : ""; - return protocol + hostname + port + url; - } +function imageMarkdown(upload) { + return `![${markdownNameFromFileName(upload.original_filename)}|${ + upload.thumbnail_width + }x${upload.thumbnail_height}](${upload.short_url || upload.url})`; +} + +function playableMediaMarkdown(upload, type) { + return `![${markdownNameFromFileName(upload.original_filename)}|${type}](${ + upload.short_url + })`; +} + +function attachmentMarkdown(upload) { + return `[${upload.original_filename}|attachment](${ + upload.short_url + }) (${I18n.toHumanSize(upload.filesize)})`; } export function getUploadMarkdown(upload) { - if (isAnImage(upload.original_filename)) { - const name = imageNameFromFileName(upload.original_filename); - return `![${name}|${upload.thumbnail_width}x${ - upload.thumbnail_height - }](${upload.short_url || upload.url})`; - } else if ( - /\.(mov|mp4|webm|ogv|mp3|ogg|wav|m4a)$/i.test(upload.original_filename) - ) { - return uploadLocation(upload.url); + if (isImage(upload.original_filename)) { + return imageMarkdown(upload); + } else if (isAudio(upload.original_filename)) { + return playableMediaMarkdown(upload, "audio"); + } else if (isVideo(upload.original_filename)) { + return playableMediaMarkdown(upload, "video"); } else { - return `[${upload.original_filename}|attachment](${ - upload.short_url - }) (${I18n.toHumanSize(upload.filesize)})`; + return attachmentMarkdown(upload); } } diff --git a/app/assets/javascripts/pretty-text/engines/discourse-markdown-it.js.es6 b/app/assets/javascripts/pretty-text/engines/discourse-markdown-it.js.es6 index eb480233973..81bfdf033a0 100644 --- a/app/assets/javascripts/pretty-text/engines/discourse-markdown-it.js.es6 +++ b/app/assets/javascripts/pretty-text/engines/discourse-markdown-it.js.es6 @@ -139,14 +139,43 @@ export function extractDataAttribute(str) { return [key, value]; } +// videoHTML and audioHTML follow the same HTML syntax +// as oneboxer.rb when dealing with these formats +function videoHTML(token) { + const src = token.attrGet("src"); + const origSrc = token.attrGet("data-orig-src"); + return ``; +} + +function audioHTML(token) { + const src = token.attrGet("src"); + const origSrc = token.attrGet("data-orig-src"); + return ``; +} + const IMG_SIZE_REGEX = /^([1-9]+[0-9]*)x([1-9]+[0-9]*)(\s*,\s*(x?)([1-9][0-9]{0,2}?)([%x]?))?$/; -function renderImage(tokens, idx, options, env, slf) { +function renderImageOrPlayableMedia(tokens, idx, options, env, slf) { const token = tokens[idx]; const alt = slf.renderInlineAsText(token.children, options, env); const split = alt.split("|"); const altSplit = []; + // markdown-it supports returning HTML instead of continuing to render the current token + // see https://github.com/markdown-it/markdown-it/blob/master/docs/architecture.md#renderer + if (split[1] === "video") { + return videoHTML(token); + } else if (split[1] === "audio") { + return audioHTML(token); + } + + // parsing ![myimage|500x300]() or ![myimage|75%]() or ![myimage|500x300, 75%] for (let i = 0, match, data; i < split.length; ++i) { if ((match = split[i].match(IMG_SIZE_REGEX)) && match[1] && match[2]) { let width = match[1]; @@ -194,8 +223,11 @@ function renderImage(tokens, idx, options, env, slf) { return slf.renderToken(tokens, idx, options); } -function setupImageDimensions(md) { - md.renderer.rules.image = renderImage; +// we have taken over the ![]() syntax in markdown to +// be able to render a video or audio URL as well as the +// image using |video and |audio in the text inside [] +function setupImageAndPlayableMediaRenderer(md) { + md.renderer.rules.image = renderImageOrPlayableMedia; } function renderAttachment(tokens, idx, options, env, slf) { @@ -319,7 +351,7 @@ export function setup(opts, siteSettings, state) { setupUrlDecoding(opts.engine); setupHoister(opts.engine); - setupImageDimensions(opts.engine); + setupImageAndPlayableMediaRenderer(opts.engine); setupAttachments(opts.engine); setupBlockBBCode(opts.engine); setupInlineBBCode(opts.engine); diff --git a/app/assets/javascripts/pretty-text/engines/discourse-markdown/upload-protocol.js.es6 b/app/assets/javascripts/pretty-text/engines/discourse-markdown/upload-protocol.js.es6 index 7790998e026..3e075b13ac7 100644 --- a/app/assets/javascripts/pretty-text/engines/discourse-markdown/upload-protocol.js.es6 +++ b/app/assets/javascripts/pretty-text/engines/discourse-markdown/upload-protocol.js.es6 @@ -44,9 +44,16 @@ function rule(state) { token.attrs[srcIndex][1] = mapped.url; token.attrs.push(["data-base62-sha1", mapped.base62_sha1]); } else { - token.attrs[srcIndex][1] = state.md.options.discourse.getURL( - "/images/transparent.png" - ); + // no point putting a transparent .png for audio/video + if (token.content.match(/\|video|\|audio/)) { + token.attrs[srcIndex][1] = state.md.options.discourse.getURL( + "/404" + ); + } else { + token.attrs[srcIndex][1] = state.md.options.discourse.getURL( + "/images/transparent.png" + ); + } token.attrs.push(["data-orig-src", origSrc]); } diff --git a/app/assets/javascripts/pretty-text/upload-short-url.js.es6 b/app/assets/javascripts/pretty-text/upload-short-url.js.es6 index 9201a12b2a8..b82e524173a 100644 --- a/app/assets/javascripts/pretty-text/upload-short-url.js.es6 +++ b/app/assets/javascripts/pretty-text/upload-short-url.js.es6 @@ -39,43 +39,58 @@ export function resetCache() { _cache = {}; } -function _loadCachedShortUrls($uploads) { - $uploads.each((idx, upload) => { - const $upload = $(upload); - let url; +function retrieveCachedUrl($upload, dataAttribute, callback) { + const cachedUpload = lookupCachedUploadUrl($upload.data(dataAttribute)); + const url = + dataAttribute === "orig-href" ? cachedUpload.short_path : cachedUpload.url; + if (url) { + $upload.removeAttr(`data-${dataAttribute}`); + if (url !== MISSING) { + callback(url); + } + } +} + +function _loadCachedShortUrls($uploads) { + $uploads.each((_idx, upload) => { + const $upload = $(upload); switch (upload.tagName) { case "A": - url = lookupCachedUploadUrl($upload.data("orig-href")).short_path; + retrieveCachedUrl($upload, "orig-href", url => { + $upload.attr("href", url); - if (url) { - $upload.removeAttr("data-orig-href"); - - if (url !== MISSING) { - $upload.attr("href", url); - - // Replace "|attachment" with class='attachment' - // TODO: This is a part of the cooking process now and should be - // removed in the future. - const content = $upload.text().split("|"); - if (content[1] === ATTACHMENT_CSS_CLASS) { - $upload.addClass(ATTACHMENT_CSS_CLASS); - $upload.text(content[0]); - } + // Replace "|attachment" with class='attachment' + // TODO: This is a part of the cooking process now and should be + // removed in the future. + const content = $upload.text().split("|"); + if (content[1] === ATTACHMENT_CSS_CLASS) { + $upload.addClass(ATTACHMENT_CSS_CLASS); + $upload.text(content[0]); } - } + }); break; case "IMG": - url = lookupCachedUploadUrl($upload.data("orig-src")).url; + retrieveCachedUrl($upload, "orig-src", url => { + $upload.attr("src", url); + }); - if (url) { - $upload.removeAttr("data-orig-src"); + break; + case "SOURCE": // video tag > source tag + retrieveCachedUrl($upload, "orig-src", url => { + $upload.attr("src", url); - if (url !== MISSING) { - $upload.attr("src", url); + if (url.startsWith(`//${window.location.host}`)) { + let hostRegex = new RegExp("//" + window.location.host, "g"); + url = url.replace(hostRegex, ""); } - } + $upload.attr("src", window.location.origin + url); + + // this is necessary, otherwise because of the src change the + // video just doesn't bother loading! + $upload.parent()[0].load(); + }); break; } @@ -94,7 +109,8 @@ function _loadShortUrls($uploads, ajax) { } export function resolveAllShortUrls(ajax) { - const attributes = "img[data-orig-src], a[data-orig-href]"; + const attributes = + "img[data-orig-src], a[data-orig-href], source[data-orig-src]"; let $shortUploadUrls = $(attributes); if ($shortUploadUrls.length > 0) { diff --git a/app/assets/javascripts/pretty-text/white-lister.js.es6 b/app/assets/javascripts/pretty-text/white-lister.js.es6 index 2fec9b4f437..fa711f07f5f 100644 --- a/app/assets/javascripts/pretty-text/white-lister.js.es6 +++ b/app/assets/javascripts/pretty-text/white-lister.js.es6 @@ -132,6 +132,8 @@ export const DEFAULT_LIST = [ "abbr[title]", "aside.quote", "aside[data-*]", + "audio", + "audio[controls]", "b", "big", "blockquote", @@ -192,7 +194,14 @@ export const DEFAULT_LIST = [ "strong", "sub", "sup", + "source[src]", + "source[data-orig-src]", + "source[type]", "ul", + "video", + "video[height]", + "video[width]", + "video[controls]", "ruby", "ruby[lang]", "rb", diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index 6575bc98229..1c2e9998944 100644 --- a/lib/oneboxer.rb +++ b/lib/oneboxer.rb @@ -7,7 +7,7 @@ Dir["#{Rails.root}/lib/onebox/engine/*_onebox.rb"].sort.each { |f| require f } module Oneboxer ONEBOX_CSS_CLASS = "onebox" AUDIO_REGEX = /^\.(mp3|og[ga]|opus|wav|m4[abpr]|aac|flac)$/i - VIDEO_REGEX = /^\.(mov|mp4|m4v|webm|ogv|3gp)$/i + VIDEO_REGEX = /^\.(mov|mp4|webm|m4v|3gp|ogv|avi|mpeg|ogv)$/i # keep reloaders happy unless defined? Oneboxer::Result @@ -195,8 +195,7 @@ module Oneboxer
HTML diff --git a/test/javascripts/lib/pretty-text-test.js.es6 b/test/javascripts/lib/pretty-text-test.js.es6 index fe72dbec0a2..8178ad79959 100644 --- a/test/javascripts/lib/pretty-text-test.js.es6 +++ b/test/javascripts/lib/pretty-text-test.js.es6 @@ -973,6 +973,28 @@ QUnit.test("images", assert => { ); }); +QUnit.test("video", assert => { + assert.cooked( + "![baby shark|video](upload://eyPnj7UzkU0AkGkx2dx8G4YM1Jx.mp4)", + `

`, + "It returns the correct video player HTML" + ); +}); + +QUnit.test("audio", assert => { + assert.cooked( + "![young americans|audio](upload://eyPnj7UzkU0AkGkx2dx8G4YM1Jx.mp3)", + `

`, + "It returns the correct audio player HTML" + ); +}); + QUnit.test("censoring", assert => { assert.cookedOptions( "Pleased to meet you, but pleeeease call me later, xyz123", diff --git a/test/javascripts/lib/upload-short-url-test.js.es6 b/test/javascripts/lib/upload-short-url-test.js.es6 index c18ae0ff9af..f969e648789 100644 --- a/test/javascripts/lib/upload-short-url-test.js.es6 +++ b/test/javascripts/lib/upload-short-url-test.js.es6 @@ -33,9 +33,22 @@ QUnit.module("lib:pretty-text/upload-short-url", { } ]; + const otherMediaSrcs = [ + { + short_url: "upload://d.mp4", + url: "/uploads/default/original/3X/c/b/4.mp4", + short_path: "/uploads/short-url/d.mp4" + }, + { + short_url: "upload://e.mp3", + url: "/uploads/default/original/3X/c/b/5.mp3", + short_path: "/uploads/short-url/e.mp3" + } + ]; + // prettier-ignore server.post("/uploads/lookup-urls", () => { //eslint-disable-line - return response(imageSrcs.concat(attachmentSrcs)); + return response(imageSrcs.concat(attachmentSrcs.concat(otherMediaSrcs))); }); fixture().html( @@ -79,4 +92,16 @@ QUnit.test("resolveAllShortUrls", async assert => { url: "/uploads/default/original/3X/c/b/3.pdf", short_path: "/uploads/short-url/c.pdf" }); + + lookup = lookupCachedUploadUrl("upload://d.mp4"); + assert.deepEqual(lookup, { + url: "/uploads/default/original/3X/c/b/4.mp4", + short_path: "/uploads/short-url/d.mp4" + }); + + lookup = lookupCachedUploadUrl("upload://e.mp3"); + assert.deepEqual(lookup, { + url: "/uploads/default/original/3X/c/b/5.mp3", + short_path: "/uploads/short-url/e.mp3" + }); }); diff --git a/test/javascripts/lib/uploads-test.js.es6 b/test/javascripts/lib/uploads-test.js.es6 index 0b6bb937654..1dbf5fde3f1 100644 --- a/test/javascripts/lib/uploads-test.js.es6 +++ b/test/javascripts/lib/uploads-test.js.es6 @@ -1,7 +1,7 @@ import { validateUploadedFiles, authorizedExtensions, - isAnImage, + isImage, allowsImages, allowsAttachments, getUploadMarkdown @@ -122,18 +122,18 @@ QUnit.test("allows valid uploads to go through", assert => { assert.not(bootbox.alert.calledOnce); }); -QUnit.test("isAnImage", assert => { +QUnit.test("isImage", assert => { ["png", "jpg", "jpeg", "gif", "ico"].forEach(extension => { var image = "image." + extension; - assert.ok(isAnImage(image), image + " is recognized as an image"); + assert.ok(isImage(image), image + " is recognized as an image"); assert.ok( - isAnImage("http://foo.bar/path/to/" + image), + isImage("http://foo.bar/path/to/" + image), image + " is recognized as an image" ); }); - assert.not(isAnImage("file.txt")); - assert.not(isAnImage("http://foo.bar/path/to/file.txt")); - assert.not(isAnImage("")); + assert.not(isImage("file.txt")); + assert.not(isImage("http://foo.bar/path/to/file.txt")); + assert.not(isImage("")); }); QUnit.test("allowsImages", assert => {