-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
report: ensure SVG elements in <defs> have unique ids #9151
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,16 @@ | |
|
||
/* globals self, Util, CategoryRenderer */ | ||
|
||
/** | ||
* An always-increasing counter for making unique SVG ID suffixes. | ||
*/ | ||
const getUniqueSuffix = (() => { | ||
let svgSuffix = 0; | ||
return function() { | ||
return svgSuffix++; | ||
}; | ||
})(); | ||
|
||
class PwaCategoryRenderer extends CategoryRenderer { | ||
/** | ||
* @param {LH.ReportResult.Category} category | ||
|
@@ -62,6 +72,11 @@ class PwaCategoryRenderer extends CategoryRenderer { | |
tmpl)); | ||
wrapper.href = `#${category.id}`; | ||
|
||
// Correct IDs in case multiple instances end up in the page. | ||
const svgRoot = tmpl.querySelector('svg'); | ||
if (!svgRoot) throw new Error('no SVG element found in PWA score gauge template'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't this exactly what our `this.dom.find is for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes, but the typescript types assume the return type is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment to this effect? |
||
PwaCategoryRenderer._makeSvgReferencesUnique(svgRoot); | ||
|
||
const allGroups = this._getGroupIds(category.auditRefs); | ||
const passingGroupIds = this._getPassingGroupIds(category.auditRefs); | ||
|
||
|
@@ -147,6 +162,38 @@ class PwaCategoryRenderer extends CategoryRenderer { | |
|
||
return auditsElem; | ||
} | ||
|
||
/** | ||
* Alters SVG id references so multiple instances of an SVG element can coexist | ||
* in a single page. If `svgRoot` has a `<defs>` block, gives all elements defined | ||
* in it unique ids, then updates id references (`<use xlink:href="...">`, | ||
* `fill="url(#...)"`) to the altered ids in all descendents of `svgRoot`. | ||
* @param {SVGElement} svgRoot | ||
*/ | ||
static _makeSvgReferencesUnique(svgRoot) { | ||
const defsEl = svgRoot.querySelector('defs'); | ||
if (!defsEl) return; | ||
|
||
const idSuffix = getUniqueSuffix(); | ||
const elementsToUpdate = defsEl.querySelectorAll('[id]'); | ||
for (const el of elementsToUpdate) { | ||
const oldId = el.id; | ||
const newId = `${oldId}-${idSuffix}`; | ||
el.id = newId; | ||
|
||
// Update all <use>s. | ||
const useEls = svgRoot.querySelectorAll(`use[href="#${oldId}"]`); | ||
for (const useEl of useEls) { | ||
useEl.setAttribute('href', `#${newId}`); | ||
} | ||
|
||
// Update all fill="url(#...)"s. | ||
const fillEls = svgRoot.querySelectorAll(`[fill="url(#${oldId})"]`); | ||
for (const fillEl of fillEls) { | ||
fillEl.setAttribute('fill', `url(#${newId})`); | ||
} | ||
} | ||
} | ||
} | ||
|
||
if (typeof module !== 'undefined' && module.exports) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -669,24 +669,24 @@ | |
<!-- Just fast and reliable. --> | ||
<g class="lh-gauge--pwa__component lh-gauge--pwa__fast-reliable-badge" transform="translate(20, 29)"> | ||
<path fill="url(#lh-gauge--pwa__fast-reliable__shadow-gradient)" d="M33.63 19.49A30 30 0 0 1 16.2 30.36L3 17.14 17.14 3l16.49 16.49z"/> | ||
<use xlink:href="#lh-gauge--pwa__fast-reliable-badge" /> | ||
<use href="#lh-gauge--pwa__fast-reliable-badge" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. apparently |
||
</g> | ||
|
||
<!-- Just installable. --> | ||
<g class="lh-gauge--pwa__component lh-gauge--pwa__installable-badge" transform="translate(20, 29)"> | ||
<path fill="url(#lh-gauge--pwa__installable__shadow-gradient)" d="M33.629 19.487c-4.272 5.453-10.391 9.39-17.415 10.869L3 17.142 17.142 3 33.63 19.487z"/> | ||
<use xlink:href="#lh-gauge--pwa__installable-badge" /> | ||
<use href="#lh-gauge--pwa__installable-badge" /> | ||
</g> | ||
|
||
<!-- Fast and reliable and installable. --> | ||
<g class="lh-gauge--pwa__component lh-gauge--pwa__fast-reliable-installable-badges"> | ||
<g transform="translate(8, 29)"> <!-- fast and reliable --> | ||
<path fill="url(#lh-gauge--pwa__fast-reliable__shadow-gradient)" d="M16.321 30.463L3 17.143 17.142 3l22.365 22.365A29.864 29.864 0 0 1 22 31c-1.942 0-3.84-.184-5.679-.537z"/> | ||
<use xlink:href="#lh-gauge--pwa__fast-reliable-badge" /> | ||
<use href="#lh-gauge--pwa__fast-reliable-badge" /> | ||
</g> | ||
<g transform="translate(32, 29)"> <!-- installable --> | ||
<path fill="url(#lh-gauge--pwa__installable__shadow-gradient)" d="M25.982 11.84a30.107 30.107 0 0 1-13.08 15.203L3 17.143 17.142 3l8.84 8.84z"/> | ||
<use xlink:href="#lh-gauge--pwa__installable-badge" /> | ||
<use href="#lh-gauge--pwa__installable-badge" /> | ||
</g> | ||
</g> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -242,7 +242,7 @@ describe('PwaCategoryRenderer', () => { | |
describe('#renderScoreGauge', () => { | ||
it('renders an error score gauge in case of category error', () => { | ||
category.score = null; | ||
const badgeGauge = pwaRenderer.renderScoreGauge(category); | ||
const badgeGauge = pwaRenderer.renderScoreGauge(category, sampleResults.categoryGroups); | ||
|
||
// Not a PWA gauge. | ||
assert.strictEqual(badgeGauge.querySelector('.lh-gauge--pwa__wrapper'), null); | ||
|
@@ -251,5 +251,37 @@ describe('PwaCategoryRenderer', () => { | |
assert.strictEqual(percentageElem.textContent, '?'); | ||
assert.strictEqual(percentageElem.title, Util.UIStrings.errorLabel); | ||
}); | ||
|
||
it('renders score gauges with unique ids for items in <defs>', () => { | ||
const gauge1 = pwaRenderer.renderScoreGauge(category, sampleResults.categoryGroups); | ||
const gauge1Ids = [...gauge1.querySelectorAll('defs [id]')].map(el => el.id); | ||
assert.ok(gauge1Ids.length > 3); | ||
|
||
const gauge2 = pwaRenderer.renderScoreGauge(category, sampleResults.categoryGroups); | ||
const gauge2Ids = [...gauge2.querySelectorAll('defs [id]')].map(el => el.id); | ||
assert.ok(gauge2Ids.length === gauge1Ids.length); | ||
|
||
/** Returns whether id is used by at least one <use> or fill under `rootEl`. */ | ||
function idInUseElOrFillAttr(rootEl, id) { | ||
const isUse = rootEl.querySelector(`use[href="#${id}"]`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. feels like a bit of a bummer that we're testing this in a way that's basically identical to the implementation, WDYT about getting the outerHTML and checking to make sure there's no old ID without a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yeah, I actually started doing this before getting hamstrung by jsdom weirdness with After that, I convinced myself that this is a good test because even though it's using the same selectors as the implementation, it's testing exactly what we want the observable behavior to be: given two pwa score gauges, 1) do they share any There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. alright you've convinced me too :) |
||
const isFill = rootEl.querySelector(`[fill="url(#${id})"]`); | ||
|
||
return !!(isUse || isFill); | ||
} | ||
|
||
// Check that each gauge1 ID is actually used in gauge1 and isn't used in gauge2. | ||
for (const gauge1Id of gauge1Ids) { | ||
assert.equal(idInUseElOrFillAttr(gauge1, gauge1Id), true); | ||
assert.ok(!gauge2Ids.includes(gauge1Id)); | ||
assert.equal(idInUseElOrFillAttr(gauge2, gauge1Id), false); | ||
} | ||
|
||
// And that the reverse is true for gauge2 IDs. | ||
for (const gauge2Id of gauge2Ids) { | ||
assert.equal(idInUseElOrFillAttr(gauge1, gauge2Id), false); | ||
assert.equal(idInUseElOrFillAttr(gauge2, gauge2Id), true); | ||
assert.ok(!gauge1Ids.includes(gauge2Id)); | ||
} | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is not my favorite thing but felt a little bit better than
Math.floor(Math.random() * 9999)
as a suffix generator. I can switch and avoid this if anyone feels strongly, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah I like this much better than math.random 👍
🚲 🏠 naming though
getNextSVGSuffix()
getUniqueSuffix()
getAndIncrementID()
any of those sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getUniqueSuffix
👍