Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(manifest): remove css color verification #14447

Merged
merged 3 commits into from
Oct 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 6 additions & 14 deletions core/audits/themed-omnibox.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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 {
Expand All @@ -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<string>} failures
Expand All @@ -63,8 +55,8 @@ class ThemedOmnibox extends MultiCheckAudit {
if (!themeColorMeta) {
// TODO(#7238): i18n
failures.push('No `<meta name="theme-color">` 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');
}
}

Expand Down
16 changes: 0 additions & 16 deletions core/lib/manifest-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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
Expand Down Expand Up @@ -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;
}

Expand Down
12 changes: 0 additions & 12 deletions core/test/audits/splash-screen-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
10 changes: 0 additions & 10 deletions core/test/audits/themed-omnibox-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'}];
Expand Down
13 changes: 0 additions & 13 deletions core/test/computed/manifest-values-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
17 changes: 0 additions & 17 deletions core/test/lib/manifest-parser-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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==
Expand Down