From 939554372eb921e5e5cbb9b2708fc2375bc47a1c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Mar 2023 00:25:10 +0800 Subject: [PATCH 01/13] fix aria.js --- templates/base/footer_content.tmpl | 2 +- .../repo/issue/view_content/add_reaction.tmpl | 2 +- .../repo/issue/view_content/context_menu.tmpl | 2 +- web_src/js/features/aria.js | 144 ++++++++++++------ web_src/js/features/aria.md | 30 +++- web_src/js/features/common-global.js | 30 ++-- web_src/js/features/repo-legacy.js | 3 - web_src/js/modules/tippy.js | 3 +- 8 files changed, 151 insertions(+), 65 deletions(-) diff --git a/templates/base/footer_content.tmpl b/templates/base/footer_content.tmpl index c3b96a0245a9e..53d0a2c77cf18 100644 --- a/templates/base/footer_content.tmpl +++ b/templates/base/footer_content.tmpl @@ -21,7 +21,7 @@ {{end}}
{{svg "octicon-globe"}} -
{{.locale.LangName}}
+ {{.locale.LangName}}
{{range .AllLangs}} {{.Name}} diff --git a/templates/repo/issue/view_content/add_reaction.tmpl b/templates/repo/issue/view_content/add_reaction.tmpl index 692d09e67980b..94c1813bf7735 100644 --- a/templates/repo/issue/view_content/add_reaction.tmpl +++ b/templates/repo/issue/view_content/add_reaction.tmpl @@ -1,5 +1,5 @@ {{if .ctxData.IsSigned}} -
+
{{svg "octicon-smiley"}} diff --git a/templates/repo/issue/view_content/context_menu.tmpl b/templates/repo/issue/view_content/context_menu.tmpl index c073c74ea32f0..0cc207afd4d90 100644 --- a/templates/repo/issue/view_content/context_menu.tmpl +++ b/templates/repo/issue/view_content/context_menu.tmpl @@ -1,5 +1,5 @@ {{if .ctxData.IsSigned}} -
+
{{svg "octicon-kebab-horizontal"}} diff --git a/web_src/js/features/aria.js b/web_src/js/features/aria.js index 46944336adc10..7872f56a60911 100644 --- a/web_src/js/features/aria.js +++ b/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 = $('
').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 = $('
').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($('
').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) { diff --git a/web_src/js/features/aria.md b/web_src/js/features/aria.md index a55344de47d96..ffe525c04360c 100644 --- a/web_src/js/features/aria.md +++ b/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
@@ -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
diff --git a/web_src/js/features/common-global.js b/web_src/js/features/common-global.js index 4fa6942467ee3..8258fa7477de7 100644 --- a/web_src/js/features/common-global.js +++ b/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')); diff --git a/web_src/js/features/repo-legacy.js b/web_src/js/features/repo-legacy.js index 70542ad883e9c..986fd0948fdb0 100644 --- a/web_src/js/features/repo-legacy.js +++ b/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); diff --git a/web_src/js/modules/tippy.js b/web_src/js/modules/tippy.js index ce8f0369f11c9..f57821204db89 100644 --- a/web_src/js/modules/tippy.js +++ b/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}); }, }); } From f1484ee880d22d8cd0b944a470a0c8639df40999 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Mar 2023 01:01:36 +0800 Subject: [PATCH 02/13] update document --- web_src/js/features/aria.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/web_src/js/features/aria.md b/web_src/js/features/aria.md index ffe525c04360c..6fc372d148317 100644 --- a/web_src/js/features/aria.md +++ b/web_src/js/features/aria.md @@ -53,16 +53,18 @@ 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. +According to the public discussion with fsologureng in chat channel: -> On the old web there were many menus implemented with an auto-submit select, +> 2023-03-13 14:52:00 UTC +> +> fsologureng: 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? +> wxiaoguang: 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. +> fsologureng: Yes. I think it will provide better accessibility because is more coherent with the current fomantic based implementation. Reference: From cb2c573a9c09e5db786f006d19048853005cc88f Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Mar 2023 01:16:55 +0800 Subject: [PATCH 03/13] implement the combobox layout suggested by #19861 --- web_src/js/features/aria.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/web_src/js/features/aria.js b/web_src/js/features/aria.js index 7872f56a60911..0f9aa7eed1183 100644 --- a/web_src/js/features/aria.js +++ b/web_src/js/features/aria.js @@ -29,10 +29,14 @@ function attachOneDropdownAria($dropdown) { 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'; + // There are 2 possible solutions about the role: combobox or menu. + // The idea is that if there is an input, then it's a combobox, otherwise it's a menu. + // Since #19861 we have prepared the "combobox" solution, but didn't get enough time to put it into practice and test before. + const isComboBox = $dropdown.find('input').length > 0; + + const focusableRole = isComboBox ? 'combobox' : 'button'; + const listPopupRole = isComboBox ? 'listbox' : 'menu'; + const listItemRole = isComboBox ? 'option' : 'menuitem'; // make the item has role=option/menuitem, and add an id if there wasn't one yet. function prepareMenuItem($item) { From 15285595298994035f4d5e0aba1c44d026c4550b Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Mar 2023 09:43:23 +0800 Subject: [PATCH 04/13] wrap comment lines, check "is hidden" before hide item tippy --- web_src/js/features/common-global.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/web_src/js/features/common-global.js b/web_src/js/features/common-global.js index 8258fa7477de7..010e4956ee7e5 100644 --- a/web_src/js/features/common-global.js +++ b/web_src/js/features/common-global.js @@ -93,8 +93,9 @@ export function initGlobalCommon() { // 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. + // The "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() { @@ -105,11 +106,14 @@ 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 + // hide all tippy elements of items after a while. eg: use Enter to click "Copy Link" in the Issue Context Menu setTimeout(() => { - $(this).find('.menu > .item').each((_, item) => { - item._tippy?.hide(); - }); + const $dropdown = $(this); + if ($dropdown.dropdown('is hidden')) { + $(this).find('.menu > .item').each((_, item) => { + item._tippy?.hide(); + }); + } }, 2000); }, }); From 1fce62ba04fae79789b87de930bae68323b40e12 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Mar 2023 09:59:31 +0800 Subject: [PATCH 05/13] wrap comments --- web_src/js/features/common-global.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/web_src/js/features/common-global.js b/web_src/js/features/common-global.js index 010e4956ee7e5..a7118068bb359 100644 --- a/web_src/js/features/common-global.js +++ b/web_src/js/features/common-global.js @@ -93,8 +93,9 @@ export function initGlobalCommon() { // do not init "custom" dropdowns, "custom" dropdowns are managed by their own code. $uiDropdowns.filter(':not(.custom)').dropdown({fullTextSearch: 'exact'}); - // The "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, + // The "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', From 86ef1f91034b71f2099d2e7c996f5af6e8f96a97 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Mar 2023 10:38:57 +0800 Subject: [PATCH 06/13] skip "custom" dropdowns in attachOneDropdownAria --- web_src/js/features/aria.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web_src/js/features/aria.js b/web_src/js/features/aria.js index 0f9aa7eed1183..3bb7781b4712a 100644 --- a/web_src/js/features/aria.js +++ b/web_src/js/features/aria.js @@ -7,7 +7,7 @@ function generateAriaId() { } function attachOneDropdownAria($dropdown) { - if ($dropdown.attr('data-aria-attached')) return; + if ($dropdown.attr('data-aria-attached') || $dropdown.hasClass('custom')) return; $dropdown.attr('data-aria-attached', 1); // Dropdown has 2 different focusing behaviors From 4fdc9499b5fb04671be7968396b6530e34b6d157 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Mar 2023 10:57:23 +0800 Subject: [PATCH 07/13] sync original chat messages for record --- web_src/js/features/aria.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/web_src/js/features/aria.md b/web_src/js/features/aria.md index 6fc372d148317..09f134b262b85 100644 --- a/web_src/js/features/aria.md +++ b/web_src/js/features/aria.md @@ -57,12 +57,15 @@ According to the public discussion with fsologureng in chat channel: > 2023-03-13 14:52:00 UTC > -> fsologureng: On the old web there were many menus implemented with an auto-submit select, +> wxiaoguang: Yup, the "language dropdown" is arguable (menu or combobox). It's the only one we need to mark it as combobox if you strongly prefer. +> But for others (Create Repo / Profile / Reaction Menu / Issue Context Menu), I think they are not suitable to be combobox. +> +> fsologureng: I repeat an argument stated in my first comment: "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." > -> wxiaoguang: We can mark all "dropdown" as "combobox", never use "menu" in code. Do you think this solution is clear enough? +> wxiaoguang: If you strongly prefer, we can mark all "dropdown" as "combobox", never use "menu" in code. Do you think this solution is clear enough? > > fsologureng: Yes. I think it will provide better accessibility because is more coherent with the current fomantic based implementation. From c7dda873d74ab7da48d4fe3784234149cab00ea4 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 15 Mar 2023 10:48:27 +0800 Subject: [PATCH 08/13] clarify the "item active" logic, use "removeAttr" instead of "attr(..., '')" --- web_src/js/features/aria.js | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/web_src/js/features/aria.js b/web_src/js/features/aria.js index 3bb7781b4712a..3e5ea0a1a2f51 100644 --- a/web_src/js/features/aria.js +++ b/web_src/js/features/aria.js @@ -86,13 +86,21 @@ function attachOneDropdownAria($dropdown) { // update aria attributes according to current active/selected item const refreshAria = () => { - $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 - - // if there is an active item, use its id. if no active item, then the empty string is set - $focusable.attr('aria-activedescendant', $active.attr('id')); + const menuVisible = isMenuVisible(); + $focusable.attr('aria-expanded', menuVisible ? 'true' : 'false'); + + // if there is an active item, use it (the user is navigating between items) + // otherwise use the "selected" for combobox (for the last selected item) + let $active = $menu.find('> .item.active, > .item.selected'); + + // if there is an active item, use its id. if no active item or the dropdown is used as menu and is hidden, empty the active item + const activeId = $active.attr('id'); + if (menuVisible && activeId) { + $focusable.attr('aria-activedescendant', $active.attr('id')); + } else if (!isComboBox && !menuVisible) { + $focusable.removeAttr('aria-activedescendant'); + $active.removeClass('active').removeClass('selected'); + } }; $dropdown.on('keydown', (e) => { From 696a12060d580073cf5514e4529138e0d7eda9de Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 15 Mar 2023 11:32:40 +0800 Subject: [PATCH 09/13] add a comment of annoying Chrome crash for future developers, fix lint --- web_src/js/features/aria.js | 2 +- web_src/js/features/common-global.js | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/web_src/js/features/aria.js b/web_src/js/features/aria.js index 3e5ea0a1a2f51..3147a87187da5 100644 --- a/web_src/js/features/aria.js +++ b/web_src/js/features/aria.js @@ -91,7 +91,7 @@ function attachOneDropdownAria($dropdown) { // if there is an active item, use it (the user is navigating between items) // otherwise use the "selected" for combobox (for the last selected item) - let $active = $menu.find('> .item.active, > .item.selected'); + const $active = $menu.find('> .item.active, > .item.selected'); // if there is an active item, use its id. if no active item or the dropdown is used as menu and is hidden, empty the active item const activeId = $active.attr('id'); diff --git a/web_src/js/features/common-global.js b/web_src/js/features/common-global.js index cdb9132805d39..dfde4c1b90b34 100644 --- a/web_src/js/features/common-global.js +++ b/web_src/js/features/common-global.js @@ -91,6 +91,8 @@ export function initGlobalCommon() { const $uiDropdowns = $('.ui.dropdown'); // do not init "custom" dropdowns, "custom" dropdowns are managed by their own code. + // NOTICE: some old Android Chrome browsers has a crash bug, this line triggers the bug, + // the browser crashes when users click the Issue Reaction Menu button ("return" before this line, no crash. after, crash) $uiDropdowns.filter(':not(.custom)').dropdown({fullTextSearch: 'exact'}); // The "jump" means this dropdown is mainly used for "menu" purpose, From 9bad02d09c171259cd824918e69c2a0ab8ce1301 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 15 Mar 2023 12:11:04 +0800 Subject: [PATCH 10/13] add more comments, and use a small delay to avoid UI lag (due to Fomantic UI animation) --- web_src/js/features/aria.js | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/web_src/js/features/aria.js b/web_src/js/features/aria.js index 3147a87187da5..8230ba0a85b3e 100644 --- a/web_src/js/features/aria.js +++ b/web_src/js/features/aria.js @@ -38,14 +38,16 @@ function attachOneDropdownAria($dropdown) { const listPopupRole = isComboBox ? 'listbox' : 'menu'; const listItemRole = isComboBox ? 'option' : 'menuitem'; - // make the item has role=option/menuitem, and add an id if there wasn't one yet. + // make the item has role=option/menuitem, add an id if there wasn't one yet, make items as non-focusable + // the elements inside the dropdown menu item should not be focusable, the focus should always be on the dropdown primary element. 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. + $item.find('a').attr('tabindex', '-1'); } - // delegate the dropdown's template function to add aria attributes + // delegate the dropdown's template function to add aria attributes. + // the "template" functions are used for dynamic creation (eg: AJAX) const dropdownTemplates = {...$dropdown.dropdown('setting', 'templates')}; const dropdownTemplatesMenuOld = dropdownTemplates.menu; dropdownTemplates.menu = function(response, fields, preserveHTML, className) { @@ -92,12 +94,11 @@ function attachOneDropdownAria($dropdown) { // if there is an active item, use it (the user is navigating between items) // otherwise use the "selected" for combobox (for the last selected item) const $active = $menu.find('> .item.active, > .item.selected'); - - // if there is an active item, use its id. if no active item or the dropdown is used as menu and is hidden, empty the active item - const activeId = $active.attr('id'); - if (menuVisible && activeId) { + // if the popup is visible and has an active/selected item, use its id as aria-activedescendant + if (menuVisible) { $focusable.attr('aria-activedescendant', $active.attr('id')); - } else if (!isComboBox && !menuVisible) { + } else if (!isComboBox) { + // for menu, when the popup is hidden, no need to keep the aria-activedescendant, and clear the active/selected item $focusable.removeAttr('aria-activedescendant'); $active.removeClass('active').removeClass('selected'); } @@ -116,7 +117,9 @@ function attachOneDropdownAria($dropdown) { // use setTimeout to run the refreshAria in next tick (to make sure the Fomantic UI code has finished its work) // do not return any value, jQuery has return-value related behaviors. - const deferredRefreshAria = () => { setTimeout(refreshAria, 0) }; + // when the popup is hiding, it's better to have a small "delay", because there is a Fomantic UI animation + // without the delay for hiding, the UI will be somewhat laggy and sometimes may get stuck in the animation. + const deferredRefreshAria = (delay = 0) => { setTimeout(refreshAria, delay) }; $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. @@ -142,14 +145,14 @@ function attachOneDropdownAria($dropdown) { $dropdown[0].addEventListener('blur', (e) => { debug(e.type); ignoreClickPreVisible = ignoreClickPreEvents = 0; - deferredRefreshAria(); + deferredRefreshAria(100); }, true); $dropdown[0].addEventListener('mouseup', (e) => { debug(e.type); setTimeout(() => { debug(`${e.type} (deferred)`); ignoreClickPreVisible = ignoreClickPreEvents = 0; - deferredRefreshAria(); + deferredRefreshAria(100); }, 0); }, true); $dropdown[0].addEventListener('click', (e) => { From 674d952bfb6fef430496dd82233937d0ed0ad17e Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 15 Mar 2023 12:36:46 +0800 Subject: [PATCH 11/13] remove unnecessary comment --- web_src/js/features/common-global.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/web_src/js/features/common-global.js b/web_src/js/features/common-global.js index dfde4c1b90b34..cdb9132805d39 100644 --- a/web_src/js/features/common-global.js +++ b/web_src/js/features/common-global.js @@ -91,8 +91,6 @@ export function initGlobalCommon() { const $uiDropdowns = $('.ui.dropdown'); // do not init "custom" dropdowns, "custom" dropdowns are managed by their own code. - // NOTICE: some old Android Chrome browsers has a crash bug, this line triggers the bug, - // the browser crashes when users click the Issue Reaction Menu button ("return" before this line, no crash. after, crash) $uiDropdowns.filter(':not(.custom)').dropdown({fullTextSearch: 'exact'}); // The "jump" means this dropdown is mainly used for "menu" purpose, From e42624a84f579d5744b66853de6325f5d163fb07 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 17 Mar 2023 09:43:43 +0800 Subject: [PATCH 12/13] remove unrelated code and comments, add more documents --- web_src/js/features/aria.js | 10 ---- web_src/js/features/aria.md | 102 ++++++++++++++++++++---------------- 2 files changed, 57 insertions(+), 55 deletions(-) diff --git a/web_src/js/features/aria.js b/web_src/js/features/aria.js index 8230ba0a85b3e..15fed5b87a386 100644 --- a/web_src/js/features/aria.js +++ b/web_src/js/features/aria.js @@ -127,41 +127,31 @@ function attachOneDropdownAria($dropdown) { // * 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($('
').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(100); }, true); $dropdown[0].addEventListener('mouseup', (e) => { - debug(e.type); setTimeout(() => { - debug(`${e.type} (deferred)`); ignoreClickPreVisible = ignoreClickPreEvents = 0; deferredRefreshAria(100); }, 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; diff --git a/web_src/js/features/aria.md b/web_src/js/features/aria.md index 09f134b262b85..679cec774cd2c 100644 --- a/web_src/js/features/aria.md +++ b/web_src/js/features/aria.md @@ -1,4 +1,27 @@ -**This document is used as aria/a11y reference for future developers** +# Background + +This document is used as aria/accessibility(a11y) reference for future developers. + +There are a lot of a11y problems in the Fomantic UI library. This `aria.js` is used +as a workaround to make the UI more accessible. + +The `aria.js` is designed to avoid touching the official Fomantic UI library, +and to be as independent as possible, so it can be easily modified/removed in the future. + +To test the aria/accessibility with screen readers, developers can use the following steps: + +* On macOS, you can use VoiceOver. + * Press `Command + F5` to turn on VoiceOver. + * Try to operate the UI with keyboard-only. + * Use Tab/Shift+Tab to switch focus between elements. + * Arrow keys to navigate between menu/combobox items (only aria-active, not really focused). + * Press Enter to trigger the aria-active element. +* On Android, you can use TalkBack. + * Go to Settings -> Accessibility -> TalkBack, turn it on. + * Long-press or press+swipe to switch the aria-active element (not really focused). + * Double-tap means old single-tap on the aria-active element. + * Double-finger swipe means old single-finger swipe. +* TODO: on Windows, on Linux, on iOS # Checkbox @@ -10,7 +33,8 @@ The ideal checkboxes should be: ``` -However, related styles aren't supported (not implemented) yet, so at the moment, almost all the checkboxes are still using Fomantic UI checkbox. +However, related CSS styles aren't supported (not implemented) yet, so at the moment, +almost all the checkboxes are still using Fomantic UI checkbox. ## Fomantic UI Checkbox @@ -21,64 +45,52 @@ However, related styles aren't supported (not implemented) yet, so at the moment
``` -Then the JS `$.checkbox()` should be called to make it work with keyboard and label-clicking, then it works like the ideal checkboxes. +Then the JS `$.checkbox()` should be called to make it work with keyboard and label-clicking, +then it works like the ideal checkboxes. -There is still a problem: Fomantic UI checkbox is not friendly to screen readers, so we add IDs to all the Fomantic UI checkboxes automatically by JS. +There is still a problem: Fomantic UI checkbox is not friendly to screen readers, +so we add IDs to all the Fomantic UI checkboxes automatically by JS. +If the `label` part is empty, then the checkbox needs to get the `aria-label` attribute manually. # Dropdown -## ARIA Dropdown - -There are different solutions: - -* combobox + listbox + option -* menu + menuitem - -```html -
- - -
-``` - - ## Fomantic UI Dropdown -There are 2 possible solutions about the role: combobox or menu +Fomantic Dropdown is designed to be used for many purposes: -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" +* Menu (the profile menu in navbar, the language menu in footer) +* Popup (the branch/tag panel, the review box) +* Simple `