refactor: issue comment context menu and buttons (#39771)

1. Support show "Quote reply" shortcut in help menu, fix the TODO
2. Help #36862 to reuse existing "issue/comment context menu"

web_src/css/review.css

@@ -131,20 +131,21 @@
   max-width: calc(100vw - 70px); /* leave enough space on left, and align the page content */
 }
 
-#review-box {
+.ui.button.review-box-button {
   position: relative;
+  padding-right: 0.5em !important; /* override .repository .diff-detail-box .ui.button */
 }
 
-#review-box .review-comments-counter {
+.review-box-button .review-comments-counter {
   background-color: var(--color-primary-light-4);
   color: var(--color-primary-contrast);
 }
 
-#review-box:hover .review-comments-counter {
+.review-box-button:hover .review-comments-counter {
   background-color: var(--color-primary-light-5);
 }
 
-#review-box .review-comments-counter[data-pending-comment-number="0"] {
+.review-box-button .review-comments-counter[data-pending-comment-number="0"] {
   display: none;
 }
 

web_src/js/features/repo-diff.ts

@@ -77,7 +77,7 @@ function initRepoDiffConversationForm() {
 
       // the default behavior is to add a pending review, so if no submitter, it also means "pending_review"
       if (!submitter || submitter?.matches('button[name="pending_review"]')) {
-        const reviewBox = document.querySelector('#review-box')!;
+        const reviewBox = document.querySelector('.review-box-button')!;
         const counter = reviewBox?.querySelector('.review-comments-counter');
         if (!counter) return;
         const num = parseInt(counter.getAttribute('data-pending-comment-number')!) + 1 || 1;

web_src/js/features/repo-issue-edit.test.ts

@@ -1,10 +1,10 @@
-import {tryShortcutQuoteReply} from './repo-issue-edit.ts';
+import {tryQuoteSelectedReply} from './repo-issue-edit.ts';
 
-test('tryShortcutQuoteReply', () => {
+test('tryQuoteSelectedReply', () => {
   const container = document.createElement('div');
   container.innerHTML = `
     <div class="comment-container">
-      <button class="quote-reply">Quote reply</button>
+      <button data-shortcut-handler="quoteSelectedReply">Quote reply</button>
       <div class="render-content markup">Comment text</div>
       <div class="raw-content">Raw text</div>
     </div>
@@ -20,24 +20,24 @@ test('tryShortcutQuoteReply', () => {
   button.addEventListener('click', onClick);
 
   selection.removeAllRanges();
-  expect(tryShortcutQuoteReply()).toBe(false);
+  expect(tryQuoteSelectedReply()).toBe(false);
 
   selection.selectAllChildren(container.querySelector('.raw-content')!);
-  expect(tryShortcutQuoteReply()).toBe(false);
+  expect(tryQuoteSelectedReply()).toBe(false);
 
   const range = document.createRange();
   range.selectNodeContents(container.querySelector('.render-content')!.firstChild!);
   selection.removeAllRanges();
   selection.addRange(range);
   selection.collapseToStart();
-  expect(tryShortcutQuoteReply()).toBe(false);
+  expect(tryQuoteSelectedReply()).toBe(false);
   expect(onClick).not.toHaveBeenCalled();
 
   selection.removeAllRanges();
   selection.addRange(range);
-  expect(tryShortcutQuoteReply()).toBe(true);
+  expect(tryQuoteSelectedReply()).toBe(true);
   expect(onClick).toHaveBeenCalledTimes(1);
 
   button.remove();
-  expect(tryShortcutQuoteReply()).toBe(false);
+  expect(tryQuoteSelectedReply()).toBe(false);
 });

web_src/js/features/repo-issue-edit.ts

@@ -1,4 +1,4 @@
-import {handleReply} from './repo-issue.ts';
+import {handleCommentReply} from './repo-issue.ts';
 import {getComboMarkdownEditor, initComboMarkdownEditor, ComboMarkdownEditor} from './comp/ComboMarkdownEditor.ts';
 import {POST} from '../modules/fetch.ts';
 import {showErrorToast} from '../modules/toast.ts';
@@ -8,12 +8,9 @@ import {triggerUploadStateChanged} from './comp/EditorUpload.ts';
 import {convertHtmlToMarkdown} from '../markup/html2markdown.ts';
 import {applyAreYouSure} from '../modules/are-you-sure.ts';
 import {registerShortcutHandler} from '../modules/shortcut.ts';
+import {registerGlobalEventFunc} from '../modules/observer.ts';
 
-async function tryOnEditContent(e: Event) {
-  const clickTarget = (e.target as HTMLElement).closest('.edit-content');
-  if (!clickTarget) return;
-
-  e.preventDefault();
+async function onConvContentEdit(clickTarget: HTMLElement) {
   const commentContent = clickTarget.closest('.comment-header')!.nextElementSibling!;
   const editContentZone = commentContent.querySelector('.edit-content-zone')!;
   let renderContent = commentContent.querySelector('.render-content')!;
@@ -121,11 +118,7 @@ function extractSelectedMarkdown(container: HTMLElement) {
   return convertHtmlToMarkdown(el);
 }
 
-async function tryOnQuoteReply(e: Event) {
-  const clickTarget = (e.target as HTMLElement).closest('.quote-reply');
-  if (!clickTarget) return;
-
-  e.preventDefault();
+async function onConvContentQuoteReply(clickTarget: HTMLElement) {
   const contentToQuoteId = clickTarget.getAttribute('data-target');
   const targetRawToQuote = document.querySelector<HTMLElement>(`#${contentToQuoteId}.raw-content`)!;
   const targetMarkupToQuote = targetRawToQuote.parentElement!.querySelector<HTMLElement>('.render-content.markup')!;
@@ -134,9 +127,9 @@ async function tryOnQuoteReply(e: Event) {
   const quotedContent = `${contentToQuote.replace(/^/mg, '> ')}\n\n`;
 
   let editor;
-  if (clickTarget.classList.contains('quote-reply-diff')) {
-    const replyBtn = clickTarget.closest('.comment-code-cloud')!.querySelector<HTMLElement>('button.comment-form-reply')!;
-    editor = await handleReply(replyBtn);
+  const btnReply = clickTarget.closest('.comment-code-cloud')?.querySelector<HTMLElement>('button.comment-form-reply');
+  if (btnReply) {
+    editor = await handleCommentReply(btnReply);
   } else {
     // for normal issue/comment page
     editor = getComboMarkdownEditor(document.querySelector('#comment-form .combo-markdown-editor'))!;
@@ -151,7 +144,7 @@ async function tryOnQuoteReply(e: Event) {
   editor.moveCursorToEnd();
 }
 
-export function tryShortcutQuoteReply(): boolean {
+export function tryQuoteSelectedReply(): boolean {
   const selection = window.getSelection();
   if (!selection?.rangeCount || selection.isCollapsed) return false;
   const node = selection.getRangeAt(0).commonAncestorContainer;
@@ -160,7 +153,7 @@ export function tryShortcutQuoteReply(): boolean {
   const commentContainer = el?.closest<HTMLElement>('.render-content.markup')?.closest('.comment-container');
   if (!commentContainer) return false;
 
-  const quoteReplyBtn = commentContainer.querySelector<HTMLElement>('.quote-reply')!;
+  const quoteReplyBtn = commentContainer.querySelector<HTMLElement>('[data-shortcut-handler="quoteSelectedReply"]')!;
   if (!quoteReplyBtn) return false;
 
   quoteReplyBtn.click();
@@ -168,15 +161,10 @@ export function tryShortcutQuoteReply(): boolean {
 }
 
 export function initRepoIssueCommentEdit() {
-  document.addEventListener('click', (e) => {
-    tryOnEditContent(e); // Edit issue or comment content
-    tryOnQuoteReply(e); // Quote reply to the comment editor
-  });
-  // TODO: it is not list in the shortcut help modal at the moment, there are many details:
-  // 1. not every page supports such "r"
-  // 2. "r" only works when there is a ".quote-reply" menu item, but it is not for a single one
-  // To make the framework fully support such mechanism correctly, we might also need to consider about dynamically added elements.
+  registerGlobalEventFunc('click', 'onConvContentQuoteReply', onConvContentQuoteReply);
+  registerGlobalEventFunc('click', 'onConvContentEdit', onConvContentEdit);
+
   // The shortcut "r" was chosen by https://github.com/go-gitea/gitea/issues/5796#issuecomment-5528657082
-  // and GitLab: https://gitlab.com/gitlab-org/gitlab-foss/-/commit/ca5d0c82509cedb94f9bfa4a40e77706a58faafe
-  registerShortcutHandler('r', tryShortcutQuoteReply);
+  // The same to GitHub and GitLab: https://gitlab.com/gitlab-org/gitlab-foss/-/commit/ca5d0c82509cedb94f9bfa4a40e77706a58faafe
+  registerShortcutHandler('quoteSelectedReply', tryQuoteSelectedReply);
 }

web_src/js/features/repo-issue.ts

@@ -17,8 +17,9 @@ import {initRepoIssueSidebar} from './repo-issue-sidebar.ts';
 import {fomanticQuery} from '../modules/fomantic/base.ts';
 import {showFomanticModal} from '../modules/fomantic/modal.ts';
 import {ignoreAreYouSure} from '../modules/are-you-sure.ts';
-import {registerGlobalInitFunc} from '../modules/observer.ts';
+import {registerGlobalEventFunc, registerGlobalInitFunc} from '../modules/observer.ts';
 import type {FomanticApiResponse} from '../types.ts';
+import {performFetchActionRequest} from '../modules/fetch-action.ts';
 
 const {appSubUrl} = window.config;
 
@@ -94,69 +95,57 @@ export function initRepoIssueFilterItemLabel() {
 }
 
 export function initRepoIssueCommentDelete() {
-  // Delete comment
-  document.addEventListener('click', async (e) => {
-    if (!(e.target as HTMLElement).matches('.delete-comment')) return;
-    e.preventDefault();
+  registerGlobalEventFunc('click', 'onConvContentDelete', async (deleteButton) => {
+    if (!window.confirm(deleteButton.getAttribute('data-locale')!)) return;
 
-    const deleteButton = e.target as HTMLElement;
-    if (window.confirm(deleteButton.getAttribute('data-locale')!)) {
-      try {
-        const response = await POST(deleteButton.getAttribute('data-url')!);
-        if (!response.ok) throw new Error('Failed to delete comment');
+    const response = await performFetchActionRequest(deleteButton, {method: 'POST', url: deleteButton.getAttribute('data-url')!});
+    if (!response) return;
 
-        const conversationHolder = deleteButton.closest('.conversation-holder');
-        const parentTimelineItem = deleteButton.closest('.timeline-item');
-        const parentTimelineGroup = deleteButton.closest('.timeline-item-group');
+    const conversationHolder = deleteButton.closest('.conversation-holder');
+    const parentTimelineItem = deleteButton.closest('.timeline-item');
+    const parentTimelineGroup = deleteButton.closest('.timeline-item-group');
 
-        // Check if this was a pending comment.
-        if (conversationHolder?.querySelector('.pending-label')) {
-          const counter = document.querySelector('#review-box .review-comments-counter')!;
-          let num = parseInt(counter?.getAttribute('data-pending-comment-number') || '') - 1 || 0;
-          num = Math.max(num, 0);
-          counter.setAttribute('data-pending-comment-number', String(num));
-          counter.textContent = String(num);
+    // Check if this was a pending comment.
+    if (conversationHolder?.querySelector('.pending-label')) {
+      const counter = document.querySelector('.review-box-button .review-comments-counter')!;
+      let num = parseInt(counter?.getAttribute('data-pending-comment-number') || '') - 1 || 0;
+      num = Math.max(num, 0);
+      counter.setAttribute('data-pending-comment-number', String(num));
+      counter.textContent = String(num);
+    }
+
+    document.querySelector(`#${deleteButton.getAttribute('data-comment-id')}`)?.remove();
+
+    if (conversationHolder && !conversationHolder.querySelector('.comment')) {
+      const path = conversationHolder.getAttribute('data-path');
+      const side = conversationHolder.getAttribute('data-side');
+      const idx = conversationHolder.getAttribute('data-idx');
+      const lineType = conversationHolder.closest('tr')?.getAttribute('data-line-type');
+
+      // the conversation holder could appear either on the "Conversation" page, or the "Files Changed" page
+      // on the Conversation page, there is no parent "tr", so no need to do anything for "add-code-comment"
+      if (lineType) {
+        if (lineType === 'same') {
+          document.querySelector(`[data-path="${CSS.escape(String(path))}"] .add-code-comment[data-idx="${CSS.escape(String(idx))}"]`)!.classList.remove('tw-invisible');
+        } else {
+          document.querySelector(`[data-path="${CSS.escape(String(path))}"] .add-code-comment[data-side="${CSS.escape(String(side))}"][data-idx="${CSS.escape(String(idx))}"]`)!.classList.remove('tw-invisible');
         }
-
-        document.querySelector(`#${deleteButton.getAttribute('data-comment-id')}`)?.remove();
-
-        if (conversationHolder && !conversationHolder.querySelector('.comment')) {
-          const path = conversationHolder.getAttribute('data-path');
-          const side = conversationHolder.getAttribute('data-side');
-          const idx = conversationHolder.getAttribute('data-idx');
-          const lineType = conversationHolder.closest('tr')?.getAttribute('data-line-type');
-
-          // the conversation holder could appear either on the "Conversation" page, or the "Files Changed" page
-          // on the Conversation page, there is no parent "tr", so no need to do anything for "add-code-comment"
-          if (lineType) {
-            if (lineType === 'same') {
-              document.querySelector(`[data-path="${CSS.escape(String(path))}"] .add-code-comment[data-idx="${CSS.escape(String(idx))}"]`)!.classList.remove('tw-invisible');
-            } else {
-              document.querySelector(`[data-path="${CSS.escape(String(path))}"] .add-code-comment[data-side="${CSS.escape(String(side))}"][data-idx="${CSS.escape(String(idx))}"]`)!.classList.remove('tw-invisible');
-            }
-          }
-          conversationHolder.remove();
-        }
-
-        // Check if there is no review content, move the time avatar upward to avoid overlapping the content below.
-        if (!parentTimelineGroup?.querySelector('.timeline-item.comment') && !parentTimelineItem?.querySelector('.conversation-holder')) {
-          const timelineAvatar = parentTimelineGroup?.querySelector('.timeline-avatar');
-          timelineAvatar?.classList.remove('timeline-avatar-offset');
-        }
-      } catch (error) {
-        console.error(error);
       }
+      conversationHolder.remove();
+    }
+
+    // Check if there is no review content, move the time avatar upward to avoid overlapping the content below.
+    if (!parentTimelineGroup?.querySelector('.timeline-item.comment') && !parentTimelineItem?.querySelector('.conversation-holder')) {
+      const timelineAvatar = parentTimelineGroup?.querySelector('.timeline-avatar');
+      timelineAvatar?.classList.remove('timeline-avatar-offset');
     }
   });
 }
 
 export function initRepoIssueCodeCommentCancel() {
-  // Cancel inline code comment
-  document.addEventListener('click', (e) => {
-    if (!(e.target as HTMLElement).matches('.cancel-code-comment')) return;
-
-    const form = (e.target as HTMLElement).closest('form')!;
-    if (form?.classList.contains('comment-form')) {
+  registerGlobalEventFunc('click', 'onCancelCodeComment', (el) => {
+    const form = el.closest('form')!;
+    if (form.classList.contains('comment-form')) {
       hideElem(form);
       showElem(form.closest('.comment-code-cloud')!.querySelectorAll('button.comment-form-reply'));
     } else {
@@ -186,7 +175,7 @@ export function initRepoIssueComments() {
   });
 }
 
-export async function handleReply(el: HTMLElement) {
+export async function handleCommentReply(el: HTMLElement) {
   const form = el.closest('.comment-code-cloud')!.querySelector('.comment-form')!;
   const textarea = form.querySelector('textarea');
 
@@ -242,15 +231,13 @@ export function initRepoPullRequestReview() {
 
   addDelegatedEventListener(document, 'click', 'button.comment-form-reply', (el, e) => {
     e.preventDefault();
-    handleReply(el);
+    handleCommentReply(el);
   });
 
-  // The following part is only for diff views
-  if (!document.querySelector('.repository.pull.diff')) return;
-
-  const elReviewBtn = document.querySelector('.js-btn-review');
-  const elReviewPanel = document.querySelector('.review-box-panel.tippy-target');
-  if (elReviewBtn && elReviewPanel) {
+  registerGlobalInitFunc('initPullReviewBox', (el) => {
+    const elReviewBtn = el.querySelector('.review-box-button')!;
+    const elReviewPanel = el.querySelector('.review-box-panel.tippy-target');
+    if (!elReviewPanel) return;
     const tippy = createTippy(elReviewBtn, {
       content: elReviewPanel,
       theme: 'default',
@@ -260,8 +247,8 @@ export function initRepoPullRequestReview() {
       interactive: true,
       hideOnClick: true,
     });
-    elReviewPanel.querySelector('.close')!.addEventListener('click', () => tippy.hide());
-  }
+    elReviewPanel.querySelector('.close-review-panel')!.addEventListener('click', () => tippy.hide());
+  });
 
   addDelegatedEventListener(document, 'click', '.add-code-comment', async (el, e) => {
     e.preventDefault();

web_src/js/features/repo-legacy.ts

@@ -42,13 +42,13 @@ export function initRepository() {
   registerGlobalInitFunc('initRepoPullRequestUpdate', initRepoPullRequestUpdate);
   registerGlobalInitFunc('initRepoCitationPanel', initRepoCitationPanel);
 
+  // TODO: need to refactor more legacy if-querySelector-based "init" code to "data-global-xxx" framework in the future.
   const pageContent = document.querySelector('.page-content.repository');
   if (!pageContent) return;
 
   initRepoBranchTagSelector();
   initRepoCommentFormAndSidebar();
 
-  // Labels
   initCompLabelEdit('.page-content.repository.labels');
   initRepoMilestone();
   initRepoNew();
@@ -57,18 +57,17 @@ export function initRepository() {
   initRepoSettings();
   initRepoIssueWipNewTitle();
 
-  // Issues
-  if (pageContent.matches('.page-content.repository.view.issue')) {
-    initRepoIssueCommentEdit();
+  initRepoIssueCommentEdit();
+  initRepoIssueCommentDelete();
+  initRepoIssueCodeCommentCancel();
 
+  if (pageContent.matches('.page-content.repository.view.issue')) {
     initRepoIssueTitleEdit();
     initRepoIssueWipToggle();
     initRepoIssueComments();
 
     initRepoIssueReferenceIssue();
 
-    initRepoIssueCommentDelete();
-    initRepoIssueCodeCommentCancel();
     initCompReactionSelector();
   }
 

web_src/js/modules/shortcut.ts

@@ -1,7 +1,7 @@
 import {registerGlobalInitFunc} from './observer.ts';
 import {createElementFromHTML, hideElem, toggleElem} from '../utils/dom.ts';
 import {showFomanticModal} from './fomantic/modal.ts';
-import {html} from '../utils/html.ts';
+import {html, htmlRaw} from '../utils/html.ts';
 
 type ShortcutHandler = (el: HTMLElement) => boolean;
 
@@ -87,6 +87,7 @@ function showShortcutHelp() {
     document.body.append(modal);
   }
 
+  const seenShortcuts = new Set<string>();
   const list = modal.querySelector<HTMLElement>('.shortcut-list')!;
   list.replaceChildren(createElementFromHTML(html`<tr><td><kbd>?</kbd></td><td>Keyboard Shortcuts</td></tr>`));
   for (const elem of document.querySelectorAll<HTMLElement>('[data-shortcut-keys]')) {
@@ -99,15 +100,24 @@ function showShortcutHelp() {
       target.getAttribute('placeholder') ||
       target.getAttribute('title') ||
       getShortcutTextContent(target);
-    list.append(createElementFromHTML(html`<tr><td><kbd>${elem.getAttribute('data-shortcut-keys')}</kbd></td><td>${label}</td></tr>`));
+
+    const shortcutKeys = elem.getAttribute('data-shortcut-keys')!;
+    const shortcutHandler = elem.getAttribute('data-shortcut-handler') ?? '';
+    const shortcutIdentifier = `${shortcutKeys}::${shortcutHandler}`;
+    if (seenShortcuts.has(shortcutIdentifier)) continue;
+    seenShortcuts.add(shortcutIdentifier);
+    let keysHtml = '';
+    for (const keys of shortcutKeys.split(' ')) {
+      keysHtml += `<kbd>${keys}</kbd> `;
+    }
+    list.append(createElementFromHTML(html`<tr><td>${htmlRaw(keysHtml)}</td><td>${label}</td></tr>`));
   }
   showFomanticModal(modal);
 }
 
-export function registerShortcutHandler(fullKey: string, handler: ShortcutHandler) {
-  fullKey = fullKey.toLowerCase();
-  shortcutHandlers[fullKey] ||= [];
-  shortcutHandlers[fullKey].push(handler);
+export function registerShortcutHandler(name: string, handler: ShortcutHandler) {
+  shortcutHandlers[name] ||= [];
+  shortcutHandlers[name].push(handler);
 }
 
 export function initGlobalShortcut() {
@@ -169,20 +179,22 @@ export function initGlobalShortcut() {
     shortcutPresses.push({fullKey, pressTime});
 
     const fullKeys = shortcutPresses.map((p) => p.fullKey).join(' ');
-    for (const handler of shortcutHandlers[fullKeys.toLowerCase()] || []) {
-      if (handler(target)) {
-        e.preventDefault();
-        shortcutPresses.length = 0;
-        return;
-      }
-    }
-
     // It's said that "spaces in aria-keyshortcuts separate alternative shortcuts", but we want to support multi-key shortcuts
     // So here we use our own data attribute to store the full shortcut keys, and match it case-insensitively.
     const matchedElems = document.querySelectorAll<HTMLElement>(`[data-shortcut-keys="${CSS.escape(fullKeys)}" i]`);
     if (!matchedElems.length) return;
     // TODO: if there are multiple matches, maybe we could introduce some priority rules to determine which one to trigger
     const matchedElem = matchedElems[0];
+    const handlerName = matchedElem.getAttribute('data-shortcut-handler')!;
+    if (shortcutHandlers[handlerName]) {
+      for (const handler of shortcutHandlers[handlerName]) {
+        if (handler(target)) break;
+      }
+      e.preventDefault();
+      shortcutPresses.length = 0;
+      return;
+    }
+
     e.preventDefault();
     shortcutPresses.length = 0;
     matchedElem.click();