From 2443446e62bff9dd8a0493d10e5359a5e946a84e Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Thu, 12 Oct 2023 13:47:48 -0600 Subject: [PATCH] DEV: Prevent videos from preloading metadata (#23807) Preloading just metadata is not always respected by browsers, and sometimes the whole video will be downloaded. This switches to using a placeholder image for the video and only loads the video when the play button is clicked. --- .../instance-initializers/post-decorations.js | 47 ---------- .../video-placeholder.js | 92 +++++++++++++++++++ .../acceptance/video-placeholder-test.js | 27 ++++++ .../discourse/tests/fixtures/topic.js | 51 +++++++++- .../tests/unit/lib/pretty-text-test.js | 12 +-- .../pretty-text/addon/allow-lister.js | 1 + .../addon/engines/discourse-markdown-it.js | 27 ++++-- .../stylesheets/common/base/onebox.scss | 17 ++++ lib/pretty_text.rb | 5 +- .../blocked_hotlinked_media_spec.rb | 2 +- .../composer/category_templates_spec.rb | 4 +- 11 files changed, 215 insertions(+), 70 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/instance-initializers/video-placeholder.js create mode 100644 app/assets/javascripts/discourse/tests/acceptance/video-placeholder-test.js diff --git a/app/assets/javascripts/discourse/app/instance-initializers/post-decorations.js b/app/assets/javascripts/discourse/app/instance-initializers/post-decorations.js index 6fb319af0a1..10c7e8070f6 100644 --- a/app/assets/javascripts/discourse/app/instance-initializers/post-decorations.js +++ b/app/assets/javascripts/discourse/app/instance-initializers/post-decorations.js @@ -9,7 +9,6 @@ import { SELECTORS } from "discourse/lib/lightbox/constants"; import { withPluginApi } from "discourse/lib/plugin-api"; import { setTextDirections } from "discourse/lib/text-direction"; import { iconHTML, iconNode } from "discourse-common/lib/icon-library"; -import discourseLater from "discourse-common/lib/later"; import I18n from "I18n"; export default { @@ -82,30 +81,6 @@ export default { }); }); - const caps = owner.lookup("service:capabilities"); - if (caps.isSafari || caps.isIOS) { - api.decorateCookedElement( - (elem) => { - elem.querySelectorAll("video").forEach((video) => { - if (video.poster && video.poster !== "" && !video.autoplay) { - return; - } - - const source = video.querySelector("source"); - if (source) { - // In post-cooked.js, we create the video element in a detached DOM - // then adopt it into to the real DOM. - // This confuses safari, and preloading/autoplay do not happen. - - // Calling `.load()` tricks Safari into loading the video element correctly - source.parentElement.load(); - } - }); - }, - { afterAdopt: true, onlyStream: true } - ); - } - const oneboxTypes = { amazon: "discourse-amazon", githubactions: "fab-github", @@ -131,28 +106,6 @@ export default { }); }); - api.decorateCookedElement((element) => { - element - .querySelectorAll(".video-container") - .forEach((videoContainer) => { - const video = videoContainer.getElementsByTagName("video")[0]; - video.addEventListener("loadeddata", () => { - discourseLater(() => { - if (video.videoWidth === 0 || video.videoHeight === 0) { - const notice = document.createElement("div"); - notice.className = "notice"; - notice.innerHTML = - iconHTML("exclamation-triangle") + - " " + - I18n.t("cannot_render_video"); - - videoContainer.appendChild(notice); - } - }, 500); - }); - }); - }); - function _createButton() { const openPopupBtn = document.createElement("button"); openPopupBtn.classList.add( diff --git a/app/assets/javascripts/discourse/app/instance-initializers/video-placeholder.js b/app/assets/javascripts/discourse/app/instance-initializers/video-placeholder.js new file mode 100644 index 00000000000..17e3dc94f7d --- /dev/null +++ b/app/assets/javascripts/discourse/app/instance-initializers/video-placeholder.js @@ -0,0 +1,92 @@ +import { withPluginApi } from "discourse/lib/plugin-api"; +import { iconHTML } from "discourse-common/lib/icon-library"; +import discourseLater from "discourse-common/lib/later"; +import I18n from "I18n"; + +export default { + initialize(owner) { + withPluginApi("0.8.7", (api) => { + function handleVideoPlaceholderClick(helper, event) { + const parentDiv = event.target.closest(".video-placeholder-container"); + const wrapper = event.target.closest(".video-placeholder-wrapper"); + + const videoHTML = ` + `; + parentDiv.insertAdjacentHTML("beforeend", videoHTML); + parentDiv.classList.add("video-container"); + + const video = parentDiv.querySelector("video"); + + const caps = owner.lookup("service:capabilities"); + if (caps.isSafari || caps.isIOS) { + const source = video.querySelector("source"); + if (source) { + // In post-cooked.js, we create the video element in a detached DOM + // then adopt it into to the real DOM. + // This confuses safari, and preloading/autoplay do not happen. + + // Calling `.load()` tricks Safari into loading the video element correctly + source.parentElement.load(); + } + } + + video.addEventListener("loadeddata", () => { + discourseLater(() => { + if (video.videoWidth === 0 || video.videoHeight === 0) { + const notice = document.createElement("div"); + notice.className = "notice"; + notice.innerHTML = + iconHTML("exclamation-triangle") + + " " + + I18n.t("cannot_render_video"); + + parentDiv.appendChild(notice); + } + }, 500); + }); + + video.addEventListener("canplay", function () { + video.play(); + wrapper.remove(); + video.style.display = ""; + parentDiv.classList.remove("video-placeholder-container"); + }); + } + + function applyVideoPlaceholder(post, helper) { + if (!helper) { + return; + } + + const containers = post.querySelectorAll( + ".video-placeholder-container" + ); + + containers.forEach((container) => { + const wrapper = document.createElement("div"), + overlay = document.createElement("div"); + + wrapper.classList.add("video-placeholder-wrapper"); + container.appendChild(wrapper); + + overlay.classList.add("video-placeholder-overlay"); + overlay.style.cursor = "pointer"; + overlay.addEventListener( + "click", + handleVideoPlaceholderClick.bind(null, helper), + false + ); + overlay.innerHTML = `${iconHTML("play")}`; + wrapper.appendChild(overlay); + }); + } + + api.decorateCookedElement(applyVideoPlaceholder, { + onlyStream: true, + }); + }); + }, +}; diff --git a/app/assets/javascripts/discourse/tests/acceptance/video-placeholder-test.js b/app/assets/javascripts/discourse/tests/acceptance/video-placeholder-test.js new file mode 100644 index 00000000000..a814b5c4656 --- /dev/null +++ b/app/assets/javascripts/discourse/tests/acceptance/video-placeholder-test.js @@ -0,0 +1,27 @@ +import { click, visit } from "@ember/test-helpers"; +import { test } from "qunit"; +import { acceptance } from "discourse/tests/helpers/qunit-helpers"; + +acceptance("Video Placeholder Test", function () { + test("placeholder shows up on posts with videos", async function (assert) { + await visit("/t/54081"); + + const postWithVideo = document.querySelector( + ".video-placeholder-container" + ); + assert.ok( + postWithVideo.hasAttribute("data-video-src"), + "Video placeholder should have the 'data-video-src' attribute" + ); + + const overlay = postWithVideo.querySelector(".video-placeholder-overlay"); + + assert.dom("video").doesNotExist("The video element does not exist yet"); + + await click(overlay); + + assert.dom(".video-container").exists("The video container appears"); + + assert.dom("video").exists("The video element appears"); + }); +}); diff --git a/app/assets/javascripts/discourse/tests/fixtures/topic.js b/app/assets/javascripts/discourse/tests/fixtures/topic.js index e809fc44364..d6af54ef25a 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/topic.js +++ b/app/assets/javascripts/discourse/tests/fixtures/topic.js @@ -6887,6 +6887,55 @@ export default { can_view_edit_history: true, wiki: false, }, + { + id: 21, + username: "eviltrout", + avatar_template: "/images/avatar.png", + name: "Evil Trout", + uploaded_avatar_id: 9, + created_at: "2015-08-13T14:49:23.927Z", + cooked: + '
', + post_number: 4, + post_type: 1, + updated_at: "2015-08-13T14:49:23.927Z", + reply_count: 0, + reply_to_post_number: null, + quote_count: 0, + incoming_link_count: 0, + reads: 1, + score: 0, + yours: true, + topic_id: 9, + topic_slug: "this-is-a-test-topic", + display_username: "", + primary_group_name: null, + version: 1, + can_edit: true, + can_delete: true, + can_recover: true, + read: true, + user_title: null, + actions_summary: [ + { id: 3, can_act: true }, + { id: 4, can_act: true }, + { id: 5, hidden: true, can_act: true }, + { id: 7, can_act: true }, + { id: 8, can_act: true }, + ], + moderator: false, + admin: true, + staff: true, + user_id: 1, + hidden: false, + hidden_reason_id: null, + trust_level: 4, + deleted_at: null, + user_deleted: false, + edit_reason: null, + can_view_edit_history: true, + wiki: false, + }, ], stream: [398, 419], gaps: { before: {}, after: { 398: [419] } }, @@ -6894,7 +6943,7 @@ export default { id: 54081, title: "This is a topic with tables!", fancy_title: "This is a topic with tables!", - posts_count: 2, + posts_count: 4, created_at: "2013-02-05T21:29:00.174Z", views: 5211, reply_count: 1, diff --git a/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js b/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js index 03de7e574f6..27b301f559b 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/pretty-text-test.js @@ -1017,11 +1017,7 @@ eviltrout

test("video", function (assert) { assert.cooked( "![baby shark|video](upload://eyPnj7UzkU0AkGkx2dx8G4YM1Jx.mp4)", - `

- + `

`, "It returns the correct video player HTML" ); @@ -1043,11 +1039,7 @@ eviltrout

siteSettings: { secure_uploads: true }, lookupUploadUrls, }, - `

- + `

`, "It returns the correct video HTML when the URL is mapped with secure uploads, removing data-orig-src" ); diff --git a/app/assets/javascripts/pretty-text/addon/allow-lister.js b/app/assets/javascripts/pretty-text/addon/allow-lister.js index 8b3eb446332..bc0fb84bb0f 100644 --- a/app/assets/javascripts/pretty-text/addon/allow-lister.js +++ b/app/assets/javascripts/pretty-text/addon/allow-lister.js @@ -197,6 +197,7 @@ export const DEFAULT_LIST = [ "span.excerpt", "div.excerpt", "div.video-container", + "div.video-placeholder-container", "div.onebox-placeholder-container", "span.placeholder-icon video", "span.hashtag", diff --git a/app/assets/javascripts/pretty-text/addon/engines/discourse-markdown-it.js b/app/assets/javascripts/pretty-text/addon/engines/discourse-markdown-it.js index e3be815b0c1..9c45c66f15d 100644 --- a/app/assets/javascripts/pretty-text/addon/engines/discourse-markdown-it.js +++ b/app/assets/javascripts/pretty-text/addon/engines/discourse-markdown-it.js @@ -159,11 +159,7 @@ function videoHTML(token) { const src = token.attrGet("src"); const origSrc = token.attrGet("data-orig-src"); const dataOrigSrcAttr = origSrc !== null ? `data-orig-src="${origSrc}"` : ""; - return `
- + return `
`; } @@ -191,13 +187,26 @@ function renderImageOrPlayableMedia(tokens, idx, options, env, slf) { if (split[1] === "video") { if ( options.discourse.previewing && - !options.discourse.limitedSiteSettings.enableDiffhtmlPreview + options.discourse.limitedSiteSettings.enableDiffhtmlPreview ) { - return `
+ const src = token.attrGet("src"); + const origSrc = token.attrGet("data-orig-src"); + const dataOrigSrcAttr = + origSrc !== null ? `data-orig-src="${origSrc}"` : ""; + return `
+ +
`; + } else { + if (options.discourse.previewing) { + return `
`; - } else { - return videoHTML(token); + } else { + return videoHTML(token); + } } } else if (split[1] === "audio") { return audioHTML(token); diff --git a/app/assets/stylesheets/common/base/onebox.scss b/app/assets/stylesheets/common/base/onebox.scss index 2eecbfc1d29..2aa64ade0b4 100644 --- a/app/assets/stylesheets/common/base/onebox.scss +++ b/app/assets/stylesheets/common/base/onebox.scss @@ -919,6 +919,23 @@ aside.onebox.mixcloud-preview { height: 100%; } } +.video-placeholder-container { + position: relative; + padding: 0 0 56.25% 0; + width: 100%; + background-color: black; + + .video-placeholder-overlay { + position: absolute; + left: 50%; + top: 50%; + transform: translate(-50%, -50%); + transition: 150ms; + background-repeat: no-repeat; + background-position: center; + max-width: 30%; + } +} iframe.vimeo-onebox { width: 100%; diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index 7ac60602a7c..3b283bc01a4 100644 --- a/lib/pretty_text.rb +++ b/lib/pretty_text.rb @@ -342,11 +342,14 @@ module PrettyText allowed_pattern = allowed_src_pattern doc - .css("img[src], source[src], source[srcset], track[src]") + .css("img[src], source[src], source[srcset], track[src], div[data-video-src]") .each do |el| if el["src"] && !el["src"].match?(allowed_pattern) el[PrettyText::BLOCKED_HOTLINKED_SRC_ATTR] = el.delete("src") end + if el["data-video-src"] && !el["data-video-src"].match?(allowed_pattern) + el[PrettyText::BLOCKED_HOTLINKED_SRC_ATTR] = el["data-video-src"] + end if el["srcset"] srcs = el["srcset"].split(",").map { |e| e.split(" ", 2)[0].presence } diff --git a/spec/integration/blocked_hotlinked_media_spec.rb b/spec/integration/blocked_hotlinked_media_spec.rb index c9dfdc2a6ba..6b1f47f4f7c 100644 --- a/spec/integration/blocked_hotlinked_media_spec.rb +++ b/spec/integration/blocked_hotlinked_media_spec.rb @@ -48,7 +48,7 @@ RSpec.describe "hotlinked media blocking" do post = Fabricate(:post, raw: "![alt text|video](#{hotlinked_url})") expect(post.cooked).not_to have_tag("video source[src]") expect(post.cooked).to have_tag( - "video source", + "div", with: { PrettyText::BLOCKED_HOTLINKED_SRC_ATTR => hotlinked_url, }, diff --git a/spec/system/composer/category_templates_spec.rb b/spec/system/composer/category_templates_spec.rb index 819400c63d4..1de2173e719 100644 --- a/spec/system/composer/category_templates_spec.rb +++ b/spec/system/composer/category_templates_spec.rb @@ -325,7 +325,9 @@ describe "Composer Form Templates", type: :system do ) expect(find("#{topic_page.post_by_number_selector(1)} .cooked")).to have_css("a.attachment") expect(find("#{topic_page.post_by_number_selector(1)} .cooked")).to have_css("audio") - expect(find("#{topic_page.post_by_number_selector(1)} .cooked")).to have_css("video") + expect(find("#{topic_page.post_by_number_selector(1)} .cooked")).to have_css( + ".video-placeholder-container", + ) end it "shows labels and descriptions when a form template is assigned to the category" do