From dc21c33690d0b8dd81d8f9a6c389ca9110ebd9eb Mon Sep 17 00:00:00 2001
From: Henric Trotzig <henric.trotzig@brigade.com>
Date: Sun, 10 Jun 2018 17:59:33 +0200
Subject: [PATCH] Fix grabbing urls from srcset attributes

My first attempt at this (312ecd9) was too naive, it didn't take urls
with commas in them into account. In this second attempt, I've tried to
tdd things so that I know we can handle a wider range of values.

I've used https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img
as a guide into figuring out possible values.
---
 .../src/__tests__/getUrlsFromSrcset-test.js   | 43 +++++++++++++++++++
 .../src/getUrlsFromSrcset.js                  | 10 +++++
 .../src/waitForImagesToRender.js              |  6 +--
 3 files changed, 55 insertions(+), 4 deletions(-)
 create mode 100644 packages/happo-target-firefox/src/__tests__/getUrlsFromSrcset-test.js
 create mode 100644 packages/happo-target-firefox/src/getUrlsFromSrcset.js

diff --git a/packages/happo-target-firefox/src/__tests__/getUrlsFromSrcset-test.js b/packages/happo-target-firefox/src/__tests__/getUrlsFromSrcset-test.js
new file mode 100644
index 0000000..1d4b43a
--- /dev/null
+++ b/packages/happo-target-firefox/src/__tests__/getUrlsFromSrcset-test.js
@@ -0,0 +1,43 @@
+import getUrlsFromSrcset from '../getUrlsFromSrcset';
+
+it('handles invalid values', () => {
+  expect(getUrlsFromSrcset('      ')).toEqual([]);
+});
+
+it('handles single urls without descriptor', () => {
+  expect(getUrlsFromSrcset('/foo.png')).toEqual(['/foo.png']);
+});
+
+it('handles single urls with commas', () => {
+  expect(getUrlsFromSrcset('/foo,40,50.png')).toEqual(['/foo,40,50.png']);
+});
+
+it('handles single urls with a width descriptor', () => {
+  expect(getUrlsFromSrcset('http://foo.com/foo.png 200w')).toEqual(['http://foo.com/foo.png']);
+});
+
+it('handles single urls with a pixel density descriptor', () => {
+  expect(getUrlsFromSrcset('http://foo.com/foo.png 1.2x')).toEqual(['http://foo.com/foo.png']);
+});
+
+it('strips whitespace', () => {
+  expect(getUrlsFromSrcset(`
+    /foo.png`)).toEqual(['/foo.png']);
+});
+
+it('handles multiple urls', () => {
+  expect(getUrlsFromSrcset('/foo.png 200w,/bar.png 400w')).toEqual(['/foo.png', '/bar.png']);
+});
+
+it('handles multiple urls with commas', () => {
+  expect(getUrlsFromSrcset('http://foo.com/foo,40,50 200w,/bar,20.png 400w')).toEqual(['http://foo.com/foo,40,50', '/bar,20.png']);
+});
+
+it('handles multiple urls with whitespace', () => {
+  expect(getUrlsFromSrcset(`
+  http://foo.com/foo,40,50 200w,
+  /bar,20.png         400w
+
+
+  `)).toEqual(['http://foo.com/foo,40,50', '/bar,20.png']);
+});
diff --git a/packages/happo-target-firefox/src/getUrlsFromSrcset.js b/packages/happo-target-firefox/src/getUrlsFromSrcset.js
new file mode 100644
index 0000000..00e1af6
--- /dev/null
+++ b/packages/happo-target-firefox/src/getUrlsFromSrcset.js
@@ -0,0 +1,10 @@
+const SRCSET_ITEM = /([^\s]+)(\s+[0-9.]+[wx])?(,?\s*)/g;
+
+export default function getUrlsFromSrcset(value) {
+  const result = [];
+  let match;
+  while (match = SRCSET_ITEM.exec(value)) { // eslint-disable-line no-cond-assign
+    result.push(match[1]);
+  }
+  return result;
+}
diff --git a/packages/happo-target-firefox/src/waitForImagesToRender.js b/packages/happo-target-firefox/src/waitForImagesToRender.js
index 40ee286..253d4e2 100644
--- a/packages/happo-target-firefox/src/waitForImagesToRender.js
+++ b/packages/happo-target-firefox/src/waitForImagesToRender.js
@@ -1,4 +1,5 @@
 import findBackgroundImageUrls from './findBackgroundImageUrls';
+import getUrlsFromSrcset from './getUrlsFromSrcset';
 
 export function waitForImageToLoad(url) {
   return new Promise((resolve, reject) => {
@@ -23,10 +24,7 @@ export default function waitForImagesToRender() {
         return;
       }
 
-      srcset.split(/,\s*/).forEach((entry) => {
-        const [url] = entry.split(' ');
-        promises.push(waitForImageToLoad(url));
-      });
+      promises.push(...getUrlsFromSrcset(srcset).map(waitForImageToLoad));
     });
 
     Array.prototype.slice.call(document.body.querySelectorAll('*'))