Merge pull request #1561 from tanem/review

Tidy and improve comments

Author: tanem

.github/copilot-instructions.md

@@ -97,6 +97,7 @@ This project follows strict versioning conventions for dependencies:
 - **No arrow function class methods**: this is a functional codebase with no classes.
 - **Strict TypeScript and ESLint**: `tsconfig.base.json` and `eslint.config.mjs` enforce strict type safety. Never use `any` types; use `unknown` when type is truly dynamic. Use non-null assertions (`!`) only with runtime guarantees (e.g., array access within bounds-checked loops).
 - **Formatting**: Prettier handles all JS/TS formatting. Run `npm run format` or check with `npm run check:format`.
+- **Comment style**: Use `//` comments, not `/* */` (except for istanbul/eslint directives). Comments are wrapped to 80 columns.
 
 ## IRI Renumeration
 
@@ -121,6 +122,7 @@ When `renumerateIRIElements` is `true` (the default), the injector rewrites `id`
 - Keep `.github/copilot-instructions.md`, `README.md`, and `MIGRATION.md` up to date when making changes that affect the public API, build pipeline, testing patterns, or code conventions.
 - Use NZ English in documentation (e.g. "serialise", "normalise", "colour", "behaviour").
 - **Copilot instructions should only contain information that cannot be readily inferred from the source code.** Do not duplicate implementation details (e.g. function names, processing order, data structures) that an agent can discover by reading the relevant files. Focus on conventions, design decisions, known limitations, and non-obvious constraints.
