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

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

Merged
merged 2 commits into from
Jul 26, 2024

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Jul 19, 2024

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

  • Removes up and down arrow keydown bindings in the Tabs component.

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. #5154

@querkmachine querkmachine self-assigned this Jul 19, 2024
Copy link

github-actions bot commented Jul 19, 2024

📋 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 c84c2c5

Copy link

github-actions bot commented Jul 19, 2024

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 c84c2c5

Copy link

github-actions bot commented Jul 19, 2024

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 c84c2c5

@querkmachine querkmachine requested review from a team and selfthinker July 19, 2024 12:23
@querkmachine querkmachine marked this pull request as ready for review July 19, 2024 12:23
@selfthinker
Copy link

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
Copy link
Member

romaricpascal commented Jul 22, 2024

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
Copy link
Member

romaricpascal commented Jul 22, 2024

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
Copy link

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.

Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Romaric and Anika's takes in the convo of this PR. Reckon this is hot to go.

As for the type of change, I'd like to make a case that it's not a breaking change on the basis that we're not making any changes to the 'API' of govuk-frontend, even if it impacts functionality. The only way I can see in which this could be breaking for someone is if they have an automated test that checks for the up down arrow interaction. That's possible but IMO not likely and not critical. Generally speaking, 'functionality' is on the other side of the line we've drawn on what is and isn't breaking and I don't think this is a reason to change our minds on that.

Not expecting us to come to a conclusion on this now. Just airing my thoughts on this.

@querkmachine querkmachine force-pushed the tabs-remove-up-down-keybinding branch from eb9366c to c84c2c5 Compare July 26, 2024 11:07
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-5158 July 26, 2024 11:07 Inactive
@querkmachine
Copy link
Member Author

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

Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all good with the fix entry, thanks ⛵

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants