Refactor frontend unique id & comment

web_src/js/components/DiffCommitSelector.vue

@@ -2,7 +2,7 @@
 import {defineComponent} from 'vue';
 import {SvgIcon} from '../svg.ts';
 import {GET} from '../modules/fetch.ts';
-import {generateAriaId} from '../modules/fomantic/base.ts';
+import {generateUniqueId} from '../utils/dom.ts';
 
 type Commit = {
   id: string,
@@ -35,8 +35,8 @@ export default defineComponent({
       commits: [] as Array<Commit>,
       hoverActivated: false,
       lastReviewCommitSha: '',
-      uniqueIdMenu: generateAriaId(),
-      uniqueIdShowAll: generateAriaId(),
+      uniqueIdMenu: generateUniqueId('diff-commit-selector-menu-'),
+      uniqueIdShowAll: generateUniqueId('diff-commit-selector-show-all-'),
     };
   },
   computed: {

web_src/js/features/comp/ComboMarkdownEditor.ts

@@ -1,7 +1,7 @@
 import '@github/markdown-toolbar-element';
 import '@github/text-expander-element';
 import {attachTribute} from '../tribute.ts';
-import {hideElem, showElem, autosize, isElemVisible} from '../../utils/dom.ts';
+import {hideElem, showElem, autosize, isElemVisible, generateUniqueId} from '../../utils/dom.ts';
 import {
   EventUploadStateChanged,
   initEasyMDEPaste,
@@ -25,8 +25,6 @@ import {createTippy} from '../../modules/tippy.ts';
 import {fomanticQuery} from '../../modules/fomantic/base.ts';
 import type EasyMDE from 'easymde';
 
-let elementIdCounter = 0;
-
 /**
  * validate if the given textarea is non-empty.
  * @param {HTMLTextAreaElement} textarea - The textarea element to be validated.
@@ -125,7 +123,7 @@ export class ComboMarkdownEditor {
   setupTextarea() {
     this.textarea = this.container.querySelector('.markdown-text-editor');
     this.textarea._giteaComboMarkdownEditor = this;
-    this.textarea.id = `_combo_markdown_editor_${String(elementIdCounter++)}`;
+    this.textarea.id = generateUniqueId(`_combo_markdown_editor_`);
     this.textarea.addEventListener('input', () => triggerEditorContentChanged(this.container));
     this.applyEditorHeights(this.textarea, this.options.editorHeights);
 
@@ -213,16 +211,16 @@ export class ComboMarkdownEditor {
 
     // Fomantic Tab requires the "data-tab" to be globally unique.
     // So here it uses our defined "data-tab-for" and "data-tab-panel" to generate the "data-tab" attribute for Fomantic.
+    const uniqueIdSuffix = generateUniqueId();
     this.tabEditor = Array.from(tabs).find((tab) => tab.getAttribute('data-tab-for') === 'markdown-writer');
     this.tabPreviewer = Array.from(tabs).find((tab) => tab.getAttribute('data-tab-for') === 'markdown-previewer');
-    this.tabEditor.setAttribute('data-tab', `markdown-writer-${elementIdCounter}`);
-    this.tabPreviewer.setAttribute('data-tab', `markdown-previewer-${elementIdCounter}`);
+    this.tabEditor.setAttribute('data-tab', `markdown-writer-${uniqueIdSuffix}`);
+    this.tabPreviewer.setAttribute('data-tab', `markdown-previewer-${uniqueIdSuffix}`);
 
     const panelEditor = this.container.querySelector('.ui.tab[data-tab-panel="markdown-writer"]');
     const panelPreviewer = this.container.querySelector('.ui.tab[data-tab-panel="markdown-previewer"]');
-    panelEditor.setAttribute('data-tab', `markdown-writer-${elementIdCounter}`);
-    panelPreviewer.setAttribute('data-tab', `markdown-previewer-${elementIdCounter}`);
-    elementIdCounter++;
+    panelEditor.setAttribute('data-tab', `markdown-writer-${uniqueIdSuffix}`);
+    panelPreviewer.setAttribute('data-tab', `markdown-previewer-${uniqueIdSuffix}`);
 
     this.tabEditor.addEventListener('click', () => {
       requestAnimationFrame(() => {

web_src/js/modules/fomantic/base.ts

@@ -1,9 +1,5 @@
 import $ from 'jquery';
-let ariaIdCounter = 0;
-
-export function generateAriaId() {
-  return `_aria_auto_id_${ariaIdCounter++}`;
-}
+import {generateUniqueId} from '../../utils/dom.ts';
 
 export function linkLabelAndInput(label: Element, input: Element) {
   const labelFor = label.getAttribute('for');
@@ -12,7 +8,7 @@ export function linkLabelAndInput(label: Element, input: Element) {
   if (inputId && !labelFor) { // missing "for"
     label.setAttribute('for', inputId);
   } else if (!inputId && !labelFor) { // missing both "id" and "for"
-    const id = generateAriaId();
+    const id = generateUniqueId('_aria_label_input_');
     input.setAttribute('id', id);
     label.setAttribute('for', id);
   }

web_src/js/modules/fomantic/dropdown.ts

@@ -1,7 +1,6 @@
 import $ from 'jquery';
-import {generateAriaId} from './base.ts';
 import type {FomanticInitFunction} from '../../types.ts';
-import {queryElems} from '../../utils/dom.ts';
+import {generateUniqueId, queryElems} from '../../utils/dom.ts';
 
 const ariaPatchKey = '_giteaAriaPatchDropdown';
 const fomanticDropdownFn = $.fn.dropdown;
@@ -47,7 +46,7 @@ function ariaDropdownFn(this: any, ...args: Parameters<FomanticInitFunction>) {
 // make the item has role=option/menuitem, add an id if there wasn't one yet, make items as non-focusable
 // the elements inside the dropdown menu item should not be focusable, the focus should always be on the dropdown primary element.
 function updateMenuItem(dropdown: HTMLElement, item: HTMLElement) {
-  if (!item.id) item.id = generateAriaId();
+  if (!item.id) item.id = generateUniqueId('_aria_dropdown_item_');
   item.setAttribute('role', (dropdown as any)[ariaPatchKey].listItemRole);
   item.setAttribute('tabindex', '-1');
   for (const el of item.querySelectorAll('a, input, button')) el.setAttribute('tabindex', '-1');
@@ -59,7 +58,7 @@ function updateMenuItem(dropdown: HTMLElement, item: HTMLElement) {
 function updateSelectionLabel(label: HTMLElement) {
   // the "label" is like this: "<a|div class="ui label" data-value="1">the-label-name <i|svg class="delete icon"/></a>"
   if (!label.id) {
-    label.id = generateAriaId();
+    label.id = generateUniqueId('_aria_dropdown_label_');
   }
   label.tabIndex = -1;
 
@@ -127,7 +126,7 @@ function delegateDropdownModule($dropdown: any) {
 function attachStaticElements(dropdown: HTMLElement, focusable: HTMLElement, menu: HTMLElement) {
   // prepare static dropdown menu list popup
   if (!menu.id) {
-    menu.id = generateAriaId();
+    menu.id = generateUniqueId('_aria_dropdown_menu_');
   }
 
   $(menu).find('> .item').each((_, item) => updateMenuItem(dropdown, item));

web_src/js/utils/dom.ts

@@ -290,14 +290,12 @@ export function isElemVisible(el: HTMLElement): boolean {
 export function replaceTextareaSelection(textarea: HTMLTextAreaElement, text: string) {
   const before = textarea.value.slice(0, textarea.selectionStart ?? undefined);
   const after = textarea.value.slice(textarea.selectionEnd ?? undefined);
-  let success = true;
+  let success = false;
 
   textarea.contentEditable = 'true';
   try {
     success = document.execCommand('insertText', false, text); // eslint-disable-line @typescript-eslint/no-deprecated
-  } catch {
-    success = false;
-  }
+  } catch {} // ignore the error if execCommand is not supported or failed
   textarea.contentEditable = 'false';
 
   if (success && !textarea.value.slice(0, textarea.selectionStart ?? undefined).endsWith(text)) {
@@ -310,11 +308,10 @@ export function replaceTextareaSelection(textarea: HTMLTextAreaElement, text: st
   }
 }
 
-// Warning: Do not enter any unsanitized variables here
 export function createElementFromHTML<T extends HTMLElement>(htmlString: string): T {
   htmlString = htmlString.trim();
-  // some tags like "tr" are special, it must use a correct parent container to create
-  // eslint-disable-next-line github/unescaped-html-literal -- FIXME: maybe we need to use other approaches to create elements from HTML, e.g. using DOMParser
+  // There is no way to create some elements without a proper parent, jQuery's approach: https://github.com/jquery/jquery/blob/main/src/manipulation/wrapMap.js
+  // eslint-disable-next-line github/unescaped-html-literal
   if (htmlString.startsWith('<tr')) {
     const container = document.createElement('table');
     container.innerHTML = htmlString;
@@ -364,14 +361,19 @@ export function addDelegatedEventListener<T extends HTMLElement, E extends Event
     const elem = (e.target as HTMLElement).closest(selector);
     // It strictly checks "parent contains the target elem" to avoid side effects of selector running on outside the parent.
     // Keep in mind that the elem could have been removed from parent by other event handlers before this event handler is called.
-    // For example: tippy popup item, the tippy popup could be hidden and removed from DOM before this.
-    // It is caller's responsibility make sure the elem is still in parent's DOM when this event handler is called.
+    // For example, tippy popup item, the tippy popup could be hidden and removed from DOM before this.
+    // It is the caller's responsibility to make sure the elem is still in parent's DOM when this event handler is called.
     if (!elem || (parent !== document && !parent.contains(elem))) return;
     listener(elem as T, e as E);
   }, options);
 }
 
-/** Returns whether a click event is a left-click without any modifiers held */
+// Returns whether a click event is a left-click without any modifiers held
 export function isPlainClick(e: MouseEvent) {
   return e.button === 0 && !e.ctrlKey && !e.metaKey && !e.altKey && !e.shiftKey;
 }
+
+let uniqueIdCounter = 0;
+export function generateUniqueId(prefix: string = ''): string {
+  return `${prefix}${uniqueIdCounter++}`;
+}

web_src/js/utils/html.ts

@@ -17,7 +17,7 @@ export function html(tmpl: TemplateStringsArray, ...parts: Array<any>): string {
   let output = tmpl[0];
   for (let i = 0; i < parts.length; i++) {
     const value = parts[i];
-    const valueEscaped = (value instanceof rawObject) ? value.toString() : htmlEscape(String(parts[i]));
+    const valueEscaped = (value instanceof rawObject) ? value.toString() : htmlEscape(String(value));
     output = output + valueEscaped + tmpl[i + 1];
   }
   return output;