+- **README structure follows [standard-readme](https://github.com/RichardLitt/standard-readme).** Keep the main `README.md` concise and scannable. Detailed feature documentation (usage examples, caveats, limitations) belongs in a `README.md` within the relevant `examples/` subdirectory, linked from the main README.
 
 ## Writing Style
 

src/clone-svg.ts

@@ -1,3 +1,6 @@
+// Deep clone to avoid mutating cached SVG originals. Each injection receives
+// its own copy that can be safely modified (attribute transfer, IRI
+// renumeration, script removal, etc.).
 const cloneSvg = (sourceSvg: SVGSVGElement) =>
   sourceSvg.cloneNode(true) as SVGSVGElement
 

src/inject-element.ts

@@ -6,6 +6,9 @@ import uniqueId from './unique-id'
 
 type ElementType = Element | HTMLElement | null
 
+// Tracks elements currently being injected. Prevents duplicate injection if
+// SVGInjector is called with the same element twice before the first injection
+// completes.
 const injectedElements: ElementType[] = []
 const ranScripts: Record<string, boolean> = {}
 const svgNamespace = 'http://www.w3.org/2000/svg'
@@ -27,22 +30,17 @@ const injectElement = (
     return
   }
 
-  // Make sure we aren't already in the process of injecting this element to
-  // avoid a race condition if multiple injections for the same element are run.
-  // :NOTE: Using indexOf() only _after_ we check for SVG support and bail, so
-  // no need for IE8 indexOf() polyfill.
   if (injectedElements.indexOf(el) !== -1) {
-    // TODO: Extract.
     injectedElements.splice(injectedElements.indexOf(el), 1)
+    // Release the DOM reference to allow GC. The cast is needed because the
+    // parameter is typed as NonNullable.
     ;(el as ElementType) = null
     return
   }
 
-  // Remember the request to inject this element, in case other injection calls
-  // are also trying to replace this element before we finish.
   injectedElements.push(el)
-
-  // Try to avoid loading the orginal image src if possible.
+  // Clear src to prevent the browser from fetching the original image URL while
+  // the SVG load is in progress.
   el.setAttribute('src', '')
 
   // Strip fragment identifier for sprite support. The base URL is used for
@@ -55,7 +53,6 @@ const injectElement = (
 
   loadSvg(baseUrl, httpRequestWithCredentials, (error, loadedSvg) => {
     if (!loadedSvg) {
-      // TODO: Extract.
       injectedElements.splice(injectedElements.indexOf(el), 1)
       ;(el as ElementType) = null
       callback(error)
@@ -115,7 +112,6 @@ const injectElement = (
 
     svg.setAttribute('data-src', elUrl)
 
-    // Copy all the data elements to the svg.
     const elData: Attr[] = [].filter.call(el.attributes, (at: Attr) => {
       return /^data-\w[\w-]*$/.test(at.name)
     })
@@ -128,20 +124,13 @@ const injectElement = (
     })
 
     if (renumerateIRIElements) {
-      // Make sure any internally referenced clipPath ids and their clip-path
-      // references are unique.
-      //
-      // This addresses the issue of having multiple instances of the same SVG
-      // on a page and only the first clipPath id is referenced.
+      // Rewrite IRI element ids to be unique across injection instances.
+      // Browsers skip clipPaths in hidden parent elements, so duplicate ids
+      // cause all but the first instance to lose clipping. Reference:
+      // https://bugzilla.mozilla.org/show_bug.cgi?id=376027.
       //
-      // Browsers often shortcut the SVG Spec and don't use clipPaths contained
-      // in parent elements that are hidden, so if you hide the first SVG
-      // instance on the page, then all other instances lose their clipping.
-      // Reference: https://bugzilla.mozilla.org/show_bug.cgi?id=376027
-
-      // Handle all defs elements that have iri capable attributes as defined by
-      // w3c: http://www.w3.org/TR/SVG/linking.html#processingIRI. Mapping IRI
-      // addressable elements to the properties that can reference them.
+      // IRI-addressable elements mapped to referencing properties per the SVG
+      // spec: http://www.w3.org/TR/SVG/linking.html#processingIRI.
       const iriElementsAndProperties: Record<string, string[]> = {
         clipPath: ['clip-path'],
         'color-profile': ['color-profile'],
@@ -214,7 +203,6 @@ const injectElement = (
       Object.keys(iriElementsAndProperties).forEach((key) => {
         properties = iriElementsAndProperties[key]!
 
-        // All of the properties that can reference this element type.
         let referencingElements: NodeListOf<Element>
         Array.prototype.forEach.call(properties, (property: string) => {
           referencingElements = svg.querySelectorAll('[' + property + ']')
@@ -300,14 +288,12 @@ const injectElement = (
       }
     }
 
-    // Remove any unwanted/invalid namespaces that might have been added by SVG
-    // editing tools.
+    // Remove invalid namespaces that SVG editing tools may have added.
     svg.removeAttribute('xmlns:a')
 
-    // Post page load injected SVGs don't automatically have their script
-    // elements run, so we'll need to make that happen, if requested.
+    // Injected SVGs don't automatically run their script elements, so extract
+    // and evaluate them manually if requested.
 
-    // Find then prune the scripts.
     const scripts = svg.querySelectorAll('script')
     const scriptsToEval: string[] = []
     let script: string | null
@@ -317,7 +303,7 @@ const injectElement = (
       const scriptElement = scripts[i]!
       scriptType = scriptElement.getAttribute('type')
 
-      // Only process javascript types. SVG defaults to 'application/ecmascript'
+      // Only process JavaScript types. SVG defaults to 'application/ecmascript'
       // for unset types.
       /* istanbul ignore else */
       if (
@@ -326,21 +312,17 @@ const injectElement = (
         scriptType === 'application/javascript' ||
         scriptType === 'text/javascript'
       ) {
-        // innerText for IE, textContent for other browsers.
         script = scriptElement.innerText || scriptElement.textContent
 
-        // Stash.
         /* istanbul ignore else */
         if (script) {
           scriptsToEval.push(script)
         }
 
-        // Tidy up and remove the script element since we don't need it anymore.
         svg.removeChild(scriptElement)
       }
     }
 
-    // Run/Eval the scripts if needed.
     if (
       scriptsToEval.length > 0 &&
       (evalScripts === 'always' ||
@@ -351,24 +333,18 @@ const injectElement = (
         l < scriptsToEvalLen;
         l++
       ) {
-        // :NOTE: Yup, this is a form of eval, but it is being used to eval code
-        // the caller has explictely asked to be loaded, and the code is in a
-        // caller defined SVG file... not raw user input.
-        //
-        // Also, the code is evaluated in a closure and not in the global scope.
-        // If you need to put something in global scope, use 'window'.
+        // This is a form of eval, but only for code the caller has explicitly
+        // asked to load from their own SVG files. The code runs in a closure,
+        // not the global scope.
         new Function(scriptsToEval[l]!)(window)
       }
 
-      // Remember we already ran scripts for this svg.
       ranScripts[elUrl] = true
     }
 
-    // :WORKAROUND: IE doesn't evaluate <style> tags in SVGs that are
-    // dynamically added to the page. This trick will trigger IE to read and use
-    // any existing SVG <style> tags.
-    //
-    // Reference: https://github.com/iconic/SVGInjector/issues/23.
+    // Some browsers don't evaluate <style> tags in SVGs that are dynamically
+    // added to the page. This triggers a re-read. Reference:
+    // https://github.com/iconic/SVGInjector/issues/23.
     const styleTags = svg.querySelectorAll('style')
     Array.prototype.forEach.call(styleTags, (styleTag: HTMLStyleElement) => {
       styleTag.textContent += ''
@@ -386,12 +362,7 @@ const injectElement = (
       return
     }
 
-    // Replace the image with the svg.
     el.parentNode.replaceChild(svg, el)
-
-    // Now that we no longer need it, drop references to the original element so
-    // it can be GC'd.
-    // TODO: Extract
     injectedElements.splice(injectedElements.indexOf(el), 1)
     ;(el as ElementType) = null
 

src/load-svg-cached.ts

@@ -26,7 +26,7 @@ const loadSvgCached = (
     // Errors are always refetched.
   }
 
-  // Seed the cache to indicate we are loading this URL.
+  // Seed the cache to indicate this URL is loading.
   cache.set(url, undefined)
   queueRequest(url, callback)
 

src/make-ajax-request.ts

@@ -10,6 +10,10 @@ const makeAjaxRequest = (
 
   httpRequest.onreadystatechange = () => {
     try {
+      // URLs with a .svg extension skip content-type validation. This avoids
+      // failures on the file:// protocol where browsers don't send Content-Type
+      // headers, and is unnecessary when the extension already indicates SVG
+      // content.
       if (!/\.svg/i.test(url) && httpRequest.readyState === 2) {
         const contentType = httpRequest.getResponseHeader('Content-Type')
         if (!contentType) {
@@ -33,6 +37,7 @@ const makeAjaxRequest = (
           )
         }
 
+        // Browsers return status 0 (not 200) for successful file:// loads.
         if (
           httpRequest.status === 200 ||
           (isLocal() && httpRequest.status === 0)
@@ -61,7 +66,6 @@ const makeAjaxRequest = (
 
   httpRequest.withCredentials = httpRequestWithCredentials
 
-  // Defensive check for old browsers that might not have overrideMimeType
   /* istanbul ignore else */
   // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
   if (httpRequest.overrideMimeType) {

src/request-queue.ts

@@ -16,7 +16,7 @@ export const processRequestQueue = (url: string) => {
   }
 
   for (let i = 0, len = callbacks.length; i < len; i++) {
-    // Make these calls async so we avoid blocking the page/renderer.
+    // Async to avoid blocking the renderer.
     setTimeout(() => {
       if (Array.isArray(requestQueue[url])) {
         const cacheValue = cache.get(url)

src/svg-injector.ts

@@ -62,6 +62,7 @@ const SVGInjector = (
       (error, svg) => {
         afterEach(error, svg)
         afterAll(1)
+        // Release the DOM reference to allow GC.
         elements = null
       },
     )

test/eval-scripts.test.ts

@@ -22,6 +22,9 @@ test.describe('eval scripts', () => {
   const expectedDefault =
     '<svg xmlns="http://www.w3.org/2000/svg" width="100%" height="100%" viewBox="0 0 100 100" class="injected-svg inject-me" data-src="/fixtures/script.svg" xmlns:xlink="http://www.w3.org/1999/xlink"><circle cx="50" cy="50" r="15" fill="green"></circle></svg><svg xmlns="http://www.w3.org/2000/svg" width="100%" height="100%" viewBox="0 0 100 100" class="injected-svg inject-me" data-src="/fixtures/script.svg" xmlns:xlink="http://www.w3.org/1999/xlink"><circle cx="50" cy="50" r="15" fill="green"></circle></svg>'
 
+  // Replace window.alert with a counter. Playwright's default dialog handling
+  // would auto-dismiss alerts without tracking them, so we need a controlled
+  // way to verify how many times scripts in the injected SVGs call alert().
   const setupAlerts = async (page: Page) => {
     await page.evaluate(() => {
       ;(window as unknown as AlertWindow).__alertCount = 0

test/local.test.ts

@@ -17,6 +17,9 @@ interface LocalWindow extends Window {
 }
 
 test.describe('local', () => {
+  // Lets the real XHR fail naturally on a file:// page. The browser's
+  // cross-origin restrictions cause the request to fail, triggering the
+  // local-specific error message via isLocal() in make-ajax-request.ts.
   test('not found', async ({ page }) => {
     await addSvgInjector(page)
     await page.goto(blankFileUrl)
@@ -37,6 +40,11 @@ test.describe('local', () => {
     )
   })
 
+  // Playwright browsers enforce cross-origin restrictions on file:// pages, so
+  // a real XHR to a local SVG file would fail. The mock below simulates what a
+  // successful local XHR looks like: status 0 (not 200), no Content-Type
+  // header, and a populated responseXML. This exercises the `isLocal() &&
+  // httpRequest.status === 0` success path in make-ajax-request.ts.
   test('ok', async ({ page }) => {
     await addSvgInjector(page)
     await page.goto(blankFileUrl)

test/svg-injector.test.ts

@@ -160,6 +160,8 @@ test.describe('SVGInjector', () => {
     })
 
     const actual = formatHtml(result.html)
+    // Firefox serialises SVG attributes in a different order than Chromium and
+    // WebKit, so we need browser-specific expected strings.
     const expectedFirefox =
       '<svg width="150" height="150" viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg" class="injected-svg inject-me" data-src="/fixtures/style-tag.svg" xmlns:xlink="http://www.w3.org/1999/xlink"><style>circle {fill: orange;stroke: black;stroke-width: 10px;}</style><circle cx="50" cy="50" r="40"></circle></svg>'
     const expectedDefault =
@@ -559,6 +561,9 @@ test.describe('SVGInjector', () => {
     expect(result.error).toBe('Parent node is null')
   })
 
+  // Some older libraries (e.g. MooTools) replace window.Document with a plain
+  // function. This verifies injection still works when Document is not the
+  // native constructor.
   test('handles Document wrangling via old libs', async ({ page }) => {
     await setupPage(page)