From 834838ff84a65bc9a4097841127d2eea3c374a1e Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 8 Feb 2019 17:44:23 -0600 Subject: [PATCH 1/4] core(font-display): allow missing semicolon --- lighthouse-core/audits/font-display.js | 4 ++- .../test/audits/font-display-test.js | 31 ++++++++++++++++--- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/audits/font-display.js b/lighthouse-core/audits/font-display.js index 2a1ce26099fe..d4a0e9b7ea2e 100644 --- a/lighthouse-core/audits/font-display.js +++ b/lighthouse-core/audits/font-display.js @@ -57,7 +57,9 @@ class FontDisplay extends Audit { const fontFaceDeclarations = newlinesStripped.match(/@font-face\s*{(.*?)}/g) || []; // Go through all the @font-face declarations to find a declared `font-display: ` property for (const declaration of fontFaceDeclarations) { - const rawFontDisplay = declaration.match(/font-display:(.*?);/); + // Find the font-display value by matching a single token, optionally surrounded by whitespace, + // followed either by a semicolon or the end of a block. + const rawFontDisplay = declaration.match(/font-display:(\s*\S+\s*)(;|\s*})/); // If they didn't have a font-display property, it's the default, and it's failing; bail if (!rawFontDisplay) continue; // If they don't have one of the passing font-display values, it's failing; bail diff --git a/lighthouse-core/test/audits/font-display-test.js b/lighthouse-core/test/audits/font-display-test.js index 6a9dadd0d7c2..215bb3b05f81 100644 --- a/lighthouse-core/test/audits/font-display-test.js +++ b/lighthouse-core/test/audits/font-display-test.js @@ -42,6 +42,7 @@ describe('Performance: Font Display audit', () => { } @font-face { + font-display: 'optional' // missing a semi-colon, should still fail for being invalid block /* try no path with no quotes ' */ src: url(font.woff); } @@ -72,7 +73,7 @@ describe('Performance: Font Display audit', () => { {url: networkRecords[2].url, wastedMs: 1000}, ]; assert.strictEqual(result.rawValue, false); - assert.deepEqual(result.details.items, items); + expect(result.details.items).toEqual(items); }); it('resolves URLs relative to stylesheet URL when available', async () => { @@ -128,7 +129,7 @@ describe('Performance: Font Display audit', () => { const result = await FontDisplayAudit.audit(getArtifacts(), context); assert.strictEqual(result.rawValue, true); - assert.deepEqual(result.details.items, []); + expect(result.details.items).toEqual([]); }); it('passes when all fonts have a correct font-display rule', async () => { @@ -172,7 +173,7 @@ describe('Performance: Font Display audit', () => { const result = await FontDisplayAudit.audit(getArtifacts(), context); assert.strictEqual(result.rawValue, true); - assert.deepEqual(result.details.items, []); + expect(result.details.items).toEqual([]); }); it('should handle real-world font-face declarations', async () => { @@ -212,7 +213,7 @@ describe('Performance: Font Display audit', () => { const result = await FontDisplayAudit.audit(getArtifacts(), context); assert.strictEqual(result.rawValue, false); - assert.deepEqual(result.details.items.map(item => item.url), [ + expect(result.details.items.map(item => item.url)).toEqual([ 'https://edition.i.cdn.cnn.com/.a/fonts/cnn/3.7.2/cnnclock-black.woff2', 'https://registry.api.cnn.io/assets/fave/fonts/2.0.15/cnnsans-bold.woff', // FontAwesome should pass @@ -220,4 +221,26 @@ describe('Performance: Font Display audit', () => { 'https://fonts.gstatic.com/s/lato/v14/S6u9w4BMUTPHh50XSwiPGQ3q5d0.woff2', ]); }); + + it('passes when font-display is last declaration and is missing a semicolon', async () => { + stylesheet.content = ` + @font-face { + src: url(font.woff); + font-display: 'optional' + } + `; + + networkRecords = [ + { + url: 'https://example.com/foo/bar/font.woff', + endTime: 2, startTime: 1, + resourceType: 'Font', + }, + ]; + + const result = await FontDisplayAudit.audit(getArtifacts(), context); + assert.strictEqual(result.rawValue, true); + expect(result.details.items).toEqual([]); + }); + }); From 7c51090fb6ceb758c5f712b2c40f17daad228719 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 8 Feb 2019 17:47:37 -0600 Subject: [PATCH 2/4] lint --- lighthouse-core/audits/font-display.js | 2 +- lighthouse-core/test/audits/font-display-test.js | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lighthouse-core/audits/font-display.js b/lighthouse-core/audits/font-display.js index d4a0e9b7ea2e..9aa1753e9aaf 100644 --- a/lighthouse-core/audits/font-display.js +++ b/lighthouse-core/audits/font-display.js @@ -59,7 +59,7 @@ class FontDisplay extends Audit { for (const declaration of fontFaceDeclarations) { // Find the font-display value by matching a single token, optionally surrounded by whitespace, // followed either by a semicolon or the end of a block. - const rawFontDisplay = declaration.match(/font-display:(\s*\S+\s*)(;|\s*})/); + const rawFontDisplay = declaration.match(/font-display:(\s*\S+\s*)(;|\s*\})/); // If they didn't have a font-display property, it's the default, and it's failing; bail if (!rawFontDisplay) continue; // If they don't have one of the passing font-display values, it's failing; bail diff --git a/lighthouse-core/test/audits/font-display-test.js b/lighthouse-core/test/audits/font-display-test.js index 215bb3b05f81..8ff27c6201ed 100644 --- a/lighthouse-core/test/audits/font-display-test.js +++ b/lighthouse-core/test/audits/font-display-test.js @@ -242,5 +242,4 @@ describe('Performance: Font Display audit', () => { assert.strictEqual(result.rawValue, true); expect(result.details.items).toEqual([]); }); - }); From c7dd90e423d116562c0a4d31ffbee8116f23efae Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 11 Feb 2019 11:04:45 -0600 Subject: [PATCH 3/4] tighten up the whole audit --- lighthouse-core/audits/font-display.js | 6 +-- .../test/audits/font-display-test.js | 50 ++++++++++++------- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/lighthouse-core/audits/font-display.js b/lighthouse-core/audits/font-display.js index 9aa1753e9aaf..5a26c3b16a6f 100644 --- a/lighthouse-core/audits/font-display.js +++ b/lighthouse-core/audits/font-display.js @@ -7,7 +7,7 @@ const Audit = require('./audit'); const URL = require('../lib/url-shim').URL; -const PASSING_FONT_DISPLAY_REGEX = /block|fallback|optional|swap/; +const PASSING_FONT_DISPLAY_REGEX = /^(block|fallback|optional|swap)$/; const CSS_URL_REGEX = /url\((.*?)\)/; const CSS_URL_GLOBAL_REGEX = new RegExp(CSS_URL_REGEX, 'g'); const i18n = require('../lib/i18n/i18n.js'); @@ -59,11 +59,11 @@ class FontDisplay extends Audit { for (const declaration of fontFaceDeclarations) { // Find the font-display value by matching a single token, optionally surrounded by whitespace, // followed either by a semicolon or the end of a block. - const rawFontDisplay = declaration.match(/font-display:(\s*\S+\s*)(;|\s*\})/); + const rawFontDisplay = declaration.match(/font-display\s*:(\s*\w+\s*)(;|\})/); // If they didn't have a font-display property, it's the default, and it's failing; bail if (!rawFontDisplay) continue; // If they don't have one of the passing font-display values, it's failing; bail - const hasPassingFontDisplay = PASSING_FONT_DISPLAY_REGEX.test(rawFontDisplay[0]); + const hasPassingFontDisplay = PASSING_FONT_DISPLAY_REGEX.test(rawFontDisplay[1].trim()); if (!hasPassingFontDisplay) continue; // If it's passing, we'll try to find the URL it's referencing. diff --git a/lighthouse-core/test/audits/font-display-test.js b/lighthouse-core/test/audits/font-display-test.js index 8ff27c6201ed..0d20f4951a3a 100644 --- a/lighthouse-core/test/audits/font-display-test.js +++ b/lighthouse-core/test/audits/font-display-test.js @@ -37,12 +37,13 @@ describe('Performance: Font Display audit', () => { } @font-face { + font-display: 'optional'; // invalid quotes, should still fail for being invalid rule /* try up a directory with ' */ src: url('../font-b.woff'); } @font-face { - font-display: 'optional' // missing a semi-colon, should still fail for being invalid block + font-display: optional // missing a semi-colon, should still fail for being invalid block /* try no path with no quotes ' */ src: url(font.woff); } @@ -80,25 +81,25 @@ describe('Performance: Font Display audit', () => { stylesheet.header.sourceURL = 'https://cdn.example.com/foo/bar/file.css'; stylesheet.content = ` @font-face { - font-display: 'block'; + font-display: block; /* try with " and same directory */ src: url("./font-a.woff"); } @font-face { - font-display: 'block'; + font-display: block; /* try with " and site origin */ src: url("https://example.com/foo/bar/document-font.woff"); } @font-face { - font-display: 'fallback'; + font-display: fallback; /* try up a directory with ' */ src: url('../font-b.woff'); } @font-face { - font-display: 'optional'; + font-display: optional; /* try no path with no quotes ' */ src: url(font.woff); } @@ -135,19 +136,19 @@ describe('Performance: Font Display audit', () => { it('passes when all fonts have a correct font-display rule', async () => { stylesheet.content = ` @font-face { - font-display: 'block'; + font-display: block; /* try with " */ src: url("./font-a.woff"); } @font-face { - font-display: 'fallback'; + font-display: fallback; /* try up a directory with ' */ src: url('../font-b.woff'); } @font-face { - font-display: 'optional'; + font-display: optional; /* try no path with no quotes ' */ src: url(font.woff); } @@ -222,24 +223,35 @@ describe('Performance: Font Display audit', () => { ]); }); - it('passes when font-display is last declaration and is missing a semicolon', async () => { + it('handles varied font-display declarations', async () => { stylesheet.content = ` @font-face { - src: url(font.woff); - font-display: 'optional' + src: url(font-0.woff); + font-display: swap + } + + @font-face {src: url(font-1.woff);font-display : fallback } + + @font-face { + src: url(font-2.woff); + font-display:optional} + + @font-face { + src: url(font-3.woff); + font-display : swap ; } + + @font-face {src: url(font-4.woff);font-display:swap;} `; - networkRecords = [ - { - url: 'https://example.com/foo/bar/font.woff', - endTime: 2, startTime: 1, - resourceType: 'Font', - }, - ]; + networkRecords = Array.from({length: 5}).map((_, idx) => ({ + url: `https://example.com/foo/bar/font-${idx}.woff`, + endTime: 2, startTime: 1, + resourceType: 'Font', + })); const result = await FontDisplayAudit.audit(getArtifacts(), context); - assert.strictEqual(result.rawValue, true); expect(result.details.items).toEqual([]); + assert.strictEqual(result.rawValue, true); }); }); From 9377384c6c9aa3449f0e83430cd5213a42354076 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 11 Feb 2019 15:50:16 -0600 Subject: [PATCH 4/4] move whitespace out of capture group --- lighthouse-core/audits/font-display.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/audits/font-display.js b/lighthouse-core/audits/font-display.js index 5a26c3b16a6f..53953b73c679 100644 --- a/lighthouse-core/audits/font-display.js +++ b/lighthouse-core/audits/font-display.js @@ -59,11 +59,11 @@ class FontDisplay extends Audit { for (const declaration of fontFaceDeclarations) { // Find the font-display value by matching a single token, optionally surrounded by whitespace, // followed either by a semicolon or the end of a block. - const rawFontDisplay = declaration.match(/font-display\s*:(\s*\w+\s*)(;|\})/); + const rawFontDisplay = declaration.match(/font-display\s*:\s*(\w+)\s*(;|\})/); // If they didn't have a font-display property, it's the default, and it's failing; bail if (!rawFontDisplay) continue; // If they don't have one of the passing font-display values, it's failing; bail - const hasPassingFontDisplay = PASSING_FONT_DISPLAY_REGEX.test(rawFontDisplay[1].trim()); + const hasPassingFontDisplay = PASSING_FONT_DISPLAY_REGEX.test(rawFontDisplay[1]); if (!hasPassingFontDisplay) continue; // If it's passing, we'll try to find the URL it's referencing.