Fix dropdown delegating and some UI problems (#34014)

The old logic is incomplete. See the comment for the improved logic.

Fix #34011

And more fixes:

1. use empty "alt" for images, otherwise the width is not right when the
image fails to load
2. remove the "dropdown icon" patch, because it has been clearly done in
"dropdown.js" now
3. remove the "dropdown filtered item" patch, added a clear callback,
and improve the logic
4. fix global init when a node is removed and added back gain (eg: the
"cherry pick" dialog with a dropdown)

Author: wxiaoguang

modules/templates/util_avatar.go

@@ -34,7 +34,8 @@ func AvatarHTML(src string, size int, class, name string) template.HTML {
 		name = "avatar"
 	}
 
-	return template.HTML(`<img loading="lazy" class="` + class + `" src="` + src + `" title="` + html.EscapeString(name) + `" width="` + sizeStr + `" height="` + sizeStr + `"/>`)
+	// use empty alt, otherwise if the image fails to load, the width will follow the "alt" text's width
+	return template.HTML(`<img loading="lazy" alt="" class="` + class + `" src="` + src + `" title="` + html.EscapeString(name) + `" width="` + sizeStr + `" height="` + sizeStr + `"/>`)
 }
 
 // Avatar renders user avatars. args: user, size (int), class (string)

templates/repo/icon.tmpl

@@ -1,6 +1,6 @@
 {{$avatarLink := (.RelAvatarLink ctx)}}
 {{if $avatarLink}}
-	<img class="ui avatar tw-align-middle" src="{{$avatarLink}}" width="24" height="24" alt="{{.FullName}}">
+	<img class="ui avatar tw-align-middle" src="{{$avatarLink}}" width="24" height="24" alt="" aria-hidden="true">
 {{else if $.IsMirror}}
 	{{svg "octicon-mirror" 24}}
 {{else if $.IsFork}}

web_src/fomantic/build/components/dropdown.js

