From b1d48f0cfbf520172a1b410d7e95a6d1becf9da8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frithjof=20Klo=CC=88s?= Date: Tue, 9 Apr 2019 17:46:01 +0200 Subject: [PATCH 1/5] PWA-1774 Extended the MediaProviders to responsify embedded videos --- .../media-providers/MediaProvider.js | 69 +++++++++++++++++ .../media-providers/MediaProvider.spec.js | 76 +++++++++++++++++++ .../collections/media-providers/Vimeo.js | 30 ++++---- .../collections/media-providers/Vimeo.spec.js | 7 ++ .../collections/media-providers/YouTube.js | 31 +++----- .../media-providers/YouTube.spec.js | 9 ++- .../collections/media-providers/index.js | 1 + .../collections/media-providers/style.js | 16 ++++ yarn.lock | 5 ++ 9 files changed, 208 insertions(+), 36 deletions(-) create mode 100644 libraries/common/collections/media-providers/MediaProvider.js create mode 100644 libraries/common/collections/media-providers/MediaProvider.spec.js create mode 100644 libraries/common/collections/media-providers/style.js diff --git a/libraries/common/collections/media-providers/MediaProvider.js b/libraries/common/collections/media-providers/MediaProvider.js new file mode 100644 index 0000000000..e0c76d346a --- /dev/null +++ b/libraries/common/collections/media-providers/MediaProvider.js @@ -0,0 +1,69 @@ +import { logger } from '@shopgate/pwa-core/helpers'; +import styles from './style'; + +/** + * The MediaProvider base class. + */ +class MediaProvider { + /** + * Constructor. + */ + constructor() { + this.containers = new Map(); + } + + /** + * Optimizes video container to make it responsive. + * @param {NodeList} container A DOM container. + * @private + * @returns {MediaProvider} + */ + responsify(container) { + // Remove fixed dimensions from the container. + container.setAttribute('height', ''); + container.setAttribute('width', ''); + + // Create the wrapper and apply styling. + const wrapper = document.createElement('div'); + + wrapper.className = styles; + + // Add the wrapper right before the container into the DOM. + container.parentNode.insertBefore(wrapper, container); + // Move the container into the wrapper. + wrapper.appendChild(container); + + return this; + } + + /** + * Add a DOM container with embedded videos. + * @param {NodeList} container A DOM container. + * @returns {MediaProvider} + */ + add() { + logger.error('MediaProvider.add() needs to be implemented within an inheriting class'); + return this; + } + + /** + * Remove a DOM container. + * @param {NodeList} container A DOM container. + * @returns {MediaProvider} + */ + remove(container) { + this.containers.delete(container); + return this; + } + + /** + * Stops all playing videos within the DOM containers. + * @returns {MediaProvider} + */ + stop() { + logger.error('MediaProvider.stop() needs to be implemented within an inheriting class'); + return this; + } +} + +export default MediaProvider; diff --git a/libraries/common/collections/media-providers/MediaProvider.spec.js b/libraries/common/collections/media-providers/MediaProvider.spec.js new file mode 100644 index 0000000000..97acb30815 --- /dev/null +++ b/libraries/common/collections/media-providers/MediaProvider.spec.js @@ -0,0 +1,76 @@ +import { JSDOM } from 'jsdom'; +import { logger } from '@shopgate/pwa-core/helpers'; +import MediaProvider from './MediaProvider'; + +jest.mock('@shopgate/pwa-core/helpers', () => ({ + logger: { + error: jest.fn(), + }, +})); + +/** + * Creates a DOM container with iframes. + * @param {Array} srcs A list of video URLs. + * @return {Object} + */ +const createContainer = (srcs) => { + const html = srcs.map(src => ``).join(''); + return new JSDOM(html).window.document; +}; + +describe('MediaProvider', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('.constructor()', () => { + it('should construct as expected', () => { + const instance = new MediaProvider(); + expect(instance.containers).toBeInstanceOf(Map); + expect(Array.from(instance.containers)).toHaveLength(0); + }); + }); + + describe('.add()', () => { + it('should log an error when it is not overwritten', () => { + const instance = new MediaProvider(); + instance.add(); + expect(logger.error).toHaveBeenCalledTimes(1); + }); + }); + + describe('.stop()', () => { + it('should log an error when it is not overwritten', () => { + const instance = new MediaProvider(); + instance.stop(); + expect(logger.error).toHaveBeenCalledTimes(1); + }); + }); + + describe('.remove()', () => { + it('should remove containers as expected', () => { + const instance = new MediaProvider(); + const containerOne = createContainer(['http://www.provider-one.com/video']); + const containerTwo = createContainer(['http://www.provider-two.com/video']); + + instance.containers.set(containerOne, containerOne); + instance.containers.set(containerTwo, containerTwo); + + instance.remove(containerTwo); + + expect(instance.containers.size).toBe(1); + expect(instance.containers.get(containerTwo)).toBeUndefined(); + expect(instance.containers.get(containerOne)).toEqual(containerOne); + + instance.remove(containerOne); + + expect(instance.containers.size).toBe(0); + }); + }); + + describe.skip('.responsify()', () => { + it('should optimize a container to be responsive', () => { + // TODO: Implement the test when a solution for the insertBefore issue was found. + }); + }); +}); diff --git a/libraries/common/collections/media-providers/Vimeo.js b/libraries/common/collections/media-providers/Vimeo.js index 7960285d60..058e1c48dd 100644 --- a/libraries/common/collections/media-providers/Vimeo.js +++ b/libraries/common/collections/media-providers/Vimeo.js @@ -1,14 +1,14 @@ -/* eslint-disable extra-rules/potential-point-free */ +import MediaProvider from './MediaProvider'; /** * The Vimeo media provider class. */ -class VimeoMediaProvider { +class VimeoMediaProvider extends MediaProvider { /** * Constructor. */ constructor() { - this.containers = new Map(); + super(); this.playerReady = false; this.deferred = []; @@ -17,7 +17,8 @@ class VimeoMediaProvider { /** * Checks if the Video player script is already available. - * If not, it injects it into the DOM and adds defferred containers. + * If not, it injects it into the DOM and adds deferred containers. + * @private */ checkPlayer() { if (typeof window.Vimeo !== 'undefined') { @@ -42,39 +43,38 @@ class VimeoMediaProvider { /** * Add a DOM container with embedded videos. + * @override * @param {NodeList} container A DOM container. + * @returns {VimeoMediaProvider} */ add(container) { if (!this.playerReady) { this.deferred.push(container); - return; + return this; } const iframes = container.querySelectorAll('iframe[src*="vimeo.com"]'); if (!iframes.length) { - return; + return this; } const players = []; iframes.forEach((iframe) => { + this.responsify(iframe); players.push(new window.Vimeo.Player(iframe)); }); this.containers.set(container, players); - } - /** - * Remove a DOM container. - * @param {NodeList} container A DOM container. - */ - remove(container) { - this.containers.delete(container); + return this; } /** * Stops all playing videos within the DOM containers. + * @override + * @returns {VimeoMediaProvider} */ stop() { this.containers.forEach((players) => { @@ -82,9 +82,9 @@ class VimeoMediaProvider { player.pause(); }); }); + + return this; } } -/* eslint-enable extra-rules/potential-point-free */ - export default VimeoMediaProvider; diff --git a/libraries/common/collections/media-providers/Vimeo.spec.js b/libraries/common/collections/media-providers/Vimeo.spec.js index 830b410b09..a248b9ed8d 100644 --- a/libraries/common/collections/media-providers/Vimeo.spec.js +++ b/libraries/common/collections/media-providers/Vimeo.spec.js @@ -49,6 +49,9 @@ describe('Vimeo media provider', () => { document.getElementsByTagName('html')[0].innerHTML = ''; instance = new Vimeo(); + // TODO Implement tests for the method when a solution for the insertBefore issue was found. + instance.responsify = jest.fn(); + playerScript = document.querySelector('script[src*="vimeo.com"]'); }; @@ -91,6 +94,8 @@ describe('Vimeo media provider', () => { it('should add multiple containers as expected', () => { const containerOne = createContainer([videos[0]]); const containerTwo = createContainer([videos[1]]); + const iframesOne = containerOne.querySelectorAll('iframe'); + const iframesTwo = containerTwo.querySelectorAll('iframe'); instance.add(containerOne); instance.add(containerTwo); @@ -99,6 +104,8 @@ describe('Vimeo media provider', () => { expect(instance.containers.size).toBe(2); expect(instance.containers.get(containerOne)).toEqual([expect.any(window.Vimeo.Player)]); expect(instance.containers.get(containerTwo)).toEqual([expect.any(window.Vimeo.Player)]); + expect(instance.responsify).toHaveBeenCalledWith(iframesOne[0]); + expect(instance.responsify).toHaveBeenCalledWith(iframesTwo[0]); }); it('should defer addition of a container if the player is not ready', () => { diff --git a/libraries/common/collections/media-providers/YouTube.js b/libraries/common/collections/media-providers/YouTube.js index 58c99651ee..cceb62ef7f 100644 --- a/libraries/common/collections/media-providers/YouTube.js +++ b/libraries/common/collections/media-providers/YouTube.js @@ -1,31 +1,28 @@ -/* eslint-disable extra-rules/potential-point-free */ import URLSearchParams from 'url-search-params'; +import MediaProvider from './MediaProvider'; /** * The YouTube media provider class. */ -class YouTubeMediaProvider { - /** - * Constructor. - */ - constructor() { - this.containers = new Map(); - } - +class YouTubeMediaProvider extends MediaProvider { /** * Add a DOM container with embedded videos. + * @override * @param {NodeList} container A DOM container. + * @returns {YouTubeMediaProvider} */ add(container) { const iframes = container .querySelectorAll('iframe[src*="youtube.com"], iframe[src*="youtube-nocookie.com"]'); if (!iframes.length) { - return; + return this; } // Update the video urls to enable stopping videos via the event API. iframes.forEach((iframe, index) => { + this.responsify(iframe); + const { src } = iframe; const [url, query] = src.split('?'); @@ -40,18 +37,14 @@ class YouTubeMediaProvider { }); this.containers.set(container, iframes); - } - /** - * Remove a DOM container. - * @param {NodeList} container A DOM container. - */ - remove(container) { - this.containers.delete(container); + return this; } /** * Stops all playing videos within the DOM containers. + * @override + * @returns {YouTubeMediaProvider} */ stop() { this.containers.forEach((iframes) => { @@ -61,9 +54,9 @@ class YouTubeMediaProvider { } }); }); + + return this; } } -/* eslint-enable extra-rules/potential-point-free */ - export default YouTubeMediaProvider; diff --git a/libraries/common/collections/media-providers/YouTube.spec.js b/libraries/common/collections/media-providers/YouTube.spec.js index 363681b9ba..5f826a2494 100644 --- a/libraries/common/collections/media-providers/YouTube.spec.js +++ b/libraries/common/collections/media-providers/YouTube.spec.js @@ -15,7 +15,8 @@ const videos = [ * @return {Object} */ const createContainer = (srcs) => { - const html = srcs.map(src => ``).join(''); + const iframes = srcs.map(src => ``).join(''); + const html = `
${iframes}
`; return new JSDOM(html).window.document; }; @@ -24,6 +25,8 @@ describe('YouTube media provider', () => { beforeEach(() => { instance = new YouTube(); + // TODO Implement tests for the method when a solution for the insertBefore issue was found. + instance.responsify = jest.fn(); }); describe('.constructor()', () => { @@ -47,6 +50,8 @@ describe('YouTube media provider', () => { expect(instance.containers.size).toBe(2); expect(instance.containers.get(containerOne)).toEqual(iframesOne); expect(instance.containers.get(containerTwo)).toEqual(iframesTwo); + expect(instance.responsify).toHaveBeenCalledWith(iframesOne[0]); + expect(instance.responsify).toHaveBeenCalledWith(iframesTwo[0]); }); it('should add a container with different types of YouTube videos', () => { @@ -97,7 +102,7 @@ describe('YouTube media provider', () => { }); describe('.stop()', () => { - it('should stop the videos within mutiple containers', () => { + it('should stop the videos within multiple containers', () => { const postMessageMock = jest.fn(); const containerOne = createContainer([videos[0]]); const containerTwo = createContainer([videos[1], videos[2]]); diff --git a/libraries/common/collections/media-providers/index.js b/libraries/common/collections/media-providers/index.js index a23dbc869b..62ed103a7d 100644 --- a/libraries/common/collections/media-providers/index.js +++ b/libraries/common/collections/media-providers/index.js @@ -1,2 +1,3 @@ +export { default as MediaProvider } from './MediaProvider'; export { default as Vimeo } from './Vimeo'; export { default as YouTube } from './YouTube'; diff --git a/libraries/common/collections/media-providers/style.js b/libraries/common/collections/media-providers/style.js new file mode 100644 index 0000000000..9c34b6b292 --- /dev/null +++ b/libraries/common/collections/media-providers/style.js @@ -0,0 +1,16 @@ +import { css } from 'glamor'; + +export default css({ + position: 'relative', + height: 0, + overflow: 'hidden', + padding: '0 0 56.25% 0', + ' iframe, object, embed': { + position: 'absolute', + top: 0, + left: 0, + width: '100%', + height: '100%', + border: 0, + }, +}).toString(); diff --git a/yarn.lock b/yarn.lock index 23d9ea50b4..8d1dc955de 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2116,6 +2116,11 @@ compare-func@^1.3.1: array-ify "^1.0.0" dot-prop "^3.0.0" +compare-versions@^3.4.0: + version "3.4.0" + resolved "https://registry.yarnpkg.com/compare-versions/-/compare-versions-3.4.0.tgz#e0747df5c9cb7f054d6d3dc3e1dbc444f9e92b26" + integrity sha512-tK69D7oNXXqUW3ZNo/z7NXTEz22TCF0pTE+YF9cxvaAM9XnkLo1fV621xCLrRR6aevJlKxExkss0vWqUCUpqdg== + component-emitter@^1.2.1: version "1.2.1" resolved "https://registry.yarnpkg.com/component-emitter/-/component-emitter-1.2.1.tgz#137918d6d78283f7df7a6b7c5a63e140e69425e6" From 0e315e6c6616a42f5fc15cd34ae938a67216b58c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frithjof=20Klo=CC=88s?= Date: Tue, 9 Apr 2019 17:55:22 +0200 Subject: [PATCH 2/5] PWA-1774 Integrated the EmbeddedMedia collection into the HTMLSanitizer component - added processing of the target a-attribute to the HTMLSanitizer --- .../HtmlSanitizer/__snapshots__/spec.jsx.snap | 3 + .../common/components/HtmlSanitizer/index.jsx | 43 ++++++--- .../common/components/HtmlSanitizer/spec.jsx | 87 ++++++++++++++----- .../Description/__snapshots__/spec.jsx.snap | 1 + .../components/Description/connector.js | 5 +- .../Description/__snapshots__/spec.jsx.snap | 1 + .../components/Description/connector.js | 5 +- 7 files changed, 109 insertions(+), 36 deletions(-) diff --git a/libraries/common/components/HtmlSanitizer/__snapshots__/spec.jsx.snap b/libraries/common/components/HtmlSanitizer/__snapshots__/spec.jsx.snap index 72cb17760c..85493545e3 100644 --- a/libraries/common/components/HtmlSanitizer/__snapshots__/spec.jsx.snap +++ b/libraries/common/components/HtmlSanitizer/__snapshots__/spec.jsx.snap @@ -40,6 +40,8 @@ exports[` strips out images with relative paths 1`] = `
+ + strips out images with relative paths 1`] = ` exports[` strips out the script tags 1`] = `
{ - const aTag = event.target.closest('a'); + const linkTag = event.target.closest('a'); + + if (linkTag) { + const { + attributes: { + href: { value: href = '' } = {}, + target: { value: target = '' } = {}, + } = {}, + } = linkTag; - if (aTag && aTag.attributes.href) { - event.preventDefault(); - const href = aTag.attributes.href.value; - this.props.settings.handleClick(href); + if (href) { + event.preventDefault(); + this.props.settings.handleClick(href, target); + } } }; @@ -75,12 +93,17 @@ class HtmlSanitizer extends Component { __html: parseHTML( this.props.children, this.props.decode, - this.props.settings + this.props.settings, + this.props.processStyles ), }; return ( -
+
); } } diff --git a/libraries/common/components/HtmlSanitizer/spec.jsx b/libraries/common/components/HtmlSanitizer/spec.jsx index d5f5f62d8a..bfb21ab69a 100644 --- a/libraries/common/components/HtmlSanitizer/spec.jsx +++ b/libraries/common/components/HtmlSanitizer/spec.jsx @@ -1,11 +1,30 @@ import React from 'react'; import { mount } from 'enzyme'; import { JSDOM } from 'jsdom'; +import { embeddedMedia } from '@shopgate/pwa-common/collections'; import HtmlSanitizer from './index'; -const mockedHandleClick = jest.fn(); +jest.mock('@shopgate/pwa-common/collections/EmbeddedMedia', () => ({ + add: jest.fn(), + remove: jest.fn(), +})); + +/** + * @param {string} html HTML markup. + * @param {Object} props Component props. + * @returns {JSX} + */ +const createWrapper = (html, props = {}) => mount(( + + {html} + +)); describe('', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + it('should render the HtmlSanitizer', () => { /** * The value for html is the HTML-escaped equivalent of the following: @@ -14,36 +33,62 @@ describe('', () => { */ const html = '<h1>Hello World!</h1>'; - const wrapper = mount(( - - {html} - - )); + const wrapper = createWrapper(html, { decode: true }); // Test result of dangerouslySetInnerHTML. expect(wrapper.html()).toEqual('

Hello World!

'); expect(wrapper.render()).toMatchSnapshot(); }); + it('should add and remove handlers for embedded media', () => { + const wrapper = createWrapper('
', { decode: true }); + const ref = wrapper.instance().htmlContainer.current; + expect(embeddedMedia.add).toHaveBeenCalledTimes(1); + expect(embeddedMedia.add).toHaveBeenCalledWith(ref); + expect(embeddedMedia.remove).toHaveBeenCalledTimes(0); + + wrapper.setProps({ children: '' }); + expect(embeddedMedia.add).toHaveBeenCalledTimes(2); + expect(embeddedMedia.add).toHaveBeenCalledWith(ref); + expect(embeddedMedia.remove).toHaveBeenCalledTimes(0); + + wrapper.unmount(); + expect(embeddedMedia.add).toHaveBeenCalledTimes(2); + expect(embeddedMedia.remove).toHaveBeenCalledTimes(1); + expect(embeddedMedia.remove).toHaveBeenCalledWith(ref); + }); + it('strips out images with relative paths', () => { const html = ` `; - const wrapper = mount(( - - {html} - - )); + const wrapper = createWrapper(html); expect(wrapper.html()).not.toContain(''); expect(wrapper.render()).toMatchSnapshot(); }); + it('should move style blocks out of the content', () => { + const html = ` +
+ + + + +
+ `; + + const wrapper = createWrapper(html, { processStyles: true }); + expect(wrapper.html()).not.toContain('