From e7291ffd5110bd6324ac52e19a87f21728ab5b9e Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 19 Jan 2021 14:09:51 +0000 Subject: [PATCH 1/3] Add expanded semantics to radioSelect buttons All buttons that open or close a region of the component should have aria-expanded attributes to show: - they have that control - the state of the region --- app/assets/javascripts/radioSelect.js | 6 +++--- tests/javascripts/radioSelect.test.js | 9 +++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/radioSelect.js b/app/assets/javascripts/radioSelect.js index fbd4175b9..53c8d7f37 100644 --- a/app/assets/javascripts/radioSelect.js +++ b/app/assets/javascripts/radioSelect.js @@ -17,7 +17,7 @@ {{/showNowAsDefault}}
{{#categories}} - + {{/categories}}
`), @@ -37,7 +37,7 @@ {{/choices}} - + `), 'chosen': Hogan.compile(` @@ -58,7 +58,7 @@ {{/choices}}
- +
`) }; diff --git a/tests/javascripts/radioSelect.test.js b/tests/javascripts/radioSelect.test.js index 4ab998df1..ab5420e07 100644 --- a/tests/javascripts/radioSelect.test.js +++ b/tests/javascripts/radioSelect.test.js @@ -179,6 +179,14 @@ describe('RadioSelect', () => { }); + test("each button's semantics should show it controls another region and that region is collapsed", () => { + + categoryButtons.forEach(btn => { + expect(btn.getAttribute('aria-expanded')).toEqual('false'); + }); + + }); + }); }); @@ -241,6 +249,7 @@ describe('RadioSelect', () => { expect(button).not.toBeNull(); expect(button.getAttribute('value')).toEqual('Done'); + expect(button.getAttribute('aria-expanded')).toEqual('true'); }); From 9651da1292a852acde3a4ee103c3d7e7ed55065e Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 19 Jan 2021 14:12:29 +0000 Subject: [PATCH 2/3] Improve focus control of radioSelect Makes focus shift to the first time in the range when you select a day. Also rewrites the code for controlling focus so it explains itself better, now it has different settings. --- app/assets/javascripts/radioSelect.js | 19 +++++++++++++------ tests/javascripts/radioSelect.test.js | 20 ++++++++++++++++++-- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/radioSelect.js b/app/assets/javascripts/radioSelect.js index 53c8d7f37..dcf7e8ee4 100644 --- a/app/assets/javascripts/radioSelect.js +++ b/app/assets/javascripts/radioSelect.js @@ -63,8 +63,13 @@ `) }; - let focusSelected = function(component) { - $('[type=radio]:checked', component).focus(); + let shiftFocus = function(target, component) { + if (target === 'now') { + $('[type=radio]', component).eq(0).focus(); + } + if (target === 'time') { + $('[type=radio]', component).eq(1).focus(); + } }; Modules.RadioSelect = function() { @@ -96,6 +101,7 @@ 'name': name, 'showNowAsDefault': showNowAsDefault }); + shiftFocus('now', component); }; const selectOption = (value) => { render('chosen', { @@ -105,7 +111,7 @@ 'name': name, 'showNowAsDefault': showNowAsDefault }); - focusSelected(component); + shiftFocus('time', component); }; const trackMouseup = (event) => { const parentNode = event.target.parentNode; @@ -134,7 +140,7 @@ 'name': name, 'showNowAsDefault': showNowAsDefault }); - focusSelected(component); + shiftFocus('time', component); }) .on('mousedown', '.js-option', function(event) { @@ -169,20 +175,21 @@ 'name': name, 'showNowAsDefault': showNowAsDefault }); + shiftFocus('time', component); } else { reset(); + shiftFocus('now', component); } - focusSelected(component); }) .on('click', '.radio-select__button--reset', function(event) { event.preventDefault(); reset(); - focusSelected(component); + shiftFocus('now', component); }); diff --git a/tests/javascripts/radioSelect.test.js b/tests/javascripts/radioSelect.test.js index ab5420e07..c4a6cb03f 100644 --- a/tests/javascripts/radioSelect.test.js +++ b/tests/javascripts/radioSelect.test.js @@ -228,9 +228,9 @@ describe('RadioSelect', () => { }); - test("keep focus on the default time slot", () => { + test("focus the first time slot", () => { - expect(document.activeElement).toBe(document.getElementById('scheduled_for-0')); + expect(document.activeElement).toBe(document.getElementById('scheduled_for-1')); }); @@ -347,6 +347,14 @@ describe('RadioSelect', () => { }) + test("focus the selected option", () => { + + selectedOption = document.querySelector('.radio-select__column:nth-child(2) input[checked=checked]'); + + expect(document.activeElement).toBe(selectedOption); + + }); + }); describe("selecting an option with the enter key should", () => { @@ -398,6 +406,14 @@ describe('RadioSelect', () => { }) + test("focus the selected option", () => { + + selectedOption = document.querySelector('.radio-select__column:nth-child(2) input[checked=checked]'); + + expect(document.activeElement).toBe(selectedOption); + + }); + }); test("clicking the 'Done' button should choose whatever time is selected", () => { From ba6b4682c3c09441e38c82aa8c82a86b83f49390 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 20 Jan 2021 11:23:01 +0000 Subject: [PATCH 3/3] Add comments to JS and improve selector in test After talking with the reviewer, it was decided that: 1. the JS could do with some comments to explain its structure and what various functions do better 2. some CSS selectors in the tests don't need to be as complex and simplifying them makes the test easier to read --- app/assets/javascripts/radioSelect.js | 27 ++++++++++++++++++--------- tests/javascripts/radioSelect.test.js | 6 +++--- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/radioSelect.js b/app/assets/javascripts/radioSelect.js index dcf7e8ee4..436397f34 100644 --- a/app/assets/javascripts/radioSelect.js +++ b/app/assets/javascripts/radioSelect.js @@ -5,6 +5,7 @@ var Modules = global.GOVUK.Modules; var Hogan = global.Hogan; + // Object holding all the states for the component's HTML let states = { 'initial': Hogan.compile(` {{#showNowAsDefault}} @@ -63,11 +64,12 @@ `) }; - let shiftFocus = function(target, component) { - if (target === 'now') { + let shiftFocus = function(elementToFocus, component) { + // The first option is always the default + if (elementToFocus === 'default') { $('[type=radio]', component).eq(0).focus(); } - if (target === 'time') { + if (elementToFocus === 'option') { $('[type=radio]', component).eq(1).focus(); } }; @@ -80,6 +82,7 @@ let render = (state, data) => { $component.html(states[state].render(data)); }; + // store array of all options in component let choices = $('label', $component).toArray().map(function(element) { let $element = $(element); return { @@ -95,13 +98,15 @@ $component.data('show-now-as-default').toString() === 'true' ? {'name': name} : false ); + + // functions for changing the state of the component's HTML const reset = () => { render('initial', { 'categories': categories, 'name': name, 'showNowAsDefault': showNowAsDefault }); - shiftFocus('now', component); + shiftFocus('default', component); }; const selectOption = (value) => { render('chosen', { @@ -111,8 +116,10 @@ 'name': name, 'showNowAsDefault': showNowAsDefault }); - shiftFocus('time', component); + shiftFocus('option', component); }; + + // use mousedown + mouseup event sequence to confirm option selection const trackMouseup = (event) => { const parentNode = event.target.parentNode; @@ -127,6 +134,7 @@ } }; + // set events $component .on('click', '.radio-select__button--category', function(event) { @@ -140,7 +148,7 @@ 'name': name, 'showNowAsDefault': showNowAsDefault }); - shiftFocus('time', component); + shiftFocus('option', component); }) .on('mousedown', '.js-option', function(event) { @@ -175,12 +183,12 @@ 'name': name, 'showNowAsDefault': showNowAsDefault }); - shiftFocus('time', component); + shiftFocus('option', component); } else { reset(); - shiftFocus('now', component); + shiftFocus('default', component); } @@ -189,10 +197,11 @@ event.preventDefault(); reset(); - shiftFocus('now', component); + shiftFocus('default', component); }); + // set HTML to initial state render('initial', { 'categories': categories, 'name': name, diff --git a/tests/javascripts/radioSelect.test.js b/tests/javascripts/radioSelect.test.js index c4a6cb03f..8dbaf711c 100644 --- a/tests/javascripts/radioSelect.test.js +++ b/tests/javascripts/radioSelect.test.js @@ -304,7 +304,7 @@ describe('RadioSelect', () => { test("focus the selected option", () => { - selectedOption = document.querySelector('.radio-select__column:nth-child(2) input[checked=checked]'); + selectedOption = document.querySelector('.radio-select__column input[checked=checked]'); expect(document.activeElement).toBe(selectedOption); @@ -349,7 +349,7 @@ describe('RadioSelect', () => { test("focus the selected option", () => { - selectedOption = document.querySelector('.radio-select__column:nth-child(2) input[checked=checked]'); + selectedOption = document.querySelector('.radio-select__column input[checked=checked]'); expect(document.activeElement).toBe(selectedOption); @@ -408,7 +408,7 @@ describe('RadioSelect', () => { test("focus the selected option", () => { - selectedOption = document.querySelector('.radio-select__column:nth-child(2) input[checked=checked]'); + selectedOption = document.querySelector('.radio-select__column input[checked=checked]'); expect(document.activeElement).toBe(selectedOption);