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 <[email protected]>

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;

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) {
     }
   };
 
+  // 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 = editContentZone.querySelector('.ui.primary.button');
+    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);
-    editContentZone.querySelector('.ui.cancel.button').addEventListener('click', cancelAndReset);
+    cancelButton.addEventListener('click', cancelAndReset);
     saveButton.addEventListener('click', saveAndRefresh);
   }
 
-  // Show write/preview tab and copy raw content as needed
-  showElem(editContentZone);
-  hideElem(renderContent);
   // 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);

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');
+});

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;
+}