-
Notifications
You must be signed in to change notification settings - Fork 327
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
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for c84c2c5 |
JavaScript changes to npm packagediff --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 |
Other changes to npm packagediff --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 |
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. |
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. |
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. |
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. |
There was a problem hiding this 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.
eb9366c
to
c84c2c5
Compare
I've added it as a fix entry in the changelog. |
There was a problem hiding this 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 ⛵
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
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