Improve <SvgIcon> to make it output svg node and optimize performance (#23570)

Before, the Vue `<SvgIcon>` always outputs DOM nodes like: 

```html
<span class="outer-class">
    <svg class="class-name-defined" ...></svg>
</span>
```

The `span` is redundant and I guess such layout and the inconsistent
`class/class-name` attributes would cause bugs sooner or later.

This PR makes the `<SvgIcon>` clear, and it's faster than before,
because it doesn't need to parse the whole SVG string.

Before: 

<details>

![image](https://user-images.githubusercontent.com/2114189/226156474-ce2c57cd-b869-486a-b75b-1eebdac8cdf7.png)

</details>

After:

![image](https://user-images.githubusercontent.com/2114189/226155774-108f49ed-7512-40c3-94a2-a6e8da18063d.png)

---------

Co-authored-by: silverwind <me@silverwind.io>

web_src/js/components/ActionRunStatus.vue

@@ -1,10 +1,10 @@
 <template>
-  <SvgIcon name="octicon-check-circle-fill" class="green" :size="size" :class-name="className" v-if="status === 'success'"/>
+  <SvgIcon name="octicon-check-circle-fill" class="ui text green" :size="size" :class-name="className" v-if="status === 'success'"/>
   <SvgIcon name="octicon-skip" class="ui text grey" :size="size" :class-name="className" v-else-if="status === 'skipped'"/>
   <SvgIcon name="octicon-clock" class="ui text yellow" :size="size" :class-name="className" v-else-if="status === 'waiting'"/>
   <SvgIcon name="octicon-blocked" class="ui text yellow" :size="size" :class-name="className" v-else-if="status === 'blocked'"/>
   <SvgIcon name="octicon-meter" class="ui text yellow" :size="size" :class-name="'job-status-rotate ' + className" v-else-if="status === 'running'"/>
-  <SvgIcon name="octicon-x-circle-fill" class="red" :size="size" v-else/>
+  <SvgIcon name="octicon-x-circle-fill" class="ui text red" :size="size" v-else/>
 </template>
 
 <script>

web_src/js/components/ContextPopup.vue

@@ -3,7 +3,7 @@
     <div v-if="loading" class="ui active centered inline loader"/>
     <div v-if="!loading && issue !== null">
       <p><small>{{ issue.repository.full_name }} on {{ createdAt }}</small></p>
-      <p><svg-icon :name="icon" :class="[color]" /> <strong>{{ issue.title }}</strong> #{{ issue.number }}</p>
+      <p><svg-icon :name="icon" :class="['text', color]" /> <strong>{{ issue.title }}</strong> #{{ issue.number }}</p>
       <p>{{ body }}</p>
       <div>
         <div

web_src/js/svg.js

@@ -105,13 +105,32 @@ export function svg(name, size = 16, className = '') {
 
   const document = parser.parseFromString(svgs[name], 'image/svg+xml');
   const svgNode = document.firstChild;
-  if (size !== 16) svgNode.setAttribute('width', String(size));
-  if (size !== 16) svgNode.setAttribute('height', String(size));
-  // filter array to remove empty string
+  if (size !== 16) {
+    svgNode.setAttribute('width', String(size));
+    svgNode.setAttribute('height', String(size));
+  }
   if (className) svgNode.classList.add(...className.split(/\s+/).filter(Boolean));
   return serializer.serializeToString(svgNode);
 }
 
+export function svgParseOuterInner(name) {
+  const svgStr = svgs[name];
+  if (!svgStr) throw new Error(`Unknown SVG icon: ${name}`);
+
+  // parse the SVG string to 2 parts
+  // * svgInnerHtml: the inner part of the SVG, will be used as the content of the <svg> VNode
+  // * svgOuter: the outer part of the SVG, including attributes
+  // the builtin SVG contents are clean, so it's safe to use `indexOf` to split the content:
+  // eg: <svg outer-attributes>${svgInnerHtml}</svg>
+  const p1 = svgStr.indexOf('>'), p2 = svgStr.lastIndexOf('<');
+  if (p1 === -1 || p2 === -1) throw new Error(`Invalid SVG icon: ${name}`);
+  const svgInnerHtml = svgStr.slice(p1 + 1, p2);
+  const svgOuterHtml = svgStr.slice(0, p1 + 1) + svgStr.slice(p2);
+  const svgDoc = parser.parseFromString(svgOuterHtml, 'image/svg+xml');
+  const svgOuter = svgDoc.firstChild;
+  return {svgOuter, svgInnerHtml};
+}
+
 export const SvgIcon = {
   name: 'SvgIcon',
   props: {
@@ -120,6 +139,32 @@ export const SvgIcon = {
     className: {type: String, default: ''},
   },
   render() {
-    return h('span', {innerHTML: svg(this.name, this.size, this.className)});
+    const {svgOuter, svgInnerHtml} = svgParseOuterInner(this.name);
+    // https://vuejs.org/guide/extras/render-function.html#creating-vnodes
+    // the `^` is used for attr, set SVG attributes like 'width', `aria-hidden`, `viewBox`, etc
+    const attrs = {};
+    for (const attr of svgOuter.attributes) {
+      if (attr.name === 'class') continue;
+      attrs[`^${attr.name}`] = attr.value;
+    }
+    attrs[`^width`] = this.size;
+    attrs[`^height`] = this.size;
+
+    // make the <SvgIcon class="foo" class-name="bar"> classes work together
+    const classes = [];
+    for (const cls of svgOuter.classList) {
+      classes.push(cls);
+    }
+    // TODO: drop the `className/class-name` prop in the future, only use "class" prop
+    if (this.className) {
+      classes.push(...this.className.split(/\s+/).filter(Boolean));
+    }
+
+    // create VNode
+    return h('svg', {
+      ...attrs,
+      class: classes,
+      innerHTML: svgInnerHtml,
+    });
   },
 };

web_src/js/svg.test.js

@@ -1,8 +1,28 @@
 import {expect, test} from 'vitest';
-import {svg} from './svg.js';
+import {svg, SvgIcon, svgParseOuterInner} from './svg.js';
+import {createApp, h} from 'vue';
 
 test('svg', () => {
   expect(svg('octicon-repo')).toMatch(/^<svg/);
   expect(svg('octicon-repo', 16)).toContain('width="16"');
   expect(svg('octicon-repo', 32)).toContain('width="32"');
 });
+
+test('svgParseOuterInner', () => {
+  const {svgOuter, svgInnerHtml} = svgParseOuterInner('octicon-repo');
+  expect(svgOuter.nodeName).toMatch('svg');
+  expect(svgOuter.classList.contains('octicon-repo')).toBeTruthy();
+  expect(svgInnerHtml).toContain('<path');
+});
+
+test('SvgIcon', () => {
+  const root = document.createElement('div');
+  createApp({render: () => h(SvgIcon, {name: 'octicon-link', size: 24, class: 'base', className: 'extra'})}).mount(root);
+  const node = root.firstChild;
+  expect(node.nodeName).toEqual('svg');
+  expect(node.getAttribute('width')).toEqual('24');
+  expect(node.getAttribute('height')).toEqual('24');
+  expect(node.classList.contains('octicon-link')).toBeTruthy();
+  expect(node.classList.contains('base')).toBeTruthy();
+  expect(node.classList.contains('extra')).toBeTruthy();
+});