From c05082669b53f3cb114d4b34fb4468925281fb73 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 5 Jul 2025 23:21:53 +0800 Subject: [PATCH] Refactor frontend unique id & comment (#34958) * there is no bug of the "unique element id", but duplicate code, this PR just merges the duplicate "element id" logic and move the function from "fomaintic" to "dom" * improve comments * make "git commit graph" page update pagination links correctly --- web_src/js/components/DiffCommitSelector.vue | 6 ++--- .../js/features/comp/ComboMarkdownEditor.ts | 16 ++++++-------- web_src/js/features/repo-graph.ts | 2 +- web_src/js/modules/fomantic/base.ts | 8 ++----- web_src/js/modules/fomantic/dropdown.ts | 9 ++++---- web_src/js/utils/dom.ts | 22 ++++++++++--------- web_src/js/utils/html.ts | 2 +- 7 files changed, 30 insertions(+), 35 deletions(-) diff --git a/web_src/js/components/DiffCommitSelector.vue b/web_src/js/components/DiffCommitSelector.vue index a375343979..4b18694bd2 100644 --- a/web_src/js/components/DiffCommitSelector.vue +++ b/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 {generateElemId} from '../utils/dom.ts'; type Commit = { id: string, @@ -35,8 +35,8 @@ export default defineComponent({ commits: [] as Array, hoverActivated: false, lastReviewCommitSha: '', - uniqueIdMenu: generateAriaId(), - uniqueIdShowAll: generateAriaId(), + uniqueIdMenu: generateElemId('diff-commit-selector-menu-'), + uniqueIdShowAll: generateElemId('diff-commit-selector-show-all-'), }; }, computed: { diff --git a/web_src/js/features/comp/ComboMarkdownEditor.ts b/web_src/js/features/comp/ComboMarkdownEditor.ts index d3773a89c4..afa3957e21 100644 --- a/web_src/js/features/comp/ComboMarkdownEditor.ts +++ b/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, generateElemId} 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 = generateElemId(`_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 tabIdSuffix = generateElemId(); 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-${tabIdSuffix}`); + this.tabPreviewer.setAttribute('data-tab', `markdown-previewer-${tabIdSuffix}`); 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-${tabIdSuffix}`); + panelPreviewer.setAttribute('data-tab', `markdown-previewer-${tabIdSuffix}`); this.tabEditor.addEventListener('click', () => { requestAnimationFrame(() => { diff --git a/web_src/js/features/repo-graph.ts b/web_src/js/features/repo-graph.ts index a1a88f1c0f..036a55f715 100644 --- a/web_src/js/features/repo-graph.ts +++ b/web_src/js/features/repo-graph.ts @@ -18,7 +18,7 @@ export function initRepoGraphGit() { const params = new URLSearchParams(window.location.search); params.set('mode', mode); window.history.replaceState(null, '', `?${params.toString()}`); - for (const link of document.querySelectorAll('#pagination .pagination a')) { + for (const link of document.querySelectorAll('#git-graph-body .pagination a')) { const href = link.getAttribute('href'); if (!href) continue; const url = new URL(href, window.location.href); diff --git a/web_src/js/modules/fomantic/base.ts b/web_src/js/modules/fomantic/base.ts index 18f91932a9..1970941e18 100644 --- a/web_src/js/modules/fomantic/base.ts +++ b/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 {generateElemId} 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 = generateElemId('_aria_label_input_'); input.setAttribute('id', id); label.setAttribute('for', id); } diff --git a/web_src/js/modules/fomantic/dropdown.ts b/web_src/js/modules/fomantic/dropdown.ts index ccc22073d7..2af428f24e 100644 --- a/web_src/js/modules/fomantic/dropdown.ts +++ b/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 {generateElemId, queryElems} from '../../utils/dom.ts'; const ariaPatchKey = '_giteaAriaPatchDropdown'; const fomanticDropdownFn = $.fn.dropdown; @@ -47,7 +46,7 @@ function ariaDropdownFn(this: any, ...args: Parameters) { // 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 = generateElemId('_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: "the-label-name " if (!label.id) { - label.id = generateAriaId(); + label.id = generateElemId('_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 = generateElemId('_aria_dropdown_menu_'); } $(menu).find('> .item').each((_, item) => updateMenuItem(dropdown, item)); diff --git a/web_src/js/utils/dom.ts b/web_src/js/utils/dom.ts index 8b540cebb1..3b14b9bcea 100644 --- a/web_src/js/utils/dom.ts +++ b/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(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('