From 2ae95f57da85c01e1f31bc03f89fa7f29791cefb Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 13 Oct 2022 12:01:15 -0700 Subject: [PATCH 1/3] core(manifest): remove css color verification --- core/audits/themed-omnibox.js | 20 ++++++-------------- core/lib/manifest-parser.js | 16 ---------------- core/test/audits/themed-omnibox-test.js | 10 ---------- core/test/lib/manifest-parser-test.js | 17 ----------------- package.json | 1 - yarn.lock | 2 +- 6 files changed, 7 insertions(+), 59 deletions(-) diff --git a/core/audits/themed-omnibox.js b/core/audits/themed-omnibox.js index 214b24e489f2..58f86e8184c1 100644 --- a/core/audits/themed-omnibox.js +++ b/core/audits/themed-omnibox.js @@ -4,8 +4,6 @@ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ -import cssParsers from 'cssstyle/lib/parsers.js'; - import MultiCheckAudit from './multi-check-audit.js'; import {ManifestValues} from '../computed/manifest-values.js'; import * as i18n from '../lib/i18n/i18n.js'; @@ -28,8 +26,10 @@ const str_ = i18n.createIcuMessageFn(import.meta.url, UIStrings); * * Requirements: * * manifest is not empty - * * manifest has a valid theme_color - * * HTML has a valid theme-color meta + * * manifest has a theme_color + * * HTML has a theme-color meta + * + * Color validity is explicitly not checked. */ class ThemedOmnibox extends MultiCheckAudit { @@ -47,14 +47,6 @@ class ThemedOmnibox extends MultiCheckAudit { }; } - /** - * @param {string} color - * @return {boolean} - */ - static isValidColor(color) { - return cssParsers.valueType(color) === cssParsers.TYPES.COLOR; - } - /** * @param {LH.Artifacts.MetaElement|undefined} themeColorMeta * @param {Array} failures @@ -63,8 +55,8 @@ class ThemedOmnibox extends MultiCheckAudit { if (!themeColorMeta) { // TODO(#7238): i18n failures.push('No `` tag found'); - } else if (!ThemedOmnibox.isValidColor(themeColorMeta.content || '')) { - failures.push('The theme-color meta tag did not contain a valid CSS color'); + } else if (!themeColorMeta.content) { + failures.push('The theme-color meta tag did not contain a content value'); } } diff --git a/core/lib/manifest-parser.js b/core/lib/manifest-parser.js index a7b0cdb5a216..6fe677c410c5 100644 --- a/core/lib/manifest-parser.js +++ b/core/lib/manifest-parser.js @@ -4,8 +4,6 @@ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ -import cssParsers from 'cssstyle/lib/parsers.js'; - const ALLOWED_DISPLAY_VALUES = [ 'fullscreen', 'standalone', @@ -29,14 +27,6 @@ const ALLOWED_ORIENTATION_VALUES = [ 'landscape-secondary', ]; -/** - * @param {string} color - * @return {boolean} - */ -function isValidColor(color) { - return cssParsers.valueType(color) === cssParsers.TYPES.COLOR; -} - /** * @param {*} raw * @param {boolean=} trim @@ -72,12 +62,6 @@ function parseColor(raw) { return color; } - // Use color parser to check CSS3 Color parsing. - if (!isValidColor(color.raw)) { - color.value = undefined; - color.warning = 'ERROR: color parsing failed.'; - } - return color; } diff --git a/core/test/audits/themed-omnibox-test.js b/core/test/audits/themed-omnibox-test.js index dcd8a8110a7b..8f7595e7c241 100644 --- a/core/test/audits/themed-omnibox-test.js +++ b/core/test/audits/themed-omnibox-test.js @@ -91,16 +91,6 @@ describe('PWA: themed omnibox audit', () => { }); }); - it('fails and warns when theme-color has an invalid CSS color', () => { - const artifacts = generateMockArtifacts(); - artifacts.MetaElements = [{name: 'theme-color', content: '#1234567'}]; - const context = generateMockAuditContext(); - return ThemedOmniboxAudit.audit(artifacts, context).then(result => { - assert.equal(result.score, 0); - assert.ok(result.explanation.includes('valid CSS color')); - }); - }); - it('succeeds when theme-color present in the html', () => { const artifacts = generateMockArtifacts(); artifacts.MetaElements = [{name: 'theme-color', content: '#fafa33'}]; diff --git a/core/test/lib/manifest-parser-test.js b/core/test/lib/manifest-parser-test.js index af07a9b2744e..1bbcb5153552 100644 --- a/core/test/lib/manifest-parser-test.js +++ b/core/test/lib/manifest-parser-test.js @@ -547,23 +547,6 @@ describe('Manifest Parser', function() { assert.strictEqual(parsedManifest.theme_color.warning, undefined); }); - it('warns on invalid colors', () => { - const bgColor = 'notarealcolor'; - const themeColor = '#0123456789'; - const parsedManifest = getParsedManifest(bgColor, themeColor).value; - - assert.deepStrictEqual(parsedManifest.background_color, { - raw: bgColor, - value: undefined, - warning: 'ERROR: color parsing failed.', - }); - assert.deepStrictEqual(parsedManifest.theme_color, { - raw: themeColor, - value: undefined, - warning: 'ERROR: color parsing failed.', - }); - }); - it('warns when colors are not strings', () => { const bgColor = 15; const themeColor = false; diff --git a/package.json b/package.json index 0196d8cd2f5e..5262d46c1a5f 100644 --- a/package.json +++ b/package.json @@ -188,7 +188,6 @@ "chrome-launcher": "^0.15.1", "configstore": "^5.0.1", "csp_evaluator": "1.1.1", - "cssstyle": "1.2.1", "enquirer": "^2.3.6", "http-link-header": "^0.8.0", "intl-messageformat": "^4.4.0", diff --git a/yarn.lock b/yarn.lock index a63d6f3e1ec9..0c4e45f13406 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2777,7 +2777,7 @@ cssom@0.3.x, cssom@^0.3.4: resolved "https://registry.yarnpkg.com/cssom/-/cssom-0.3.8.tgz#9f1276f5b2b463f2114d3f2c75250af8c1a36f4a" integrity sha512-b0tGHbfegbhPJpxpiBPU2sCkigAqtM9O121le6bbOlgyV+NyGyCmVfJ6QW9eRjz8CpNfWEOYBIMIGRYkLwsIYg== -cssstyle@1.2.1, cssstyle@^1.1.1: +cssstyle@^1.1.1: version "1.2.1" resolved "https://registry.yarnpkg.com/cssstyle/-/cssstyle-1.2.1.tgz#3aceb2759eaf514ac1a21628d723d6043a819495" integrity sha512-7DYm8qe+gPx/h77QlCyFmX80+fGaE/6A/Ekl0zaszYOubvySO2saYFdQ78P29D0UsULxFKCetDGNaNRUdSF+2A== From edebdc0c2dab24f30452ecc880f2d33e4dabedb1 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Tue, 18 Oct 2022 13:30:01 -0700 Subject: [PATCH 2/3] test --- core/test/computed/manifest-values-test.js | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/core/test/computed/manifest-values-test.js b/core/test/computed/manifest-values-test.js index 0432c368c5ae..7d9fbcd6f9e3 100644 --- a/core/test/computed/manifest-values-test.js +++ b/core/test/computed/manifest-values-test.js @@ -91,19 +91,6 @@ describe('ManifestValues computed artifact', () => { assert.equal(colorResults.every(i => i.passing === false), true); }); - it('fails when a minimal manifest contains an invalid background_color', async () => { - const WebAppManifest = noUrlManifestParser(JSON.stringify({ - background_color: 'no', - theme_color: 'no', - })); - const InstallabilityErrors = {errors: []}; - const artifacts = {WebAppManifest, InstallabilityErrors}; - - const results = await ManifestValues.request(artifacts, getMockContext()); - const colorResults = results.allChecks.filter(i => i.id.includes('Color')); - assert.equal(colorResults.every(i => i.passing === false), true); - }); - it('succeeds when a minimal manifest contains a valid background_color', async () => { const WebAppManifest = noUrlManifestParser(JSON.stringify({ background_color: '#FAFAFA', From 548951e755ccaf8ec8e9064136b3c4fea6814a40 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 20 Oct 2022 17:00:45 -0700 Subject: [PATCH 3/3] test --- core/test/audits/splash-screen-test.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/core/test/audits/splash-screen-test.js b/core/test/audits/splash-screen-test.js index f2c427f0632c..c0ab5dd922d0 100644 --- a/core/test/audits/splash-screen-test.js +++ b/core/test/audits/splash-screen-test.js @@ -98,18 +98,6 @@ describe('PWA: splash screen audit', () => { }); }); - it('fails when a manifest contains invalid background color', () => { - const artifacts = generateMockArtifacts(JSON.stringify({ - background_color: 'no', - })); - const context = generateMockAuditContext(); - - return SplashScreenAudit.audit(artifacts, context).then(result => { - assert.strictEqual(result.score, 0); - assert.ok(result.explanation.includes('background_color'), result.explanation); - }); - }); - it('fails when a manifest contains no theme color', () => { const artifacts = generateMockArtifacts(); artifacts.WebAppManifest.value.theme_color.value = undefined;