Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

a11y: Improve accessibility of dropdown menus #8638

Merged
merged 16 commits into from Nov 8, 2019

Conversation

@Jookia
Copy link
Contributor

Jookia commented Oct 22, 2019

This patch set fixes keyboard and screen reader accessibility for dropdown menus and lays the groundwork for improvements to other dropdown-related components such as list boxes and search menus.

Note that this doesn't change any behaviour of the dropdown menus themselves outside focusing, but the dropdown menus do seem a little strange compared to dropdown menus on other sites as they automatically open and retain their selected item between focuses.

To test this patch, just verify that:

  • Dropdowns still work
  • Only dropdown menus have changed HTML
  • Dropdown icons next to dropdown menus are no longer focusable
  • Screen readers can hear dropdown menus
@guillep2k

This comment has been minimized.

Copy link
Member

guillep2k commented Oct 22, 2019

Please address the lint errors from the build: https://drone.gitea.io/go-gitea/gitea/14488/1/7

You do the checks locally using:

make js
@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 23, 2019

I'll fix the lint errors that are mine, but a lot are from Semantic's dropdown.js. What should be done about that?

@guillep2k

This comment has been minimized.

Copy link
Member

guillep2k commented Oct 23, 2019

I'll fix the lint errors that are mine, but a lot are from Semantic's dropdown.js. What should be done about that?

That's a good question. We normally use the /vendor directory to place external libraries, which are exempt from linting. But only when they are untouched (e.g. right from npm). Is that the case here, or you had to edit it to make it work? (I can imagine the answer).

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 23, 2019

The semantic.dropdown.js is taken from Semantic UI then edited to add accessibility features, so it's definitely not from upstream- in commit #2 of this patch series I explicitly added a note at the top of the file that it's been modified for Gitea.

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 23, 2019

I've found a bug in this: aria-expanded and aria-activedescendant are set regardless of the current role, so I'll have to fix that too.

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 23, 2019

I also realized I use double quotes instead of single quotes in a lot of places, and more style fixes.

@guillep2k

This comment has been minimized.

Copy link
Member

guillep2k commented Oct 23, 2019

@Jookia No problem. You can edit the title and prefix it with WIP: or [WIP] (with a space), but not [WIP]: for some reason 😄.

@Jookia Jookia changed the title a11y: Improve accessibility of dropdown menus WIP: a11y: Improve accessibility of dropdown menus Oct 23, 2019
@Jookia Jookia force-pushed the Jookia:a11y-dropdownmenus branch from bbcbfc4 to af7ca7a Oct 23, 2019
@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 23, 2019

Okay, I think all my issues are fixed. Linting shows no errors from my lines of code either, aside from one issue that matches the rest of the Sementic code base.

@Jookia Jookia changed the title WIP: a11y: Improve accessibility of dropdown menus a11y: Improve accessibility of dropdown menus Oct 23, 2019
@Jookia Jookia force-pushed the Jookia:a11y-dropdownmenus branch from a6e6459 to b78fc01 Oct 23, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 23, 2019

Codecov Report

Merging #8638 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8638      +/-   ##
==========================================
- Coverage   41.23%   41.22%   -0.01%     
==========================================
  Files         544      544              
  Lines       70002    70002              
==========================================
- Hits        28865    28859       -6     
- Misses      37445    37450       +5     
- Partials     3692     3693       +1
Impacted Files Coverage Δ
modules/indexer/indexer.go 44.73% <0%> (-10.53%) ⬇️
models/unit.go 62.16% <0%> (-5.41%) ⬇️
modules/migrations/migrate.go 21.22% <0%> (-1.68%) ⬇️
modules/migrations/gitea.go 10.16% <0%> (-1.49%) ⬇️
models/gpg_key.go 55.03% <0%> (-0.56%) ⬇️
models/repo.go 48.81% <0%> (-0.06%) ⬇️
models/repo_list.go 74.27% <0%> (+0.97%) ⬆️
models/error.go 32.98% <0%> (+1.23%) ⬆️
modules/task/migrate.go 28.2% <0%> (+3.84%) ⬆️
modules/process/manager.go 82.43% <0%> (+4.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9be82b...a9d856e. Read the comment docs.

Copy link
Member

guillep2k left a comment

I guess all drop-down boxes will be affected by this. What Gitea UI operations would you suggest we can test to see the differences in behavior?

templates/base/footer.tmpl Outdated Show resolved Hide resolved
templates/pwa/serviceworker_js.tmpl Outdated Show resolved Hide resolved
@@ -11,18 +11,16 @@
<div class="ui header">
{{.i18n.Tr "home.switch_dashboard_context"}}
</div>
<div class="scrolling menu items">

This comment has been minimized.

Copy link
@guillep2k

guillep2k Oct 23, 2019

Member

Why scrolling has been removed from this? What happens if it's large enough to fill the screen? Does semantic.dropdown.js take care of it?

image

This comment has been minimized.

Copy link
@Jookia

Jookia Oct 23, 2019

Author Contributor

I didn't remove just the scrolling, I removed the entire sub container. How would I test to see if it's large enough to fill the screen?

This comment has been minimized.

Copy link
@zeripath

zeripath Oct 23, 2019

Contributor

Just create more organizations

*
*/

/* This version has been modified for Gitea */

This comment has been minimized.

Copy link
@guillep2k

guillep2k Oct 23, 2019

Member
Suggested change
/* This version has been modified for Gitea */
/*
* Copyright 2019 The Gitea Authors. All rights reserved.
* This version has been modified by Gitea to improve accessibility.
*/

I'm not sure why the first comment starts with /*! instead of /*, though.

This comment has been minimized.

Copy link
@Jookia

Jookia Oct 23, 2019

Author Contributor

Can I leave out the 'All rights reserved.'? Seems a bit weird given it's under the MIT license

This comment has been minimized.

Copy link
@zeripath

zeripath Oct 23, 2019

Contributor

I think you have to still put it as it's standard boilerplate. I'd probably put the comment before the copyright so as to make it clear that only the modifications are Gitea's but it's ok

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Oct 23, 2019

Finally had time to run this.

There's an issue with using the keyboard on any of the dropdowns in that the visible label of dropdown changes including losing the down arrow. You probably need to set the aria-label instead.

@zeripath zeripath added the kind/ui label Oct 23, 2019
@zeripath zeripath added this to the 1.11.0 milestone Oct 23, 2019
@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 23, 2019

@guillep2k This should only affect dropdown menus, meaning dropdown items that you can't search, select multiple values, or store a value for a form. So in practice, just the menus that you click on that perform an action instead of setting a value, like navigation or setting repository collaborator. I would suggest testing that you can tab past these menus properly, and hear them on a screen reader (if you have one installed.)

@zeripath I do set the aria-label appropriately. The problem you're noticing is present in the existing codebase and not introduced by these commits. It's strange behavior, but that's how Semantic works.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Oct 23, 2019

@Jookia Ugh I think that needs fixing too! If it's too hard to get sorted then maybe we can fix it later(?)

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 23, 2019

I don't think it's a fix for this PR, since it requires a lot more research of the code base.

To elaborate: Semantic doesn't understand what an actual 'menu' is, it just knows about dropdowns and some mixins like having it be a form or have the items search, etc. So when used for navigation menus, it changes the value on the actual item just as you would when selecting an element in a list box.

Sometimes it does this when you click on some dropdown menus too, such as the languages at the bottom of the screen. It doesn't do it other times when you click on things such as sort buttons or the navigation buttons. The logic behind this is something I would have to dig in to.

We could just say 'dropdowns that don't set a value (aka menus) shouldn't remember their currently selected item', which would reflect them being used for menus better- after all, if they don't set a value they're likely going to contain links or actions instead.

But sometimes you want them to remember their value when you use the menus for things like repository collaboration permissions which is a plain 'menu' but its actions don't take you away from the page so you want its selection to stay.

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 23, 2019

To be clear: I really don't want to change any user visible behavior in this PR, just make it accessible. I feel like changes to behavior would need long discussion about what the right behavior is in an issue, and I don't want that to impede this PR's goal of just having the menus accessible to keyboards and screen readers. If you want I can do up an issue explaining what UI problems dropdown menus have an open the discussion there.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Oct 23, 2019

No worries - I understand what you're saying. The menus changing their title when you tap through them is a problem for accessibility (but not all of our dropdowns act as menus.) However I think there's enough in the pr already - it doesn't need to fix everything!

I was just worried that was a regression but as it's not that's great.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Oct 23, 2019

I agree with @guilep2k comments. I'll have another play to see if I can break anything but if you address those and I can't break anything then I would approve

Jookia added 5 commits Oct 23, 2019
Setting tabindex=-1 on focusable elements within dropdown menus allows
the user to treat dropdown menus as a single focusable item with its own
internal navigation using arrow keys.
Menu items are often <a> elements, which jQuery refuses to trigger click
events on. Instead it just bubbles up to the menu.

Using HTMLElement's click method fixes this and makes menu items
clickable from the keyboard using dropdown menus.
Setting role= makes assistive technology aware there is a widget here.
In this case, Orca will now exit browse mode and allow us to capture
keydown events when focused on a dropdown menu. It will also inform the
user that there's a menu focused.

Since dropdowns can be used in multiple elements each with different
ARIA roles, a guessRole method is used to find the correct role.
All roles I consider possible are listed, but only menu is implemented.
This is deliberately done before the transition finishes so that screen
readers get immediate feedback.
This makes dropdown menu buttons screen reader accessible.

aria-labelledby refers to an element using an ID, so the chosen labels
are now assigned a unique ID- This ID is not stable, do not refer to it
with user scripts.
@Jookia Jookia requested review from silverwind and guillep2k Oct 30, 2019
@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Oct 30, 2019

@silverwind Yeah it's a pretty poor situation. It would have likely been less annoying if we'd brought in semantic and it's less and JavaScript - and then Minified everything together as we could keep our own patches to semantic.

It's possible that switching over to fomantic is not actually that difficult - just difficult for me when I'm trying to do other things. If you have any free cycles it's probably worth a go - then at least we'd have a working upstream.

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 30, 2019

I'm happy to try and push these changes to Fomantic if Gitea switches to it.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Oct 30, 2019

@Jookia I'm sure they would appreciate them even if we don't switch.

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 30, 2019

I'm sure they would too, but I don't have a way to test patches for Fomantic without a project that uses Fomantic

@silverwind

This comment has been minimized.

Copy link
Member

silverwind commented Oct 31, 2019

What is submodule /public/vendor/plugins/semantic/Semantic-UI for? We never submoduled JS dependencies and it does not seem to be referenced anywhere. Can it be removed?

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Oct 31, 2019

Good catch, that was a mistake as I had it checked out there on my dev machine. Removed.

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Nov 3, 2019

So this is short another approval. I asked @guillep2k but frontend isn't his speciality, so it looks like it might be @silverwind or some other maintainer that will have to do it.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Nov 3, 2019

To be clearer to help people discover the differences between semantic 2.3.1's dropdown and @Jookia s changes. Here's a unified diff:

--- 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);
               }
@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Nov 8, 2019

Doesn't look like this is going to get reviewed, so I'm closing this PR and keeping these changes local.

@Jookia Jookia closed this Nov 8, 2019
@techknowlogick techknowlogick reopened this Nov 8, 2019
@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Nov 8, 2019

re-opening as I'm reviewing it right now (I've been travelling for the past while so I haven't been able to get to bigger reviews)

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Nov 8, 2019
@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Nov 8, 2019

Oh, thanks!

@techknowlogick techknowlogick merged commit 1274ad8 into go-gitea:master Nov 8, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/drone/pr Build is running
Details
approvals/lgtm this commit looks good
@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Nov 8, 2019

@Jookia thank you so much for your PR, we really appreciate it.

@Jookia

This comment has been minimized.

Copy link
Contributor Author

Jookia commented Nov 8, 2019

I'm glad to hear that. Thanks everyone here. Hopefully I'll find time to do some more accessibility PRs. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.