report: ensure SVG elements in <defs> have unique ids #9151
Conversation
| let svgSuffix = 0; | ||
| return function() { | ||
| return svgSuffix++; | ||
| }; |
There was a problem hiding this comment.
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.
nah I like this much better than math.random 👍
🚲 🏠 naming though
getNextSVGSuffix()getUniqueSuffix()getAndIncrementID()
any of those sound good?
| @@ -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.
apparently xlink:href is old and busted
| let svgSuffix = 0; | ||
| return function() { | ||
| return svgSuffix++; | ||
| }; |
There was a problem hiding this comment.
nah I like this much better than math.random 👍
🚲 🏠 naming though
getNextSVGSuffix()getUniqueSuffix()getAndIncrementID()
any of those sound good?
| @@ -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.
isn't this exactly what our `this.dom.find is for?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
comment to this effect?
| if (!defsEl) return; | ||
|
|
||
| const idSuffix = svgSuffixCounter(); | ||
| const elsToUpdate = defsEl.querySelectorAll('[id]'); |
There was a problem hiding this comment.
for some reason els feels so much rarer to me than an El suffix.
we look pretty solid on space here, think we can swing elements?
There was a problem hiding this comment.
we look pretty solid on space here, think we can swing
elements?
👍
|
|
||
| /** 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.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
alright you've convinced me too :)
fixes #9139
SVG elements in a
<defs>block need to have globally unique IDs when trying to reference them in a<use>or as a fill or you're going to have a bad time as styling gets more complicated (see issue for details). Rather than getting rid of the references (growing the file size) or deduping the<defs>in the page (requiring significant rework of our "ask for a gauge, get a gauge element" interface forrenderScoreGaugeand the call chain that proceeds it), this PR adds a unique suffix to SVG IDs to make sure they don't collide (apparently a common technique in some SVG libraries?).I was going to add this onto
Util, but we don't currently touch the DOM anywhere in there (and don't have jsdom set up for the tests for it), so I kept it limited topwa-category-renderer.js, the single place we use<defs>right now. We can always make it generally available if we eventually need it elsewhere.