From b573512312d82e894db7aac89f4938a6b61e1e70 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Thu, 7 Nov 2024 04:21:53 +0800
Subject: [PATCH] Correctly query the primary button in a form (#32438)

The "primary button" is used at many places, but sometimes they might
conflict (due to button switch, hidden panel, dropdown menu, etc).

Sometimes we could add a special CSS class for the buttons, but
sometimes not (see the comment of QuickSubmit)

This PR introduces `querySingleVisibleElem` to help to get the correct
primary button (the only visible one), and prevent from querying the
wrong buttons.

Fix #32437

---------

Co-authored-by: silverwind <me@silverwind.io>
---
 web_src/js/features/comp/QuickSubmit.ts |  8 +++++++-
 web_src/js/features/repo-issue-edit.ts  | 26 +++++++++++++------------
 web_src/js/utils/dom.test.ts            | 11 ++++++++++-
 web_src/js/utils/dom.ts                 | 11 +++++++++--
 4 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/web_src/js/features/comp/QuickSubmit.ts b/web_src/js/features/comp/QuickSubmit.ts
index 3ff29f4fac..385acb319f 100644
--- a/web_src/js/features/comp/QuickSubmit.ts
+++ b/web_src/js/features/comp/QuickSubmit.ts
@@ -1,3 +1,5 @@
+import {querySingleVisibleElem} from '../../utils/dom.ts';
+
 export function handleGlobalEnterQuickSubmit(target) {
   let form = target.closest('form');
   if (form) {
@@ -12,7 +14,11 @@ export function handleGlobalEnterQuickSubmit(target) {
   }
   form = target.closest('.ui.form');
   if (form) {
-    form.querySelector('.ui.primary.button')?.click();
+    // A form should only have at most one "primary" button to do quick-submit.
+    // Here we don't use a special class to mark the primary button,
+    // because there could be a lot of forms with a primary button, the quick submit should work out-of-box,
+    // but not keeps asking developers to add that special class again and again (it could be forgotten easily)
+    querySingleVisibleElem<HTMLButtonElement>(form, '.ui.primary.button')?.click();
     return true;
   }
   return false;
diff --git a/web_src/js/features/repo-issue-edit.ts b/web_src/js/features/repo-issue-edit.ts
index 77a76ad3ca..af97ee4eab 100644
--- a/web_src/js/features/repo-issue-edit.ts
+++ b/web_src/js/features/repo-issue-edit.ts
@@ -3,7 +3,7 @@ import {handleReply} from './repo-issue.ts';
 import {getComboMarkdownEditor, initComboMarkdownEditor, ComboMarkdownEditor} from './comp/ComboMarkdownEditor.ts';
 import {POST} from '../modules/fetch.ts';
 import {showErrorToast} from '../modules/toast.ts';
-import {hideElem, showElem} from '../utils/dom.ts';
+import {hideElem, querySingleVisibleElem, showElem} from '../utils/dom.ts';
 import {attachRefIssueContextPopup} from './contextpopup.ts';
 import {initCommentContent, initMarkupContent} from '../markup/content.ts';
 import {triggerUploadStateChanged} from './comp/EditorUpload.ts';
@@ -77,20 +77,22 @@ async function onEditContent(event) {
     }
   };
 
-  comboMarkdownEditor = getComboMarkdownEditor(editContentZone.querySelector('.combo-markdown-editor'));
-  if (!comboMarkdownEditor) {
-    editContentZone.innerHTML = document.querySelector('#issue-comment-editor-template').innerHTML;
-    const saveButton = editContentZone.querySelector('.ui.primary.button');
-    comboMarkdownEditor = await initComboMarkdownEditor(editContentZone.querySelector('.combo-markdown-editor'));
-    const syncUiState = () => saveButton.disabled = comboMarkdownEditor.isUploading();
-    comboMarkdownEditor.container.addEventListener(ComboMarkdownEditor.EventUploadStateChanged, syncUiState);
-    editContentZone.querySelector('.ui.cancel.button').addEventListener('click', cancelAndReset);
-    saveButton.addEventListener('click', saveAndRefresh);
-  }
-
   // Show write/preview tab and copy raw content as needed
   showElem(editContentZone);
   hideElem(renderContent);
+
+  comboMarkdownEditor = getComboMarkdownEditor(editContentZone.querySelector('.combo-markdown-editor'));
+  if (!comboMarkdownEditor) {
+    editContentZone.innerHTML = document.querySelector('#issue-comment-editor-template').innerHTML;
+    const saveButton = querySingleVisibleElem<HTMLButtonElement>(editContentZone, '.ui.primary.button');
+    const cancelButton = querySingleVisibleElem<HTMLButtonElement>(editContentZone, '.ui.cancel.button');
+    comboMarkdownEditor = await initComboMarkdownEditor(editContentZone.querySelector('.combo-markdown-editor'));
+    const syncUiState = () => saveButton.disabled = comboMarkdownEditor.isUploading();
+    comboMarkdownEditor.container.addEventListener(ComboMarkdownEditor.EventUploadStateChanged, syncUiState);
+    cancelButton.addEventListener('click', cancelAndReset);
+    saveButton.addEventListener('click', saveAndRefresh);
+  }
+
   // FIXME: ideally here should reload content and attachment list from backend for existing editor, to avoid losing data
   if (!comboMarkdownEditor.value()) {
     comboMarkdownEditor.value(rawContent.textContent);
diff --git a/web_src/js/utils/dom.test.ts b/web_src/js/utils/dom.test.ts
index 5c235795fd..d7e3a4939e 100644
--- a/web_src/js/utils/dom.test.ts
+++ b/web_src/js/utils/dom.test.ts
@@ -1,4 +1,4 @@
-import {createElementFromAttrs, createElementFromHTML} from './dom.ts';
+import {createElementFromAttrs, createElementFromHTML, querySingleVisibleElem} from './dom.ts';
 
 test('createElementFromHTML', () => {
   expect(createElementFromHTML('<a>foo<span>bar</span></a>').outerHTML).toEqual('<a>foo<span>bar</span></a>');
@@ -16,3 +16,12 @@ test('createElementFromAttrs', () => {
   }, 'txt', createElementFromHTML('<span>inner</span>'));
   expect(el.outerHTML).toEqual('<button id="the-id" class="cls-1 cls-2" disabled="" tabindex="0" data-foo="the-data">txt<span>inner</span></button>');
 });
+
+test('querySingleVisibleElem', () => {
+  let el = createElementFromHTML('<div><span>foo</span></div>');
+  expect(querySingleVisibleElem(el, 'span').textContent).toEqual('foo');
+  el = createElementFromHTML('<div><span style="display: none;">foo</span><span>bar</span></div>');
+  expect(querySingleVisibleElem(el, 'span').textContent).toEqual('bar');
+  el = createElementFromHTML('<div><span>foo</span><span>bar</span></div>');
+  expect(() => querySingleVisibleElem(el, 'span')).toThrowError('Expected exactly one visible element');
+});
diff --git a/web_src/js/utils/dom.ts b/web_src/js/utils/dom.ts
index a6e0fe2854..e2a4c60e84 100644
--- a/web_src/js/utils/dom.ts
+++ b/web_src/js/utils/dom.ts
@@ -269,8 +269,8 @@ export function initSubmitEventPolyfill() {
  */
 export function isElemVisible(element: HTMLElement): boolean {
   if (!element) return false;
-
-  return Boolean(element.offsetWidth || element.offsetHeight || element.getClientRects().length);
+  // checking element.style.display is not necessary for browsers, but it is required by some tests with happy-dom because happy-dom doesn't really do layout
+  return Boolean((element.offsetWidth || element.offsetHeight || element.getClientRects().length) && element.style.display !== 'none');
 }
 
 // replace selected text in a textarea while preserving editor history, e.g. CTRL-Z works after this
@@ -330,3 +330,10 @@ export function animateOnce(el: Element, animationClassName: string): Promise<vo
     el.classList.add(animationClassName);
   });
 }
+
+export function querySingleVisibleElem<T extends HTMLElement>(parent: Element, selector: string): T | null {
+  const elems = parent.querySelectorAll<HTMLElement>(selector);
+  const candidates = Array.from(elems).filter(isElemVisible);
+  if (candidates.length > 1) throw new Error(`Expected exactly one visible element matching selector "${selector}", but found ${candidates.length}`);
+  return candidates.length ? candidates[0] as T : null;
+}