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.

Sign up for GitHub

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

Jump to bottom

fix: cleanupIds changes style IDs incorrectly #1956

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: cleanupIds changes style IDs incorrectly #1956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
886e4fd
Handle multiple IDs in the style attribute.
johnkenny54 Feb 9, 2024
b9fb8cf
Use findReferences to find IDs in style attribute.
johnkenny54 Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
117 changes: 85 additions & 32 deletions plugins/cleanupIds.js
View file
Open in desktop
Expand Up @@ -147,6 +147,37 @@ export const fn = (_root, params) => {
const referencesById = new Map();
let deoptimized = false;

function generateIdMap() {
/**
* @param {string} id
* @returns {boolean}
*/
const isIdPreserved = (id) =>
preserveIds.has(id) || hasStringPrefix(id, preserveIdPrefixes);
const idMap = new Map();
let currentId = null;
for (const [id] of referencesById) {
const node = nodeById.get(id);
if (node != null) {
// replace referenced IDs with the minified ones
if (minify && isIdPreserved(id) === false) {
/** @type {?string} */
let currentIdString = null;
do {
currentId = generateId(currentId);
currentIdString = getIdString(currentId);
} while (
isIdPreserved(currentIdString) ||
(referencesById.has(currentIdString) &&
nodeById.get(currentIdString) == null)
);
idMap.set(id, currentIdString);
}
}
}
return idMap;
}

return {
element: {
enter: (node) => {
Expand Down Expand Up @@ -210,44 +241,66 @@ export const fn = (_root, params) => {
*/
const isIdPreserved = (id) =>
preserveIds.has(id) || hasStringPrefix(id, preserveIdPrefixes);
/** @type {?number[]} */
let currentId = null;

const idMap = generateIdMap();
const elementssWithStylesUpdated = new Set();

for (const [id, refs] of referencesById) {
// Ignore the node if no new ID was generated for it.
if (!idMap.has(id)) {
continue;
}
const node = nodeById.get(id);
if (node != null) {
// replace referenced IDs with the minified ones
if (minify && isIdPreserved(id) === false) {
/** @type {?string} */
let currentIdString = null;
do {
currentId = generateId(currentId);
currentIdString = getIdString(currentId);
} while (
isIdPreserved(currentIdString) ||
(referencesById.has(currentIdString) &&
nodeById.get(currentIdString) == null)
);
node.attributes.id = currentIdString;
for (const { element, name } of refs) {
const value = element.attributes[name];
if (value.includes('#')) {
// replace id in href and url()
element.attributes[name] = value.replace(
`#${encodeURI(id)}`,
`#${currentIdString}`,
);
} else {
// replace id in begin attribute
element.attributes[name] = value.replace(
`${id}.`,
`${currentIdString}.`,
);
if (!node) {
continue;
}

// replace referenced IDs with the minified ones
const currentIdString = idMap.get(id);
node.attributes.id = currentIdString;

for (const { element, name } of refs) {
const value = element.attributes[name];
if (value.includes('#')) {
if (name === 'style') {
if (!elementssWithStylesUpdated.has(element)) {
// The style may have more than one ID; all must be replaced at once to eliminate the possibility of overlap.
const styles = value.split(';');
for (let index = 0; index < styles.length; index++) {
const style = styles[index];
const refs = findReferences('style', style);
if (refs.length) {
const id = refs[0];
const newId = idMap.get(id);
if (newId) {
styles[index] = style.replace(
`#${encodeURI(id)}`,
`#${newId}`,
);
}
}
}
element.attributes[name] = styles.join(';');
elementssWithStylesUpdated.add(element);
}
} else {
// replace id in href and url()
element.attributes[name] = value.replace(
`#${encodeURI(id)}`,
`#${currentIdString}`,
);
}
} else {
// replace id in begin attribute
element.attributes[name] = value.replace(
`${id}.`,
`${currentIdString}.`,
);
}
// keep referenced node
nodeById.delete(id);
}

// keep referenced node
nodeById.delete(id);
}
// remove non-referenced IDs attributes from elements
if (remove) {
Expand Down
36 changes: 36 additions & 0 deletions test/plugins/cleanupIds.26.svg.txt
View file
Open in desktop
@@ -0,0 +1,36 @@
When two IDs are referenced in the same attribute, make sure they both get changed even if the new
value of the first ID is the same as the old value of the second ID.

===

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100">
<linearGradient id="a">
<stop stop-color="blue" offset="0"/>
</linearGradient>
<linearGradient id="ccc">
<stop stop-color="#f00" offset="0"/>
</linearGradient>
<linearGradient id="b">
<stop stop-color="green" offset="0"/>
</linearGradient>
<rect y="40" width="10" height="20" style="fill:url(#a)"/>
<rect y="10" width="10" height="20" style="fill:url(#ccc);stroke:url(#b);stroke-width:3"/>
<rect y="70" width="10" height="20" style="fill:url(#ccc);stroke:#ccc;stroke-width:3"/>
</svg>

@@@

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100">
<linearGradient id="a">
<stop stop-color="blue" offset="0"/>
</linearGradient>
<linearGradient id="b">
<stop stop-color="#f00" offset="0"/>
</linearGradient>
<linearGradient id="c">
<stop stop-color="green" offset="0"/>
</linearGradient>
<rect y="40" width="10" height="20" style="fill:url(#a)"/>
<rect y="10" width="10" height="20" style="fill:url(#b);stroke:url(#c);stroke-width:3"/>
<rect y="70" width="10" height="20" style="fill:url(#b);stroke:#ccc;stroke-width:3"/>
</svg>