Reintegrate the dropdown patch using fomantic hooks and template changes #16581
Comments
|
First of all I'm going to provide the original patch in its entirety: Original Patch--- semanti.dropdown.js.original 2018-03-19 04:04:27.000000000 +0000
+++ semantic.dropdown.js 2019-11-03 20:24:41.929563833 +0000
@@ ---- semanti.dropdown.js.original 2018-03-19 04:04:27.000000000 +0000
+++ semantic.dropdown.js 2019-11-03 20:24:41.929563833 +0000
@@ -8,6 +8,13 @@
*
*/
+/*
+ * Copyright 2019 The Gitea Authors
+ * Released under the MIT license
+ * http://opensource.org/licenses/MIT
+ * This version has been modified by Gitea to improve accessibility.
+ */
+
;(function ($, window, document, undefined) {
'use strict';
@@ -33,6 +40,7 @@
query = arguments[0],
methodInvoked = (typeof query == 'string'),
queryArguments = [].slice.call(arguments, 1),
+ lastAriaID = 1,
returnedValue
;
@@ -114,6 +122,8 @@
module.observeChanges();
module.instantiate();
+
+ module.aria.setup();
}
},
@@ -296,6 +306,86 @@
}
},
+ aria: {
+ setup: function() {
+ var role = module.aria.guessRole();
+ if( role !== 'menu' ) {
+ return;
+ }
+ $module.attr('aria-busy', 'true');
+ $module.attr('role', 'menu');
+ $module.attr('aria-haspopup', 'menu');
+ $module.attr('aria-expanded', 'false');
+ $menu.find('.divider').attr('role', 'separator');
+ $item.attr('role', 'menuitem');
+ $item.each(function (index, item) {
+ if( !item.id ) {
+ item.id = module.aria.nextID('menuitem');
+ }
+ });
+ $text = $module
+ .find('> .text')
+ .eq(0)
+ ;
+ if( $module.data('content') ) {
+ $text.attr('aria-hidden');
+ $module.attr('aria-label', $module.data('content'));
+ }
+ else {
+ $text.attr('id', module.aria.nextID('menutext'));
+ $module.attr('aria-labelledby', $text.attr('id'));
+ }
+ $module.attr('aria-busy', 'false');
+ },
+ nextID: function(prefix) {
+ var nextID;
+ do {
+ nextID = prefix + '_' + lastAriaID++;
+ } while( document.getElementById(nextID) );
+ return nextID;
+ },
+ setExpanded: function(expanded) {
+ if( $module.attr('aria-haspopup') ) {
+ $module.attr('aria-expanded', expanded);
+ }
+ },
+ refreshDescendant: function() {
+ if( $module.attr('aria-haspopup') !== 'menu' ) {
+ return;
+ }
+ var
+ $currentlySelected = $item.not(selector.unselectable).filter('.' + className.selected).eq(0),
+ $activeItem = $menu.children('.' + className.active).eq(0),
+ $selectedItem = ($currentlySelected.length > 0)
+ ? $currentlySelected
+ : $activeItem
+ ;
+ if( $selectedItem ) {
+ $module.attr('aria-activedescendant', $selectedItem.attr('id'));
+ }
+ else {
+ module.aria.removeDescendant();
+ }
+ },
+ removeDescendant: function() {
+ if( $module.attr('aria-haspopup') == 'menu' ) {
+ $module.removeAttr('aria-activedescendant');
+ }
+ },
+ guessRole: function() {
+ var
+ isIcon = $module.hasClass('icon'),
+ hasSearch = module.has.search(),
+ hasInput = ($input.length > 0),
+ isMultiple = module.is.multiple()
+ ;
+ if ( !isIcon && !hasSearch && !hasInput && !isMultiple ) {
+ return 'menu';
+ }
+ return 'unknown';
+ }
+ },
+
setup: {
api: function() {
var
@@ -335,6 +425,7 @@
if(settings.allowTab) {
module.set.tabbable();
}
+ $item.attr('tabindex', '-1');
},
select: function() {
var
@@ -477,6 +568,8 @@
return true;
}
if(settings.onShow.call(element) !== false) {
+ module.aria.setExpanded(true);
+ module.aria.refreshDescendant();
module.animate.show(function() {
if( module.can.click() ) {
module.bind.intent();
@@ -499,6 +592,8 @@
if( module.is.active() && !module.is.animatingOutward() ) {
module.debug('Hiding dropdown');
if(settings.onHide.call(element) !== false) {
+ module.aria.setExpanded(false);
+ module.aria.removeDescendant();
module.animate.hide(function() {
module.remove.visible();
callback.call(element);
@@ -902,7 +997,7 @@
;
if(hasSelected && !module.is.multiple()) {
module.debug('Forcing partial selection to selected item', $selectedItem);
- module.event.item.click.call($selectedItem, {}, true);
+ $selectedItem[0].click();
return;
}
else {
@@ -1363,7 +1458,7 @@
// allow selection with menu closed
if(isAdditionWithoutMenu) {
module.verbose('Selecting item from keyboard shortcut', $selectedItem);
- module.event.item.click.call($selectedItem, event);
+ $selectedItem[0].click();
if(module.is.searchSelection()) {
module.remove.searchTerm();
}
@@ -1380,7 +1475,7 @@
}
else if(selectedIsSelectable) {
module.verbose('Selecting item from keyboard shortcut', $selectedItem);
- module.event.item.click.call($selectedItem, event);
+ $selectedItem[0].click();
if(module.is.searchSelection()) {
module.remove.searchTerm();
}
@@ -1405,6 +1500,7 @@
.closest(selector.item)
.addClass(className.selected)
;
+ module.aria.refreshDescendant();
event.preventDefault();
}
}
@@ -1421,6 +1517,7 @@
.find(selector.item).eq(0)
.addClass(className.selected)
;
+ module.aria.refreshDescendant();
event.preventDefault();
}
}
@@ -1445,6 +1542,7 @@
$nextItem
.addClass(className.selected)
;
+ module.aria.refreshDescendant();
module.set.scrollPosition($nextItem);
if(settings.selectOnKeydown && module.is.single()) {
module.set.selectedItem($nextItem);
@@ -1472,6 +1570,7 @@
$nextItem
.addClass(className.selected)
;
+ module.aria.refreshDescendant();
module.set.scrollPosition($nextItem);
if(settings.selectOnKeydown && module.is.single()) {
module.set.selectedItem($nextItem);
@@ -2399,6 +2498,7 @@
module.set.scrollPosition($nextValue);
$selectedItem.removeClass(className.selected);
$nextValue.addClass(className.selected);
+ module.aria.refreshDescendant();
if(settings.selectOnKeydown && module.is.single()) {
module.set.selectedItem($nextValue);
}
8,6 +8,13 @@
*
*/
+/*
+ * Copyright 2019 The Gitea Authors
+ * Released under the MIT license
+ * http://opensource.org/licenses/MIT
+ * This version has been modified by Gitea to improve accessibility.
+ */
+
;(function ($, window, document, undefined) {
'use strict';
@@ -33,6 +40,7 @@
query = arguments[0],
methodInvoked = (typeof query == 'string'),
queryArguments = [].slice.call(arguments, 1),
+ lastAriaID = 1,
returnedValue
;
@@ -114,6 +122,8 @@
module.observeChanges();
module.instantiate();
+
+ module.aria.setup();
}
},
@@ -296,6 +306,86 @@
}
},
+ aria: {
+ setup: function() {
+ var role = module.aria.guessRole();
+ if( role !== 'menu' ) {
+ return;
+ }
+ $module.attr('aria-busy', 'true');
+ $module.attr('role', 'menu');
+ $module.attr('aria-haspopup', 'menu');
+ $module.attr('aria-expanded', 'false');
+ $menu.find('.divider').attr('role', 'separator');
+ $item.attr('role', 'menuitem');
+ $item.each(function (index, item) {
+ if( !item.id ) {
+ item.id = module.aria.nextID('menuitem');
+ }
+ });
+ $text = $module
+ .find('> .text')
+ .eq(0)
+ ;
+ if( $module.data('content') ) {
+ $text.attr('aria-hidden');
+ $module.attr('aria-label', $module.data('content'));
+ }
+ else {
+ $text.attr('id', module.aria.nextID('menutext'));
+ $module.attr('aria-labelledby', $text.attr('id'));
+ }
+ $module.attr('aria-busy', 'false');
+ },
+ nextID: function(prefix) {
+ var nextID;
+ do {
+ nextID = prefix + '_' + lastAriaID++;
+ } while( document.getElementById(nextID) );
+ return nextID;
+ },
+ setExpanded: function(expanded) {
+ if( $module.attr('aria-haspopup') ) {
+ $module.attr('aria-expanded', expanded);
+ }
+ },
+ refreshDescendant: function() {
+ if( $module.attr('aria-haspopup') !== 'menu' ) {
+ return;
+ }
+ var
+ $currentlySelected = $item.not(selector.unselectable).filter('.' + className.selected).eq(0),
+ $activeItem = $menu.children('.' + className.active).eq(0),
+ $selectedItem = ($currentlySelected.length > 0)
+ ? $currentlySelected
+ : $activeItem
+ ;
+ if( $selectedItem ) {
+ $module.attr('aria-activedescendant', $selectedItem.attr('id'));
+ }
+ else {
+ module.aria.removeDescendant();
+ }
+ },
+ removeDescendant: function() {
+ if( $module.attr('aria-haspopup') == 'menu' ) {
+ $module.removeAttr('aria-activedescendant');
+ }
+ },
+ guessRole: function() {
+ var
+ isIcon = $module.hasClass('icon'),
+ hasSearch = module.has.search(),
+ hasInput = ($input.length > 0),
+ isMultiple = module.is.multiple()
+ ;
+ if ( !isIcon && !hasSearch && !hasInput && !isMultiple ) {
+ return 'menu';
+ }
+ return 'unknown';
+ }
+ },
+
setup: {
api: function() {
var
@@ -335,6 +425,7 @@
if(settings.allowTab) {
module.set.tabbable();
}
+ $item.attr('tabindex', '-1');
},
select: function() {
var
@@ -477,6 +568,8 @@
return true;
}
if(settings.onShow.call(element) !== false) {
+ module.aria.setExpanded(true);
+ module.aria.refreshDescendant();
module.animate.show(function() {
if( module.can.click() ) {
module.bind.intent();
@@ -499,6 +592,8 @@
if( module.is.active() && !module.is.animatingOutward() ) {
module.debug('Hiding dropdown');
if(settings.onHide.call(element) !== false) {
+ module.aria.setExpanded(false);
+ module.aria.removeDescendant();
module.animate.hide(function() {
module.remove.visible();
callback.call(element);
@@ -902,7 +997,7 @@
;
if(hasSelected && !module.is.multiple()) {
module.debug('Forcing partial selection to selected item', $selectedItem);
- module.event.item.click.call($selectedItem, {}, true);
+ $selectedItem[0].click();
return;
}
else {
@@ -1363,7 +1458,7 @@
// allow selection with menu closed
if(isAdditionWithoutMenu) {
module.verbose('Selecting item from keyboard shortcut', $selectedItem);
- module.event.item.click.call($selectedItem, event);
+ $selectedItem[0].click();
if(module.is.searchSelection()) {
module.remove.searchTerm();
}
@@ -1380,7 +1475,7 @@
}
else if(selectedIsSelectable) {
module.verbose('Selecting item from keyboard shortcut', $selectedItem);
- module.event.item.click.call($selectedItem, event);
+ $selectedItem[0].click();
if(module.is.searchSelection()) {
module.remove.searchTerm();
}
@@ -1405,6 +1500,7 @@
.closest(selector.item)
.addClass(className.selected)
;
+ module.aria.refreshDescendant();
event.preventDefault();
}
}
@@ -1421,6 +1517,7 @@
.find(selector.item).eq(0)
.addClass(className.selected)
;
+ module.aria.refreshDescendant();
event.preventDefault();
}
}
@@ -1445,6 +1542,7 @@
$nextItem
.addClass(className.selected)
;
+ module.aria.refreshDescendant();
module.set.scrollPosition($nextItem);
if(settings.selectOnKeydown && module.is.single()) {
module.set.selectedItem($nextItem);
@@ -1472,6 +1570,7 @@
$nextItem
.addClass(className.selected)
;
+ module.aria.refreshDescendant();
module.set.scrollPosition($nextItem);
if(settings.selectOnKeydown && module.is.single()) {
module.set.selectedItem($nextItem);
@@ -2399,6 +2498,7 @@
module.set.scrollPosition($nextValue);
$selectedItem.removeClass(className.selected);
$nextValue.addClass(className.selected);
+ module.aria.refreshDescendant();
if(settings.selectOnKeydown && module.is.single()) {
module.set.selectedItem($nextValue);
} |
|
The patch in #16576 differs in that these sections of the patch have been deleted: @@ -902,7 +997,7 @@
;
if(hasSelected && !module.is.multiple()) {
module.debug('Forcing partial selection to selected item', $selectedItem);
- module.event.item.click.call($selectedItem, {}, true);
+ $selectedItem[0].click();
return;
}
else {
@@ -1363,7 +1458,7 @@
// allow selection with menu closed
if(isAdditionWithoutMenu) {
module.verbose('Selecting item from keyboard shortcut', $selectedItem);
- module.event.item.click.call($selectedItem, event);
+ $selectedItem[0].click();
if(module.is.searchSelection()) {
module.remove.searchTerm();
}
@@ -1363,7 +1458,7 @@
// allow selection with menu closed
if(isAdditionWithoutMenu) {
module.verbose('Selecting item from keyboard shortcut', $selectedItem);
- module.event.item.click.call($selectedItem, event);
+ $selectedItem[0].click();
if(module.is.searchSelection()) {
module.remove.searchTerm();
}These have been removed because with the updated JQuery these prevent the dropdown from ever losing focus. Now, the second and third of these don't appear to be different from So I guess the question is what was the aim of this? |
|
Set items tabindex -1@@ -335,6 +425,7 @@
if(settings.allowTab) {
module.set.tabbable();
}
+ $item.attr('tabindex', '-1');
},
select: function() {
varThis sets the tabindex to -1 on all of the items - to match roving tabindex and to make the dropdown feel a lot more a select I guess - but I'm not sure if this is really all that necessary as tabbing works fine without it at present. Is this really necessary? It could actually be set in onShow though. |
setExpanded and refreshDescendant...
+ setExpanded: function(expanded) {
+ if( $module.attr('aria-haspopup') ) {
+ $module.attr('aria-expanded', expanded);
+ }
+ },
+ refreshDescendant: function() {
+ if( $module.attr('aria-haspopup') !== 'menu' ) {
+ return;
+ }
+ var
+ $currentlySelected = $item.not(selector.unselectable).filter('.' + className.selected).eq(0),
+ $activeItem = $menu.children('.' + className.active).eq(0),
+ $selectedItem = ($currentlySelected.length > 0)
+ ? $currentlySelected
+ : $activeItem
+ ;
+ if( $selectedItem ) {
+ $module.attr('aria-activedescendant', $selectedItem.attr('id'));
+ }
+ else {
+ module.aria.removeDescendant();
+ }
+ },
+ removeDescendant: function() {
+ if( $module.attr('aria-haspopup') == 'menu' ) {
+ $module.removeAttr('aria-activedescendant');
+ }
+ },
...
@@ -477,6 +568,8 @@
return true;
}
if(settings.onShow.call(element) !== false) {
+ module.aria.setExpanded(true);
+ module.aria.refreshDescendant();
module.animate.show(function() {
if( module.can.click() ) {
module.bind.intent();
@@ -499,6 +592,8 @@
if( module.is.active() && !module.is.animatingOutward() ) {
module.debug('Hiding dropdown');
if(settings.onHide.call(element) !== false) {
+ module.aria.setExpanded(false);
+ module.aria.removeDescendant();
module.animate.hide(function() {
module.remove.visible();First of all both of these functions appear to be being called straight after Essentially these functions appear to keep track of the
|
|
So the best way to look at this patch is to look through my mindset of what I could do to immediately make things a little better without putting load on maintainers to actually fix things, since it didn't seem like maintainers wanted to spend time refactoring HTML or learning ARIA in order to fix things properly. I was also learning ARIA at the time specifically to make this patch, and getting pretty burned out in the process. In the end I was looking at the specific solution of getting some menus working so my blind friend could use Gitea better. For what it's worth, I had a lot of explanations in git commit messages but these got squashed and removed I think. I was grumpy about this for this exact reason. Gitea uses dropdowns everywhere. Not just for menus, but also for search boxes, tag selection, branch selection, everywhere. These widgets range from 'listbox' to 'combobox' to other fancy widgets that do multiple things such as manage entries in a list and letting you add new entries by searching in the entry list and adding new tags. Sometimes there's even listboxes that have buttons to open the listbox with an arrow, and those are marked as icons. All of these use Fomantic's 'dropdown' UI. So the first thing to really do is try and find some actual listboxes (which I incorrectly refer to as menu): The criteria here is that:
That seems like it's MAYBE a dropdown. The correct solution here would be to go through every dropdown in Gitea's source code and categorize what it is and how to actually handle it, because for each dropdown Fomantic also adds some kind of Javascript, even on the little dropdown icons people are supposed to click. The next step is to decide what ARIA widget this is and start implementing its role, as a contract between us and any assistive technology. I chose 'menubar' because I wasn't sure what to pick, but in retrospect it should've been 'listbox'. In any case, I did this wrong since I was trying to fit a square in to a round hole.
The next thing to think about is focus management. This is what most dropdowns on the Internet get wrong. When you use a keyboard to access ARIA widgets, you don't want to use tab, you want to just arrows and specific keys mandated by ARIA. This means for example that you can tab past listboxes without tabbing through each element in the listbox. If a person wants to use the dropdown, they'll open it and use the arrows. It's also important to note that assistive technology have a thing called 'browse mode'- this is where the browser allows assistive technology to capture all key presses on a website. This is so a user can navigate a page using the keyboard arrows to step through each line of text, or jump to headings. This is much faster than tabbing through every element on a page, but it's also something that only really comes in to play for screen readers as far as I can tell. So people unable to use a mouse still have to tab through everything, but people with screen readers can skip it. Quoting https://www.w3.org/TR/wai-aria-1.2/#application :
The lack of focus of individual elements means the assistive technology can no longer track and read out what items are selected, which is why we need to hint it:
I think widgets should manage their own focus since it provides a less confusing experiences- you can't accidentally focus subelements using tab when you're not supposed to and get in to an invalid and confusing state. With that in mind, we set the tabindex to -1 so NOTHING inside the dropdown is focusable: We also should be setting the icon next to the menu to tabindex -1 so we don't have to tab past it: But that might not have been upstreamed or something. We set aria-expanded when that changes: Then whenever anything changes, we update the aria-activedescent attribute to point to the selected item: This does not work for listboxes that let you select multiple things, only one or no elements. Calls to these hooks are peppers basically whenever input happens or the dropdown is shown. This is because I didn't wanted to dig in the dropdown spaghetti and wanted to instead make sure it worked each time. Now, when it's time to select an element, the dropdown code calls I had to revert this in one case later: |
Remaining
|
|
Thanks @Jookia I can imagine how burning out it felt - there's a reason why I never got round to fixing this myself. The documentation is highly confusing and its not clear how to map things on to Gitea's UI (or most UI TBH.)
I wouldn't assume that anyone doesn't want to learn ARIA - we just have other things to do and if someone appears to know things better most of us are happy to defer. Your experience of getting burnt out by learning ARIA is precisely my feeling of when I tried looking into it - especially as I could not get orca to work at all. Honestly - as I said I'd be very happy to copy patterns that work but finding any consistency in the documentation I last found when I looked in to this stuff was impossible.
Ah this is the issue with squash merging. Honestly if you ever feel like giving us another PR - and I hope you can - code comments and comments in issues are a better way of describing things.
Yup - it's a very versatile element. Especially with the searching.
Agreed - this is the correct solution. Whilst not a solution that can be done for 1.15 - this can and should be done for 1.16 and would definitely receive positive reviews.
This explains my slight confusion with the
My impression of fomantic-ui was that this was the one thing that it does right - but it doesn't set the active-descendent - which could be solved using a selection hook.
OK, I guess this really only applies if we have
I guess also a template level thing - but should probably not apply to searching dropdowns.
Yeah I'm not certain what happened there - may have been conflicted away.
This would require aria-multipleselected, aria-selected and focusing for the elements in the drop down IIRC?
but I don't understand why you want the onclick call exactly?
I think this is the one that I've noticed here .and had to add a fix in against. |
|
I didn't know about multipleselected/aria-selected, etc. The onClick call is because some Gitea code uses onClick elements and don't account for keyboard focus. |
OK I think we should change those to use the select callback. |
|
On Sat, Jul 31, 2021 at 02:15:13AM -0700, zeripath wrote:
OK I think we should change those to use the select callback.
Well, yeah. But who's going to spelunk the code to find them all?
|
Description
The dropdown patch from #8638 is somewhat of a blackbox with little shared understanding as to how it works and why. However, looking again at it I think there are ways that some of its work could be done using hooks already present in fomantic and with template changes.
I would like to use this issue to discuss parts of the patch and the reasoning for these
I hope @Jookia would be able to help explain this patch.
The text was updated successfully, but these errors were encountered: