alphagov / govuk-frontend

GOV.UK Frontend contains the code you need to start building a user interface for government platforms and services.
https://frontend.design-system.service.gov.uk/
MIT License
1.17k stars 320 forks source link

Remove ↑ up and ↓ down arrow key bindings from tabs #5158

Closed querkmachine closed 2 months ago

querkmachine commented 2 months ago

Currently the Tabs component binds the up and down arrow keys to the same functionality as the left and right arrow keys, using them to navigate between the previous and next tabs within a tablist.

Last week, we received a comment stating that these bindings were causing problems in NVDA, as the user became 'trapped' within the tablist and could not arrow out of it—up and down arrows being a frequent control mechanism for moving between adjacent elements.

This morning, we received another comment stating the opposite, and that the up/down bindings weren't necessarily problematic and that a user could 'break out' by switching to NVDA's document mode.

The example pattern in the WAI-ARIA Authoring Practices explicitly states that only the arrow keys matching the physical direction of the tablist should be bound to tab changes (left/right for a horizontal list, up/down for a vertical list).

On this basis, I think we should remove the existing key bindings for the up/down arrows.

Changes

Todo

There's an open-ended question about whether this constitutes a breaking change or not. Is it removing functionality or altering it? Or is it just fixing something that's unexpected? I've avoided adding a changelog just yet so we can ponder that.

Thoughts

I initially experimented with rebinding the up and down arrows to move focus between the tablist and the tab panel, as suggested in this issue, however this still felt like it had a high risk of being incongruous with how an assistive technology user expects their tools to behave and it isn't a behaviour suggested by WAI-ARIA, so I backtracked on doing that in this PR. You can see that work in a separate branch.

This does not resolve the other recent issue with the Tabs component in NVDA/Firefox. https://github.com/alphagov/govuk-frontend/issues/5154

github-actions[bot] commented 2 months ago

:clipboard: Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 112.52 KiB
dist/govuk-frontend-development.min.js 41.83 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 87.33 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 82.03 KiB
packages/govuk-frontend/dist/govuk/all.mjs 981 B
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 112.51 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 41.82 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.55 KiB
packages/govuk-frontend/dist/govuk/init.mjs 4.86 KiB

Modules

File Size (bundled) Size (minified)
all.mjs 79.16 KiB 39.79 KiB
accordion.mjs 23.5 KiB 12.39 KiB
button.mjs 5.98 KiB 2.69 KiB
character-count.mjs 22.4 KiB 9.92 KiB
checkboxes.mjs 5.83 KiB 2.83 KiB
error-summary.mjs 7.89 KiB 3.46 KiB
exit-this-page.mjs 17.1 KiB 9.26 KiB
header.mjs 4.46 KiB 2.6 KiB
notification-banner.mjs 6.26 KiB 2.62 KiB
password-input.mjs 15.15 KiB 7.25 KiB
radios.mjs 4.83 KiB 2.38 KiB
skip-link.mjs 4.39 KiB 2.18 KiB
tabs.mjs 10.05 KiB 6.06 KiB

View stats and visualisations on the review app


Action run for c84c2c5e9de8b69cbf37c1558cd1e139ea82fb40

github-actions[bot] commented 2 months ago

