Fix aria.js bugs: incorrect role element problem, mobile focus problem, tippy problem

templates/base/footer_content.tmpl

@@ -21,7 +21,7 @@
 			{{end}}
 			<div class="ui language bottom floating slide up dropdown link item">
 				{{svg "octicon-globe"}}
-				<div class="text">{{.locale.LangName}}</div>
+				<span>{{.locale.LangName}}</span>
 				<div class="menu language-menu">
 					{{range .AllLangs}}
 						<a lang="{{.Lang}}" data-url="{{AppSubUrl}}/?lang={{.Lang}}" class="item {{if eq $.locale.Lang .Lang}}active selected{{end}}">{{.Name}}</a>

templates/repo/issue/view_content/add_reaction.tmpl

@@ -1,5 +1,5 @@
 {{if .ctxData.IsSigned}}
-<div class="item action ui pointing select-reaction dropdown top right" data-action-url="{{.ActionURL}}">
+<div class="item action ui dropdown jump pointing top right select-reaction" data-action-url="{{.ActionURL}}">
 	<a class="add-reaction">
 		{{svg "octicon-smiley"}}
 	</a>

templates/repo/issue/view_content/context_menu.tmpl

@@ -1,5 +1,5 @@
 {{if .ctxData.IsSigned}}
-<div class="item action ui pointing custom dropdown top right context-dropdown">
+<div class="item action ui dropdown jump pointing top right context-dropdown">
 	<a class="context-menu">
 		{{svg "octicon-kebab-horizontal"}}
 	</a>

web_src/js/features/aria.js

@@ -6,42 +6,16 @@ function generateAriaId() {
   return `_aria_auto_id_${ariaIdCounter++}`;
 }
 
