[go: nahoru, domu]
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

report: ensure SVG elements in <defs> have unique ids #9151

Merged
merged 2 commits into from
Jun 11, 2019
Merged

report: ensure SVG elements in <defs> have unique ids #9151

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
d022d8e
report: ensure unique SVG element ids
brendankenny Jun 7, 2019
691ba17
names naming names
brendankenny Jun 10, 2019
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
47 changes: 47 additions & 0 deletions lighthouse-core/report/html/renderer/pwa-category-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
};
Copy link
Member Author

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.

Copy link
Collaborator

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getUniqueSuffix 👍

})();

class PwaCategoryRenderer extends CategoryRenderer {
/**
* @param {LH.ReportResult.Category} category
Expand Down Expand Up @@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this exactly what our `this.dom.find is for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this exactly what our `this.dom.find is for?

yes, but the typescript types assume the return type is HTMLElement, and I didn't want us to have to add a check that the thing is an HTMLElement not SVG everywhere else, so it seemed ok for a one off (we could probably do contextual typing for dom.find if it's a tag name like we do for dom.createElement, et al)

Copy link
Collaborator

Choose a reason for hiding this comment

The 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);

Expand Down Expand Up @@ -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) {
Expand Down
8 changes: 4 additions & 4 deletions lighthouse-core/report/html/templates.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently xlink:href is old and busted

</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>

Expand Down
34 changes: 33 additions & 1 deletion lighthouse-core/test/report/html/renderer/pwa-category-renderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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}"]`);
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 -{suffix} with it?

Copy link
Member Author
@brendankenny brendankenny Jun 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The 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 -{suffix} with it?

yeah, I actually started doing this before getting hamstrung by jsdom weirdness with outerHTML, which makes it difficult to do.

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 <defs> ids and 2) are the ids internally consistent in each (which ends up a bit of a stronger guarantee than the outerHTML check).

Copy link
Collaborator

Choose a reason for hiding this comment

The 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));
}
});
});
});