Skip to content

Commit

Permalink
fix(inlineStyles): empty css block created empty attribute (#1823)
Browse files Browse the repository at this point in the history
When running this plugin on an SVG with an empty block in the CSS, it
would apply it to the matched elements by adding an empty `style`
attribute. See the test for an example.

This resolves that by just dropping the declaration if it's empty.
  • Loading branch information
SethFalco authored Nov 7, 2023
1 parent 07c0919 commit 5aad38b
Show file tree
Hide file tree
Showing 9 changed files with 80 additions and 95 deletions.
35 changes: 13 additions & 22 deletions lib/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

const csstree = require('css-tree');
const {
// @ts-ignore not defined in @types/csso
// @ts-ignore internal api
syntax: { specificity },
} = require('csso');
const { visit, matches } = require('./xast.js');
Expand Down Expand Up @@ -47,9 +47,7 @@ const parseRule = (ruleNode, dynamic) => {
}
});

/**
* @type {StylesheetRule[]}
*/
/** @type {StylesheetRule[]} */
const rules = [];
csstree.walk(ruleNode.prelude, (node) => {
if (node.type === 'Selector') {
Expand Down Expand Up @@ -78,9 +76,7 @@ const parseRule = (ruleNode, dynamic) => {
* @type {(css: string, dynamic: boolean) => Array<StylesheetRule>}
*/
const parseStylesheet = (css, dynamic) => {
/**
* @type {Array<StylesheetRule>}
*/
/** @type {Array<StylesheetRule>} */
const rules = [];
const ast = csstree.parse(css, {
parseValue: false,
Expand Down Expand Up @@ -111,9 +107,7 @@ const parseStylesheet = (css, dynamic) => {
* @type {(css: string) => Array<StylesheetDeclaration>}
*/
const parseStyleDeclarations = (css) => {
/**
* @type {Array<StylesheetDeclaration>}
*/
/** @type {Array<StylesheetDeclaration>} */
const declarations = [];
const ast = csstree.parse(css, {
context: 'declarationList',
Expand All @@ -135,9 +129,7 @@ const parseStyleDeclarations = (css) => {
* @type {(stylesheet: Stylesheet, node: XastElement) => ComputedStyles}
*/
const computeOwnStyle = (stylesheet, node) => {
/**
* @type {ComputedStyles}
*/
/** @type {ComputedStyles} */
const computedStyle = {};
const importantStyles = new Map();

Expand Down Expand Up @@ -197,10 +189,12 @@ const computeOwnStyle = (stylesheet, node) => {
};

/**
* Compares two selector specificities.
* extracted from https://github.com/keeganstreet/specificity/blob/main/specificity.js#L211
* Compares selector specificities.
* Derived from https://github.com/keeganstreet/specificity/blob/8757133ddd2ed0163f120900047ff0f92760b536/specificity.js#L207
*
* @type {(a: Specificity, b: Specificity) => number}
* @param {Specificity} a
* @param {Specificity} b
* @returns {number}
*/
const compareSpecificity = (a, b) => {
for (let i = 0; i < 4; i += 1) {
Expand All @@ -213,18 +207,15 @@ const compareSpecificity = (a, b) => {

return 0;
};
exports.compareSpecificity = compareSpecificity;

/**
* @type {(root: XastRoot) => Stylesheet}
*/
const collectStylesheet = (root) => {
/**
* @type {Array<StylesheetRule>}
*/
/** @type {Array<StylesheetRule>} */
const rules = [];
/**
* @type {Map<XastElement, XastParent>}
*/
/** @type {Map<XastElement, XastParent>} */
const parents = new Map();
visit(root, {
element: {
Expand Down
2 changes: 1 addition & 1 deletion lib/svgo/plugins.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const { visit } = require('../xast.js');
*/
const invokePlugins = (ast, info, plugins, overrides, globalOverrides) => {
for (const plugin of plugins) {
const override = overrides == null ? null : overrides[plugin.name];
const override = overrides?.[plugin.name];
if (override === false) {
continue;
}
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,14 @@
"commander": "^7.2.0",
"css-select": "^5.1.0",
"css-tree": "^2.2.1",
"csso": "^5.0.5",
"csso": "5.0.5",
"picocolors": "^1.0.0"
},
"devDependencies": {
"@rollup/plugin-commonjs": "^22.0.2",
"@rollup/plugin-node-resolve": "^14.1.0",
"@types/css-tree": "^2.0.0",
"@types/csso": "^5.0.1",
"@types/csso": "~5.0.3",
"@types/jest": "^29.5.5",
"del": "^6.0.0",
"eslint": "^8.24.0",
Expand Down
56 changes: 11 additions & 45 deletions plugins/inlineStyles.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,57 +8,24 @@

const csstree = require('css-tree');
const {
// @ts-ignore not defined in @types/csso
// @ts-ignore internal api
syntax: { specificity },
} = require('csso');
const {
visitSkip,
querySelectorAll,
detachNodeFromParent,
} = require('../lib/xast.js');
const { compareSpecificity } = require('../lib/style');

exports.name = 'inlineStyles';
exports.description = 'inline styles (additional options)';

/**
* Compares two selector specificities.
* extracted from https://github.com/keeganstreet/specificity/blob/main/specificity.js#L211
*
* @type {(a: Specificity, b: Specificity) => number}
*/
const compareSpecificity = (a, b) => {
for (var i = 0; i < 4; i += 1) {
if (a[i] < b[i]) {
return -1;
} else if (a[i] > b[i]) {
return 1;
}
}
return 0;
};

/**
* Moves + merges styles from style elements to element styles
*
* Options
* onlyMatchedOnce (default: true)
* inline only selectors that match once
*
* removeMatchedSelectors (default: true)
* clean up matched selectors,
* leave selectors that hadn't matched
*
* useMqs (default: ['', 'screen'])
* what media queries to be used
* empty string element for styles outside media queries
*
* usePseudos (default: [''])
* what pseudo-classes/-elements to be used
* empty string element for all non-pseudo-classes and/or -elements
*
* @author strarsis <[email protected]>
* Merges styles from style nodes into inline styles.
*
* @type {import('./plugins-types').Plugin<'inlineStyles'>}
* @author strarsis <[email protected]>
*/
exports.fn = (root, params) => {
const {
Expand Down Expand Up @@ -205,9 +172,7 @@ exports.fn = (root, params) => {
for (const selector of sortedSelectors) {
// match selectors
const selectorText = csstree.generate(selector.item.data);
/**
* @type {Array<XastElement>}
*/
/** @type {Array<XastElement>} */
const matchedElements = [];
try {
for (const node of querySelectorAll(root, selectorText)) {
Expand All @@ -232,9 +197,7 @@ exports.fn = (root, params) => {
// apply <style/> to matched elements
for (const selectedEl of matchedElements) {
const styleDeclarationList = csstree.parse(
selectedEl.attributes.style == null
? ''
: selectedEl.attributes.style,
selectedEl.attributes.style ?? '',
{
context: 'declarationList',
parseValue: false,
Expand Down Expand Up @@ -280,8 +243,11 @@ exports.fn = (root, params) => {
}
},
});
selectedEl.attributes.style =
csstree.generate(styleDeclarationList);

const newStyles = csstree.generate(styleDeclarationList);
if (newStyles.length !== 0) {
selectedEl.attributes.style = newStyles;
}
}

if (
Expand Down
28 changes: 23 additions & 5 deletions plugins/plugins-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,28 @@ type DefaultPlugins = {
};
mergeStyles: void;
inlineStyles: {
/**
* Inlines selectors that match once only.
*
* @default true
*/
onlyMatchedOnce?: boolean;
/**
* Clean up matched selectors. Unused selects are left as-is.
*
* @default true
*/
removeMatchedSelectors?: boolean;
useMqs?: Array<string>;
usePseudos?: Array<string>;
/**
* Media queries to use. An empty string indicates all selectors outside of
* media queries.
*/
useMqs?: string[];
/**
* Pseudo-classes and elements to use. An empty string indicates all
* all non-pseudo-classes and elements.
*/
usePseudos?: string[];
};
mergePaths: {
force?: boolean;
Expand All @@ -89,21 +107,21 @@ type DefaultPlugins = {
* Disable or enable a structure optimisations.
* @default true
*/
restructure?: boolean | undefined;
restructure?: boolean;
/**
* Enables merging of @media rules with the same media query by splitted by other rules.
* The optimisation is unsafe in general, but should work fine in most cases. Use it on your own risk.
* @default false
*/
forceMediaMerge?: boolean | undefined;
forceMediaMerge?: boolean;
/**
* Specify what comments to leave:
* - 'exclamation' or true – leave all exclamation comments
* - 'first-exclamation' – remove every comment except first one
* - false – remove all comments
* @default true
*/
comments?: string | boolean | undefined;
comments?: string | boolean;
/**
* Advanced optimizations
*/
Expand Down
8 changes: 2 additions & 6 deletions plugins/removeUnknownsAndDefaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,16 +182,12 @@ exports.fn = (root, params) => {
attributesDefaults.get(name) === value
) {
// keep defaults if parent has own or inherited style
if (
computedParentStyle == null ||
computedParentStyle[name] == null
) {
if (computedParentStyle?.[name] == null) {
delete node.attributes[name];
}
}
if (uselessOverrides && node.attributes.id == null) {
const style =
computedParentStyle == null ? null : computedParentStyle[name];
const style = computedParentStyle?.[name];
if (
presentationNonInheritableGroupAttrs.includes(name) === false &&
style != null &&
Expand Down
20 changes: 20 additions & 0 deletions test/plugins/inlineStyles.23.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 1 addition & 7 deletions test/regression.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,11 @@ const runTests = async ({ list }) => {
name.startsWith('w3c-svg-11-test-suite/svg/animate-') ||
// messed gradients
name === 'w3c-svg-11-test-suite/svg/pservers-grad-18-b.svg' ||
// animated filter
name === 'w3c-svg-11-test-suite/svg/filters-light-04-f.svg' ||
// animated filter
name === 'w3c-svg-11-test-suite/svg/filters-composite-05-f.svg' ||
// removing wrapping <g> breaks :first-child pseudo-class
name === 'w3c-svg-11-test-suite/svg/styling-pres-04-f.svg' ||
// rect is converted to path which matches wrong styles
name === 'w3c-svg-11-test-suite/svg/styling-css-08-f.svg' ||
// external image
name === 'w3c-svg-11-test-suite/svg/struct-image-02-b.svg' ||
// complex selectors are messed becase of converting shapes to paths
// complex selectors are messed because of converting shapes to paths
name === 'w3c-svg-11-test-suite/svg/struct-use-10-f.svg' ||
name === 'w3c-svg-11-test-suite/svg/struct-use-11-f.svg' ||
name === 'w3c-svg-11-test-suite/svg/styling-css-01-b.svg' ||
Expand Down
14 changes: 7 additions & 7 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -958,12 +958,12 @@ __metadata:
languageName: node
linkType: hard

"@types/csso@npm:^5.0.1":
version: 5.0.1
resolution: "@types/csso@npm:5.0.1"
"@types/csso@npm:~5.0.3":
version: 5.0.3
resolution: "@types/csso@npm:5.0.3"
dependencies:
"@types/css-tree": "*"
checksum: 62c7e534dfde79c1bf3b3761baec06ac2aa2b568da9be4a548b95e709b65b3944b8ad4cd4d8926de2b4bb301b4f102fb0f8c1354cb203a92ebccf59e58c957a6
checksum: 3d299ea755732f9b913cfb3d94849e173cd1019559058af0a372aa1ca8f48a3c63aa74932fdfa2f2f25ee78255a115feaaec01ae4fe9578e76b7c4acd8ae3f2a
languageName: node
linkType: hard

Expand Down Expand Up @@ -1695,7 +1695,7 @@ __metadata:
languageName: node
linkType: hard

"csso@npm:^5.0.5":
"csso@npm:5.0.5":
version: 5.0.5
resolution: "csso@npm:5.0.5"
dependencies:
Expand Down Expand Up @@ -4563,12 +4563,12 @@ __metadata:
"@rollup/plugin-node-resolve": ^14.1.0
"@trysound/sax": 0.2.0
"@types/css-tree": ^2.0.0
"@types/csso": ^5.0.1
"@types/csso": ~5.0.3
"@types/jest": ^29.5.5
commander: ^7.2.0
css-select: ^5.1.0
css-tree: ^2.2.1
csso: ^5.0.5
csso: 5.0.5
del: ^6.0.0
eslint: ^8.24.0
jest: ^29.5.5
Expand Down

0 comments on commit 5aad38b

Please sign in to comment.