JavaScript changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
index 754775151..bc47ebb0c 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
@@ -1034,15 +1034,11 @@ class Tabs extends GOVUKFrontendComponent {
     onTabKeydown(e) {
         switch (e.key) {
             case "ArrowLeft":
-            case "ArrowUp":
             case "Left":
-            case "Up":
                 this.activatePreviousTab(), e.preventDefault();
                 break;
             case "ArrowRight":
-            case "ArrowDown":
             case "Right":
-            case "Down":
                 this.activateNextTab(), e.preventDefault()
         }
     }

Action run for c84c2c5e9de8b69cbf37c1558cd1e139ea82fb40

github-actions[bot] commented 2 months ago

Other changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/all.bundle.js b/packages/govuk-frontend/dist/govuk/all.bundle.js
index fbc644fbf..1bccdae71 100644
--- a/packages/govuk-frontend/dist/govuk/all.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/all.bundle.js
@@ -2250,16 +2250,12 @@
     onTabKeydown(event) {
       switch (event.key) {
         case 'ArrowLeft':
-        case 'ArrowUp':
         case 'Left':
-        case 'Up':
           this.activatePreviousTab();
           event.preventDefault();
           break;
         case 'ArrowRight':
-        case 'ArrowDown':
         case 'Right':
-        case 'Down':
           this.activateNextTab();
           event.preventDefault();
           break;
diff --git a/packages/govuk-frontend/dist/govuk/all.bundle.mjs b/packages/govuk-frontend/dist/govuk/all.bundle.mjs
index 3d03f5f0f..bec0c33e5 100644
--- a/packages/govuk-frontend/dist/govuk/all.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/all.bundle.mjs
@@ -2244,16 +2244,12 @@ class Tabs extends GOVUKFrontendComponent {
   onTabKeydown(event) {
     switch (event.key) {
       case 'ArrowLeft':
-      case 'ArrowUp':
       case 'Left':
-      case 'Up':
         this.activatePreviousTab();
         event.preventDefault();
         break;
       case 'ArrowRight':
-      case 'ArrowDown':
       case 'Right':
-      case 'Down':
         this.activateNextTab();
         event.preventDefault();
         break;
diff --git a/packages/govuk-frontend/dist/govuk/components/tabs/tabs.bundle.js b/packages/govuk-frontend/dist/govuk/components/tabs/tabs.bundle.js
index 63cce73bc..2ec07316d 100644
--- a/packages/govuk-frontend/dist/govuk/components/tabs/tabs.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/components/tabs/tabs.bundle.js
@@ -291,16 +291,12 @@
     onTabKeydown(event) {
       switch (event.key) {
         case 'ArrowLeft':
-        case 'ArrowUp':
         case 'Left':
-        case 'Up':
           this.activatePreviousTab();
           event.preventDefault();
           break;
         case 'ArrowRight':
-        case 'ArrowDown':
         case 'Right':
-        case 'Down':
           this.activateNextTab();
           event.preventDefault();
           break;
diff --git a/packages/govuk-frontend/dist/govuk/components/tabs/tabs.bundle.mjs b/packages/govuk-frontend/dist/govuk/components/tabs/tabs.bundle.mjs
index 01efd7654..96cfa7f4f 100644
--- a/packages/govuk-frontend/dist/govuk/components/tabs/tabs.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/components/tabs/tabs.bundle.mjs
@@ -285,16 +285,12 @@ class Tabs extends GOVUKFrontendComponent {
   onTabKeydown(event) {
     switch (event.key) {
       case 'ArrowLeft':
-      case 'ArrowUp':
       case 'Left':
-      case 'Up':
         this.activatePreviousTab();
         event.preventDefault();
         break;
       case 'ArrowRight':
-      case 'ArrowDown':
       case 'Right':
-      case 'Down':
         this.activateNextTab();
         event.preventDefault();
         break;
diff --git a/packages/govuk-frontend/dist/govuk/components/tabs/tabs.mjs b/packages/govuk-frontend/dist/govuk/components/tabs/tabs.mjs
index 39cd49970..82296adc4 100644
--- a/packages/govuk-frontend/dist/govuk/components/tabs/tabs.mjs
+++ b/packages/govuk-frontend/dist/govuk/components/tabs/tabs.mjs
@@ -198,16 +198,12 @@ class Tabs extends GOVUKFrontendComponent {
   onTabKeydown(event) {
     switch (event.key) {
       case 'ArrowLeft':
-      case 'ArrowUp':
       case 'Left':
-      case 'Up':
         this.activatePreviousTab();
         event.preventDefault();
         break;
       case 'ArrowRight':
-      case 'ArrowDown':
       case 'Right':
-      case 'Down':
         this.activateNextTab();
         event.preventDefault();
         break;

Action run for c84c2c5e9de8b69cbf37c1558cd1e139ea82fb40

selfthinker commented 2 months ago

I don't have any concerns from an accessibility perspective.

But I wonder if it could be a problem if users got used to using the up/down arrow keys and would now be missing the functionality. It might be difficult to find out if that is the case or not. And even if that turns out to be true, it might not be a reason to not remove them. There are certainly lots of tab components out there in other design systems and other websites that only support left/right, so users should generally be used to the that. What do others think?

romaricpascal commented 2 months ago

But I wonder if it could be a problem if users got used to using the up/down arrow keys and would now be missing the functionality. It might be difficult to find out if that is the case or not. And even if that turns out to be true, it might not be a reason to not remove them. There are certainly lots of tab components out there in other design systems and other websites that only support left/right, so users should generally be used to the that. What do others think?

Looking at the change, it's a fairly easy one to reverse should we discover we're in the wrong and release a patch.

The change looks good to merge. Interesting that there's no tests for these keys (looks like we're only testing for 'ArrowRight' in tabs.puppeteer.test.js. I've created an issue to test add tests for ArrowLeft at least (if there's none already) so we close that gap in a separate PR.

That leaves the question of statuating on whether it's a breaking change or not before we merge, which we can discuss on Slack or at dev catch-up.

romaricpascal commented 2 months ago

Digging a bit actually, looks like Bootstrap is also having some proposed changes by a user on their Tabs to remove the support for up/down arrows. This comment makes a good point about the need for both up/down and left/right when the tab list can be scrolled. However, our tabs do not scroll, but reflow when too many tabs are present, so I think it's safe for us to remove.

selfthinker commented 2 months ago

Bootstrap is also having some proposed changes by a user on their Tabs to remove the support for up/down arrows.

They also make a good point that the standard behaviour of up/down arrow keys to scroll the page breaks for non-SR keyboard users. Another reason to remove it.

querkmachine commented 2 months ago

I've added it as a fix entry in the changelog.