-// make the item has role=option, and add an id if there wasn't one yet.
-function prepareMenuItem($item) {
-  if (!$item.attr('id')) $item.attr('id', generateAriaId());
-  $item.attr({'role': 'menuitem', 'tabindex': '-1'});
-  $item.find('a').attr('tabindex', '-1'); // as above, the elements inside the dropdown menu item should not be focusable, the focus should always be on the dropdown primary element.
-}
-
-// when the menu items are loaded from AJAX requests, the items are created dynamically
-const defaultCreateDynamicMenu = $.fn.dropdown.settings.templates.menu;
-$.fn.dropdown.settings.templates.menu = function(response, fields, preserveHTML, className) {
-  const ret = defaultCreateDynamicMenu(response, fields, preserveHTML, className);
-  const $wrapper = $('<div>').append(ret);
-  const $items = $wrapper.find('> .item');
-  $items.each((_, item) => {
-    prepareMenuItem($(item));
-  });
-  return $wrapper.html();
-};
-
 function attachOneDropdownAria($dropdown) {
   if ($dropdown.attr('data-aria-attached')) return;
   $dropdown.attr('data-aria-attached', 1);
 
-  const $textSearch = $dropdown.find('input.search').eq(0);
-  const $focusable = $textSearch.length ? $textSearch : $dropdown; // see comment below
-  if (!$focusable.length) return;
-
-  // prepare menu list
-  const $menu = $dropdown.find('> .menu');
-  if (!$menu.attr('id')) $menu.attr('id', generateAriaId());
-
-  // dropdown has 2 different focusing behaviors
-  // * with search input: the input is focused, and it works perfectly with aria-activedescendant pointing another sibling element.
+  // Dropdown has 2 different focusing behaviors
+  // * with search input: the input is focused, and it works with aria-activedescendant pointing another sibling element.
   // * without search input (but the readonly text), the dropdown itself is focused. then the aria-activedescendant points to the element inside dropdown
+  // Some desktop screen readers may change the focus, but dropdown requires that the focus must be on its primary element, then they don't work well.
 
-  // expected user interactions for dropdown with aria support:
+  // Expected user interactions for dropdown with aria support:
   // * user can use Tab to focus in the dropdown, then the dropdown menu (list) will be shown
   // * user presses Tab on the focused dropdown to move focus to next sibling focusable element (but not the menu item)
   // * user can use arrow key Up/Down to navigate between menu items
@@ -51,25 +25,64 @@ function attachOneDropdownAria($dropdown) {
 
   // TODO: multiple selection is not supported yet.
 
-  $focusable.attr({
-    'role': 'menu',
-    'aria-haspopup': 'menu',
-    'aria-controls': $menu.attr('id'),
-    'aria-expanded': 'false',
-  });
+  const $textSearch = $dropdown.find('input.search').eq(0);
+  const $focusable = $textSearch.length ? $textSearch : $dropdown; // the primary element for focus, see comment above
+  if (!$focusable.length) return;
+
+  // There are 2 possible solutions about the role: combobox or menu. Always use combobox in the future, see "aria.md" for details.
+  const focusableRole = 'button';
+  const listPopupRole = 'menu';
+  const listItemRole = 'menuitem';
+
+  // make the item has role=option/menuitem, and add an id if there wasn't one yet.
+  function prepareMenuItem($item) {
+    if (!$item.attr('id')) $item.attr('id', generateAriaId());
+    $item.attr({'role': listItemRole, 'tabindex': '-1'});
+    $item.find('a').attr('tabindex', '-1'); // as above, the elements inside the dropdown menu item should not be focusable, the focus should always be on the dropdown primary element.
+  }
 
-  if ($dropdown.attr('data-content') && !$dropdown.attr('aria-label')) {
+  // delegate the dropdown's template function to add aria attributes
+  const dropdownTemplates = {...$dropdown.dropdown('setting', 'templates')};
+  const dropdownTemplatesMenuOld = dropdownTemplates.menu;
+  dropdownTemplates.menu = function(response, fields, preserveHTML, className) {
+    // when the dropdown menu items are loaded from AJAX requests, the items are created dynamically
+    const menuItems = dropdownTemplatesMenuOld(response, fields, preserveHTML, className);
+    const $wrapper = $('<div>').append(menuItems);
+    const $items = $wrapper.find('> .item');
+    $items.each((_, item) => prepareMenuItem($(item)));
+    return $wrapper.html();
+  };
+  $dropdown.dropdown('setting', 'templates', dropdownTemplates);
+
+  // use tooltip's content as aria-label if there is no aria-label
+  if ($dropdown.hasClass('tooltip') && $dropdown.attr('data-content') && !$dropdown.attr('aria-label')) {
     $dropdown.attr('aria-label', $dropdown.attr('data-content'));
   }
 
+  // prepare dropdown menu list popup
+  const $menu = $dropdown.find('> .menu');
+  if (!$menu.attr('id')) $menu.attr('id', generateAriaId());
   $menu.find('> .item').each((_, item) => {
     prepareMenuItem($(item));
   });
+  // this role could only be changed after its content is ready, otherwise some browsers+readers (like Chrome+AppleVoice) crash
+  $menu.attr('role', listPopupRole);
+
+  // make the primary element (focusable) aria-friendly
+  $focusable.attr({
+    'role': $focusable.attr('role') ?? focusableRole,
+    'aria-haspopup': listPopupRole,
+    'aria-controls': $menu.attr('id'),
+    'aria-expanded': 'false',
+  });
+
+  // when showing, it has class: ".animating.in"
+  // when hiding, it has class: ".visible.animating.out"
+  const isMenuVisible = () => ($menu.hasClass('visible') && !$menu.hasClass('out')) || $menu.hasClass('in');
 
   // update aria attributes according to current active/selected item
   const refreshAria = () => {
-    const isMenuVisible = !$menu.is('.hidden') && !$menu.is('.animating.out');
-    $focusable.attr('aria-expanded', isMenuVisible ? 'true' : 'false');
+    $focusable.attr('aria-expanded', isMenuVisible() ? 'true' : 'false');
 
     let $active = $menu.find('> .item.active');
     if (!$active.length) $active = $menu.find('> .item.selected'); // it's strange that we need this fallback at the moment
@@ -85,16 +98,59 @@ function attachOneDropdownAria($dropdown) {
       if (!$item) $item = $menu.find('> .item.selected'); // when dropdown filters items by input, there is no "value", so query the "selected" item
       // if the selected item is clickable, then trigger the click event.
       // we can not click any item without check, because Fomantic code might also handle the Enter event. that would result in double click.
-      if ($item && ($item.is('a') || $item.is('.js-aria-clickable'))) $item[0].click();
+      if ($item && ($item.is('a') || $item.hasClass('js-aria-clickable'))) $item[0].click();
     }
   });
 
   // use setTimeout to run the refreshAria in next tick (to make sure the Fomantic UI code has finished its work)
-  const deferredRefreshAria = () => { setTimeout(refreshAria, 0) }; // do not return any value, jQuery has return-value related behaviors.
-  $focusable.on('focus', deferredRefreshAria);
-  $focusable.on('mouseup', deferredRefreshAria);
-  $focusable.on('blur', deferredRefreshAria);
+  // do not return any value, jQuery has return-value related behaviors.
+  const deferredRefreshAria = () => { setTimeout(refreshAria, 0) };
   $dropdown.on('keyup', (e) => { if (e.key.startsWith('Arrow')) deferredRefreshAria(); });
+
+  // if the dropdown has been opened by focus, do not trigger the next click event again.
+  // otherwise the dropdown will be closed immediately, especially on Android with TalkBack
+  // * desktop event sequence: mousedown -> focus -> mouseup -> click
+  // * mobile event sequence: focus -> mousedown -> mouseup -> click
+  // Fomantic may stop propagation of blur event, use capture to make sure we can still get the event
+  // keep the debug code for developers who want to confirm&debug this code for different browsers (without attaching a remote debugger)
+  const showDebug = false;
+  const debug = (msg) => showDebug && $('.page-content').append($('<div>').text(`${$menu.attr('id')} ${msg}, menu visible=${isMenuVisible()}`));
+  let ignoreClickPreEvents = 0, ignoreClickPreVisible = 0;
+  $dropdown[0].addEventListener('mousedown', (e) => {
+    debug(e.type);
+    ignoreClickPreVisible += isMenuVisible() ? 1 : 0;
+    ignoreClickPreEvents++;
+  }, true);
+  $dropdown[0].addEventListener('focus', (e) => {
+    debug(e.type);
+    ignoreClickPreVisible += isMenuVisible() ? 1 : 0;
+    ignoreClickPreEvents++;
+    deferredRefreshAria();
+  }, true);
+  $dropdown[0].addEventListener('blur', (e) => {
+    debug(e.type);
+    ignoreClickPreVisible = ignoreClickPreEvents = 0;
+    deferredRefreshAria();
+  }, true);
+  $dropdown[0].addEventListener('mouseup', (e) => {
+    debug(e.type);
+    setTimeout(() => {
+      debug(`${e.type} (deferred)`);
+      ignoreClickPreVisible = ignoreClickPreEvents = 0;
+      deferredRefreshAria();
+    }, 0);
+  }, true);
+  $dropdown[0].addEventListener('click', (e) => {
+    debug(`${e.type}, pre-visible=${ignoreClickPreVisible}, pre-events=${ignoreClickPreEvents}`);
+    if (isMenuVisible() &&
+      ignoreClickPreVisible !== 2 && // dropdown is switch from invisible to visible
+      ignoreClickPreEvents === 2 // the click event is related to mousedown+focus
+    ) {
+      debug(`${e.type}, stop click propagation`);
+      e.stopPropagation(); // if the dropdown menu has been opened by focus, do not trigger the next click event again
+    }
+    ignoreClickPreEvents = ignoreClickPreVisible = 0;
+  }, true);
 }
 
 export function attachDropdownAria($dropdowns) {

web_src/js/features/aria.md

@@ -30,11 +30,10 @@ There is still a problem: Fomantic UI checkbox is not friendly to screen readers
 ## ARIA Dropdown
 
 There are different solutions:
+
 * combobox + listbox + option
 * menu + menuitem
 
-At the moment, `menu + menuitem` seems to work better with Fomantic UI Dropdown, so we only use it now.
-
 ```html
 <div>
   <input role="combobox" aria-haspopup="listbox" aria-expanded="false" aria-controls="the-menu-listbox" aria-activedescendant="item-id-123456">
@@ -49,6 +48,33 @@ At the moment, `menu + menuitem` seems to work better with Fomantic UI Dropdown,
 
 ## Fomantic UI Dropdown
 
+There are 2 possible solutions about the role: combobox or menu
+
+1. Detect if the dropdown has an input, if yes, it works like a combobox, otherwise it works like a menu
+2. Always use "combobox", never use "menu"
+
+According to the public discussion with fsologureng in chat channel, we think it's better to use "combobox" for all dropdowns.
+
+> On the old web there were many menus implemented with an auto-submit select,
+> but that didn't change the fact that they were selects for screen readers.
+> That is the case with Fomantic dropdowns as used in Gitea.
+> Implementations of auto-submit select menus fell behind in modern web design precisely because they are not usable or accessible."
+>
+> We can mark all "dropdown" as "combobox", never use "menu" in code. Do you think this solution is clear enough?
+>
+> Yes. I think it will provide better accessibility because is more coherent with the current fomantic based implementation.
+
+Reference:
+
+* Combobox:
+  * https://www.w3.org/WAI/ARIA/apg/patterns/combobox/
+  * A combobox is an **input widget** with an associated popup that enables users to select a value for the combobox from
+    a collection of possible values. In some implementations, the popup presents allowed values, while in other implementations,
+    the popup presents suggested values, and users may either select one of the suggestions or type a value.
+* Menu:
+  * https://www.w3.org/WAI/ARIA/apg/patterns/menubar/
+  * A menu is a widget that offers a list of choices to the user, such as a set of **actions or functions**.
+
 ```html
 <!-- read-only dropdown -->
 <div class="ui dropdown"> <!-- focused here, then it's not perfect to use aria-activedescendant to point to the menu item -->

web_src/js/features/common-global.js

@@ -89,9 +89,12 @@ export function initGlobalCommon() {
 
   // Semantic UI modules.
   const $uiDropdowns = $('.ui.dropdown');
-  $uiDropdowns.filter(':not(.custom)').dropdown({
-    fullTextSearch: 'exact'
-  });
+
+  // do not init "custom" dropdowns, "custom" dropdowns are managed by their own code.
+  $uiDropdowns.filter(':not(.custom)').dropdown({fullTextSearch: 'exact'});
+
+  // "jump" means this dropdown is mainly used for "menu" purpose, clicking an item will jump to somewhere else or trigger an action/function.
+  // when a dropdown is used for non-refresh actions with tippy, it must have this "jump" class to hide the tippy when dropdown is closed.
   $uiDropdowns.filter('.jump').dropdown({
     action: 'hide',
     onShow() {
@@ -101,17 +104,20 @@ export function initGlobalCommon() {
     },
     onHide() {
       this._tippy?.enable();
+
+      // hide all tippy elements of items after a while. eg: keyboard Enter to trigger the "Copy Link" in the Issue Context Menu
+      setTimeout(() => {
+        $(this).find('.menu > .item').each((_, item) => {
+          item._tippy?.hide();
+        });
+      }, 2000);
     },
-    fullTextSearch: 'exact'
-  });
-  $uiDropdowns.filter('.slide.up').dropdown({
-    transition: 'slide up',
-    fullTextSearch: 'exact'
-  });
-  $uiDropdowns.filter('.upward').dropdown({
-    direction: 'upward',
-    fullTextSearch: 'exact'
   });
+
+  // special animations/popup-directions
+  $uiDropdowns.filter('.slide.up').dropdown({transition: 'slide up'});
+  $uiDropdowns.filter('.upward').dropdown({direction: 'upward'});
+
   attachDropdownAria($uiDropdowns);
 
   attachCheckboxAria($('.ui.checkbox'));

web_src/js/features/repo-legacy.js

@@ -601,9 +601,6 @@ export function initRepository() {
 }
 
 function initRepoIssueCommentEdit() {
-  // Issue/PR Context Menus
-  $('.comment-header-right .context-dropdown').dropdown({action: 'hide'});
-
   // Edit issue or comment content
   $(document).on('click', '.edit-content', onEditContent);
 

web_src/js/modules/tippy.js

@@ -52,10 +52,11 @@ export function showTemporaryTooltip(target, content) {
     onHidden: (tippy) => {
       if (oldContent) {
         tippy.setContent(oldContent);
+        tippy.setProps({onHidden: undefined});
       } else {
         tippy.destroy();
+        // after destroy, the `_tippy` is detached, it can't do "setProps (etc...)" anymore
       }
-      tippy.setProps({onHidden: undefined});
     },
   });
 }