@@ -752,6 +752,7 @@ $.fn.dropdown = function(parameters) {
               if(module.is.searchSelection() && module.can.show() && module.is.focusedOnSearch() ) {
                 module.show();
               }
+              settings.onAfterFiltered.call(element); // GITEA-PATCH: callback to correctly handle the filtered items
             }
           ;
           if(settings.useLabels && module.has.maxSelections()) {
@@ -3992,6 +3993,8 @@ $.fn.dropdown.settings = {
   onShow        : function(){},
   onHide        : function(){},
 
+  onAfterFiltered: function(){}, // GITEA-PATCH: callback to correctly handle the filtered items
+
   /* Component */
   name           : 'Dropdown',
   namespace      : 'dropdown',

web_src/js/modules/fomantic/dropdown.test.ts

@@ -23,7 +23,27 @@ test('hideScopedEmptyDividers-simple', () => {
 `);
 });
 
-test('hideScopedEmptyDividers-hidden1', () => {
+test('hideScopedEmptyDividers-items-all-filtered', () => {
+  const container = createElementFromHTML(`<div>
+<div class="any"></div>
+<div class="divider"></div>
+<div class="item filtered">a</div>
+<div class="item filtered">b</div>
+<div class="divider"></div>
+<div class="any"></div>
+</div>`);
+  hideScopedEmptyDividers(container);
+  expect(container.innerHTML).toEqual(`
+<div class="any"></div>
+<div class="divider hidden transition"></div>
+<div class="item filtered">a</div>
+<div class="item filtered">b</div>
+<div class="divider"></div>
+<div class="any"></div>
+`);
+});
+
+test('hideScopedEmptyDividers-hide-last', () => {
   const container = createElementFromHTML(`<div>
 <div class="item">a</div>
 <div class="divider" data-scope="b"></div>
@@ -37,7 +57,7 @@ test('hideScopedEmptyDividers-hidden1', () => {
 `);
 });
 
-test('hideScopedEmptyDividers-hidden2', () => {
+test('hideScopedEmptyDividers-scoped-items', () => {
   const container = createElementFromHTML(`<div>
 <div class="item" data-scope="">a</div>
 <div class="divider" data-scope="b"></div>

web_src/js/modules/fomantic/dropdown.ts

@@ -9,26 +9,33 @@ const fomanticDropdownFn = $.fn.dropdown;
 // use our own `$().dropdown` function to patch Fomantic's dropdown module
 export function initAriaDropdownPatch() {
   if ($.fn.dropdown === ariaDropdownFn) throw new Error('initAriaDropdownPatch could only be called once');
+  $.fn.dropdown.settings.onAfterFiltered = onAfterFiltered;
   $.fn.dropdown = ariaDropdownFn;
   $.fn.fomanticExt.onResponseKeepSelectedItem = onResponseKeepSelectedItem;
   (ariaDropdownFn as FomanticInitFunction).settings = fomanticDropdownFn.settings;
 }
 
 // the patched `$.fn.dropdown` function, it passes the arguments to Fomantic's `$.fn.dropdown` function, and:
-// * it does the one-time attaching on the first call
-// * it delegates the `onLabelCreate` to the patched `onLabelCreate` to add necessary aria attributes
+// * it does the one-time element event attaching on the first call
+// * it delegates the module internal functions like `onLabelCreate` to the patched functions to add more features.
 function ariaDropdownFn(this: any, ...args: Parameters<FomanticInitFunction>) {
   const ret = fomanticDropdownFn.apply(this, args);
 
-  // if the `$().dropdown()` call is without arguments, or it has non-string (object) argument,
-  // it means that this call will reset the dropdown internal settings, then we need to re-delegate the callbacks.
-  const needDelegate = (!args.length || typeof args[0] !== 'string');
   for (const el of this) {
     if (!el[ariaPatchKey]) {
-      attachInit(el);
+      // the elements don't belong to the dropdown "module" and won't be reset
+      // so we only need to initialize them once.
+      attachInitElements(el);
     }
-    if (needDelegate) {
-      delegateOne($(el));
+
+    // if the `$().dropdown()` call is without arguments, or it has non-string (object) argument,
+    // it means that such call will reset the dropdown "module" including internal settings,
+    // then we need to re-delegate the callbacks.
+    const $dropdown = $(el);
+    const dropdownModule = $dropdown.data('module-dropdown');
+    if (!dropdownModule.giteaDelegated) {
+      dropdownModule.giteaDelegated = true;
+      delegateDropdownModule($dropdown);
     }
   }
   return ret;
@@ -61,37 +68,17 @@ function updateSelectionLabel(label: HTMLElement) {
   }
 }
 
-function processMenuItems($dropdown: any, dropdownCall: any) {
-  const hideEmptyDividers = dropdownCall('setting', 'hideDividers') === 'empty';
+function onAfterFiltered(this: any) {
+  const $dropdown = $(this);
+  const hideEmptyDividers = $dropdown.dropdown('setting', 'hideDividers') === 'empty';
   const itemsMenu = $dropdown[0].querySelector('.scrolling.menu') || $dropdown[0].querySelector('.menu');
   if (hideEmptyDividers) hideScopedEmptyDividers(itemsMenu);
 }
 
 // delegate the dropdown's template functions and callback functions to add aria attributes.
-function delegateOne($dropdown: any) {
+function delegateDropdownModule($dropdown: any) {
   const dropdownCall = fomanticDropdownFn.bind($dropdown);
 
-  // If there is a "search input" in the "menu", Fomantic will only "focus the input" but not "toggle the menu" when the "dropdown icon" is clicked.
-  // Actually, Fomantic UI doesn't support such layout/usage. It needs to patch the "focusSearch" / "blurSearch" functions to make sure it toggles the menu.
-  const oldFocusSearch = dropdownCall('internal', 'focusSearch');
-  const oldBlurSearch = dropdownCall('internal', 'blurSearch');
-  // * If the "dropdown icon" is clicked, Fomantic calls "focusSearch", so show the menu
-  dropdownCall('internal', 'focusSearch', function (this: any) { dropdownCall('show'); oldFocusSearch.call(this) });
-  // * If the "dropdown icon" is clicked again when the menu is visible, Fomantic calls "blurSearch", so hide the menu
-  dropdownCall('internal', 'blurSearch', function (this: any) { oldBlurSearch.call(this); dropdownCall('hide') });
-
-  const oldFilterItems = dropdownCall('internal', 'filterItems');
-  dropdownCall('internal', 'filterItems', function (this: any, ...args: any[]) {
-    oldFilterItems.call(this, ...args);
-    processMenuItems($dropdown, dropdownCall);
-  });
-
-  const oldShow = dropdownCall('internal', 'show');
-  dropdownCall('internal', 'show', function (this: any, ...args: any[]) {
-    oldShow.call(this, ...args);
-    processMenuItems($dropdown, dropdownCall);
-  });
-
   // the "template" functions are used for dynamic creation (eg: AJAX)
   const dropdownTemplates = {...dropdownCall('setting', 'templates'), t: performance.now()};
   const dropdownTemplatesMenuOld = dropdownTemplates.menu;
@@ -163,9 +150,8 @@ function attachStaticElements(dropdown: HTMLElement, focusable: HTMLElement, men
   }
 }
 
-function attachInit(dropdown: HTMLElement) {
+function attachInitElements(dropdown: HTMLElement) {
   (dropdown as any)[ariaPatchKey] = {};
-  if (dropdown.classList.contains('custom')) return;
 
   // Dropdown has 2 different focusing behaviors
   // * with search input: the input is focused, and it works with aria-activedescendant pointing another sibling element.
@@ -305,9 +291,11 @@ export function hideScopedEmptyDividers(container: Element) {
   const visibleItems: Element[] = [];
   const curScopeVisibleItems: Element[] = [];
   let curScope: string = '', lastVisibleScope: string = '';
-  const isScopedDivider = (item: Element) => item.matches('.divider') && item.hasAttribute('data-scope');
+  const isDivider = (item: Element) => item.classList.contains('divider');
+  const isScopedDivider = (item: Element) => isDivider(item) && item.hasAttribute('data-scope');
   const hideDivider = (item: Element) => item.classList.add('hidden', 'transition'); // dropdown has its own classes to hide items
-
+  const showDivider = (item: Element) => item.classList.remove('hidden', 'transition');
+  const isHidden = (item: Element) => item.classList.contains('hidden') || item.classList.contains('filtered') || item.classList.contains('tw-hidden');
   const handleScopeSwitch = (itemScope: string) => {
     if (curScopeVisibleItems.length === 1 && isScopedDivider(curScopeVisibleItems[0])) {
       hideDivider(curScopeVisibleItems[0]);
@@ -323,34 +311,37 @@ export function hideScopedEmptyDividers(container: Element) {
     curScopeVisibleItems.length = 0;
   };
 
+  // reset hidden dividers
+  queryElems(container, '.divider', showDivider);
+
   // hide the scope dividers if the scope items are empty
   for (const item of container.children) {
     const itemScope = item.getAttribute('data-scope') || '';
     if (itemScope !== curScope) {
       handleScopeSwitch(itemScope);
     }
-    if (!item.classList.contains('filtered') && !item.classList.contains('tw-hidden')) {
+    if (!isHidden(item)) {
       curScopeVisibleItems.push(item as HTMLElement);
     }
   }
   handleScopeSwitch('');
 
   // hide all leading and trailing dividers
   while (visibleItems.length) {
-    if (!visibleItems[0].matches('.divider')) break;
+    if (!isDivider(visibleItems[0])) break;
     hideDivider(visibleItems[0]);
     visibleItems.shift();
   }
   while (visibleItems.length) {
-    if (!visibleItems[visibleItems.length - 1].matches('.divider')) break;
+    if (!isDivider(visibleItems[visibleItems.length - 1])) break;
     hideDivider(visibleItems[visibleItems.length - 1]);
     visibleItems.pop();
   }
   // hide all duplicate dividers, hide current divider if next sibling is still divider
   // no need to update "visibleItems" array since this is the last loop
-  for (const item of visibleItems) {
-    if (!item.matches('.divider')) continue;
-    if (item.nextElementSibling?.matches('.divider')) hideDivider(item);
+  for (let i = 0; i < visibleItems.length - 1; i++) {
+    if (!visibleItems[i].matches('.divider')) continue;
+    if (visibleItems[i + 1].matches('.divider')) hideDivider(visibleItems[i]);
   }
 }
 

web_src/js/modules/observer.ts

@@ -46,9 +46,11 @@ function callGlobalInitFunc(el: HTMLElement) {
   const func = globalInitFuncs[initFunc];
   if (!func) throw new Error(`Global init function "${initFunc}" not found`);
 
+  // when an element node is removed and added again, it should not be re-initialized again.
   type GiteaGlobalInitElement = Partial<HTMLElement> & {_giteaGlobalInited: boolean};
-  if ((el as GiteaGlobalInitElement)._giteaGlobalInited) throw new Error(`Global init function "${initFunc}" already executed`);
+  if ((el as GiteaGlobalInitElement)._giteaGlobalInited) return;
   (el as GiteaGlobalInitElement)._giteaGlobalInited = true;
+
   func(el);
 }