From f5cfa5f39a8a0816203c1c69f61c8611e0b4f82f Mon Sep 17 00:00:00 2001 From: Nick Colley Date: Tue, 6 Aug 2019 12:58:03 +0100 Subject: [PATCH 1/5] Only polyfill details component if native does not exist This change simplifies the polyfilling we do for details to only polyfill if the browser does not support the native details element. I have tested this in many different variations of assistive technologies and browsers and this seems to be a positive change. iOS VoiceOver goes from: "help with nationality, button" to "help with nationality, collapsed" "double tap to expand" One arguably negative is that in JAWs you can no longer use aria-controls functionality to move to the summary details, but given that the content is right under the summary I don't think this would cause a barrier. All this in mind, this feels like a really good future friendly change that'll allow GOV.UK Frontend to take advantage of native support for this element going forwards. --- src/govuk/components/details/details.js | 33 ++++++++++++------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/govuk/components/details/details.js b/src/govuk/components/details/details.js index a4d5df4f33..f9a715e814 100644 --- a/src/govuk/components/details/details.js +++ b/src/govuk/components/details/details.js @@ -11,9 +11,6 @@ import { generateUniqueID } from '../../common.js' var KEY_ENTER = 13 var KEY_SPACE = 32 -// Create a flag to know if the browser supports navtive details -var NATIVE_DETAILS = typeof document.createElement('details').open === 'boolean' - function Details ($module) { this.$module = $module } @@ -63,6 +60,13 @@ Details.prototype.init = function () { return } + // If there is native details support, we want to avoid running code to polyfill native behaviour. + var hasNativeDetails = typeof this.$module.open === 'boolean' + + if (hasNativeDetails) { + return + } + // Save shortcuts to the inner summary and content elements var $summary = this.$summary = $module.getElementsByTagName('summary').item(0) var $content = this.$content = $module.getElementsByTagName('div').item(0) @@ -92,9 +96,7 @@ Details.prototype.init = function () { // // We have to use the camelcase `tabIndex` property as there is a bug in IE6/IE7 when we set the correct attribute lowercase: // See http://web.archive.org/web/20170120194036/http://www.saliences.com/browserBugs/tabIndex.html for more information. - if (!NATIVE_DETAILS) { - $summary.tabIndex = 0 - } + $summary.tabIndex = 0 // Detect initial open state var openAttr = $module.getAttribute('open') !== null @@ -104,9 +106,7 @@ Details.prototype.init = function () { } else { $summary.setAttribute('aria-expanded', 'false') $content.setAttribute('aria-hidden', 'true') - if (!NATIVE_DETAILS) { - $content.style.display = 'none' - } + $content.style.display = 'none' } // Bind an event to handle summary elements @@ -128,16 +128,15 @@ Details.prototype.setAttributes = function () { $summary.setAttribute('aria-expanded', (expanded ? 'false' : 'true')) $content.setAttribute('aria-hidden', (hidden ? 'false' : 'true')) - if (!NATIVE_DETAILS) { - $content.style.display = (expanded ? 'none' : '') + $content.style.display = (expanded ? 'none' : '') - var hasOpenAttr = $module.getAttribute('open') !== null - if (!hasOpenAttr) { - $module.setAttribute('open', 'open') - } else { - $module.removeAttribute('open') - } + var hasOpenAttr = $module.getAttribute('open') !== null + if (!hasOpenAttr) { + $module.setAttribute('open', 'open') + } else { + $module.removeAttribute('open') } + return true } From 37508de468d22bbc072330a98360507dbc363a58 Mon Sep 17 00:00:00 2001 From: Nick Colley Date: Tue, 6 Aug 2019 13:01:59 +0100 Subject: [PATCH 2/5] Restructure details make clear what is polyfill code Since all JavaScript in this component is polyfill code by seperating this if we do add features to the details component that are not polyfill code in the future this'll give a clear distinction between the two. This'll make deleting this code in the future easier as well. --- src/govuk/components/details/details.js | 90 ++++++++++++------------- 1 file changed, 42 insertions(+), 48 deletions(-) diff --git a/src/govuk/components/details/details.js b/src/govuk/components/details/details.js index f9a715e814..9d8c131e5a 100644 --- a/src/govuk/components/details/details.js +++ b/src/govuk/components/details/details.js @@ -15,48 +15,8 @@ function Details ($module) { this.$module = $module } -/** -* Handle cross-modal click events -* @param {object} node element -* @param {function} callback function -*/ -Details.prototype.handleInputs = function (node, callback) { - node.addEventListener('keypress', function (event) { - var target = event.target - // When the key gets pressed - check if it is enter or space - if (event.keyCode === KEY_ENTER || event.keyCode === KEY_SPACE) { - if (target.nodeName.toLowerCase() === 'summary') { - // Prevent space from scrolling the page - // and enter from submitting a form - event.preventDefault() - // Click to let the click event do all the necessary action - if (target.click) { - target.click() - } else { - // except Safari 5.1 and under don't support .click() here - callback(event) - } - } - } - }) - - // Prevent keyup to prevent clicking twice in Firefox when using space key - node.addEventListener('keyup', function (event) { - var target = event.target - if (event.keyCode === KEY_SPACE) { - if (target.nodeName.toLowerCase() === 'summary') { - event.preventDefault() - } - } - }) - - node.addEventListener('click', callback) -} - Details.prototype.init = function () { - var $module = this.$module - - if (!$module) { + if (!this.$module) { return } @@ -67,6 +27,12 @@ Details.prototype.init = function () { return } + this.polyfillDetails() +} + +Details.prototype.polyfillDetails = function () { + var $module = this.$module + // Save shortcuts to the inner summary and content elements var $summary = this.$summary = $module.getElementsByTagName('summary').item(0) var $content = this.$content = $module.getElementsByTagName('div').item(0) @@ -110,14 +76,14 @@ Details.prototype.init = function () { } // Bind an event to handle summary elements - this.handleInputs($summary, this.setAttributes.bind(this)) + this.polyfillHandleInputs($summary, this.polyfillSetAttributes.bind(this)) } /** * Define a statechange function that updates aria-expanded and style.display * @param {object} summary element */ -Details.prototype.setAttributes = function () { +Details.prototype.polyfillSetAttributes = function () { var $module = this.$module var $summary = this.$summary var $content = this.$content @@ -141,13 +107,41 @@ Details.prototype.setAttributes = function () { } /** -* Remove the click event from the node element +* Handle cross-modal click events * @param {object} node element +* @param {function} callback function */ -Details.prototype.destroy = function (node) { - node.removeEventListener('keypress') - node.removeEventListener('keyup') - node.removeEventListener('click') +Details.prototype.polyfillHandleInputs = function (node, callback) { + node.addEventListener('keypress', function (event) { + var target = event.target + // When the key gets pressed - check if it is enter or space + if (event.keyCode === KEY_ENTER || event.keyCode === KEY_SPACE) { + if (target.nodeName.toLowerCase() === 'summary') { + // Prevent space from scrolling the page + // and enter from submitting a form + event.preventDefault() + // Click to let the click event do all the necessary action + if (target.click) { + target.click() + } else { + // except Safari 5.1 and under don't support .click() here + callback(event) + } + } + } + }) + + // Prevent keyup to prevent clicking twice in Firefox when using space key + node.addEventListener('keyup', function (event) { + var target = event.target + if (event.keyCode === KEY_SPACE) { + if (target.nodeName.toLowerCase() === 'summary') { + event.preventDefault() + } + } + }) + + node.addEventListener('click', callback) } export default Details From f389e0a2c3a3d9a3e9bd4cb20de90504f50ff3e7 Mon Sep 17 00:00:00 2001 From: Nick Colley Date: Wed, 7 Aug 2019 18:31:27 +0100 Subject: [PATCH 3/5] Add example with elements that are not native Since we're running tests in an environment that has native support for the details element, we need to fake a lack of support by using an example with divs. --- app/views/examples/details-polyfill/index.njk | 56 +++++++++ src/govuk/components/details/details.test.js | 114 +++++++++++------- 2 files changed, 129 insertions(+), 41 deletions(-) create mode 100644 app/views/examples/details-polyfill/index.njk diff --git a/app/views/examples/details-polyfill/index.njk b/app/views/examples/details-polyfill/index.njk new file mode 100644 index 0000000000..860b3aabe2 --- /dev/null +++ b/app/views/examples/details-polyfill/index.njk @@ -0,0 +1,56 @@ +{% from "back-link/macro.njk" import govukBackLink %} + +{% extends "layout.njk" %} + +{% block beforeContent %} + {{ govukBackLink({ + "href": "/" + }) }} +{% endblock %} + +{% block content %} +
+
+ +

+ Details polyfill examples +

+ +

+ In order to test our polyfill code for details we need to have examples that do not use the native elements. +

+ + +

+ Default +

+ +
+ + + Help with nationality + + +
+ We need to know your nationality so we can work out which elections you’re entitled to vote in. If you cannot provide your nationality, you’ll have to send copies of identity documents through the post. +
+
+ +

+ Expanded +

+ +
+ + + Help with nationality + + +
+ We need to know your nationality so we can work out which elections you’re entitled to vote in. If you cannot provide your nationality, you’ll have to send copies of identity documents through the post. +
+
+ +
+
+{% endblock %} diff --git a/src/govuk/components/details/details.test.js b/src/govuk/components/details/details.test.js index faeda7e19f..e2d7d49b6c 100644 --- a/src/govuk/components/details/details.test.js +++ b/src/govuk/components/details/details.test.js @@ -5,130 +5,162 @@ const PORT = configPaths.ports.test const baseUrl = 'http://localhost:' + PORT -describe('/components/details', () => { - describe('/components/details/preview', () => { +describe('details', () => { + describe('/examples/details-polyfill', () => { it('should add to summary the button role', async () => { - await page.goto(baseUrl + '/components/details/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - const summaryRole = await page.evaluate(() => document.body.getElementsByTagName('summary')[0].getAttribute('role')) + const summaryRole = await page.evaluate(() => { + return document.getElementById('default').querySelector('summary').getAttribute('role') + }) expect(summaryRole).toBe('button') }) it('should set the element controlled by the summary using aria-controls', async () => { - await page.goto(baseUrl + '/components/details/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - const summaryAriaControls = await page.evaluate(() => document.body.getElementsByTagName('summary')[0].getAttribute('aria-controls')) - const controlledContainerId = await page.evaluate(() => document.body.getElementsByTagName('details')[0].querySelectorAll('div')[0].getAttribute('id')) + const summaryAriaControls = await page.evaluate(() => { + return document.getElementById('default').querySelector('summary').getAttribute('aria-controls') + }) + const controlledContainerId = await page.evaluate(() => { + return document.getElementById('default').querySelector('.govuk-details__text').getAttribute('id') + }) expect(summaryAriaControls).toBe(controlledContainerId) }) it('should set the expanded state of the summary to false using aria-expanded', async () => { - await page.goto(baseUrl + '/components/details/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - const summaryAriaExpanded = await page.evaluate(() => document.body.getElementsByTagName('summary')[0].getAttribute('aria-expanded')) + const summaryAriaExpanded = await page.evaluate(() => { + return document.getElementById('default').querySelector('summary').getAttribute('aria-expanded') + }) expect(summaryAriaExpanded).toBe('false') }) it('should present the content as hidden using aria-hidden', async () => { - await page.goto(baseUrl + '/components/details/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - const hiddenContainerAriaHidden = await page.evaluate(() => document.body.getElementsByTagName('details')[0].querySelectorAll('div')[0].getAttribute('aria-hidden')) + const hiddenContainerAriaHidden = await page.evaluate(() => { + return document.getElementById('default').querySelector('.govuk-details__text').getAttribute('aria-hidden') + }) expect(hiddenContainerAriaHidden).toBe('true') }) it('should indicate the open state of the content', async () => { - await page.goto(baseUrl + '/components/details/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - const detailsOpen = await page.evaluate(() => document.body.getElementsByTagName('details')[0].getAttribute('open')) + const detailsOpen = await page.evaluate(() => { + return document.getElementById('default').getAttribute('open') + }) expect(detailsOpen).toBeNull() }) describe('when details is triggered', () => { it('should indicate the expanded state of the summary using aria-expanded', async () => { - await page.goto(baseUrl + '/components/details/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - await page.click('summary') + await page.click('#default summary') - const summaryAriaExpanded = await page.evaluate(() => document.body.getElementsByTagName('summary')[0].getAttribute('aria-expanded')) + const summaryAriaExpanded = await page.evaluate(() => { + return document.getElementById('default').querySelector('summary').getAttribute('aria-expanded') + }) expect(summaryAriaExpanded).toBe('true') }) it('should indicate the visible state of the content using aria-hidden', async () => { - await page.goto(baseUrl + '/components/details/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - await page.click('summary') + await page.click('#default summary') - const hiddenContainerAriaHidden = await page.evaluate(() => document.body.getElementsByTagName('details')[0].querySelectorAll('div')[0].getAttribute('aria-hidden')) + const hiddenContainerAriaHidden = await page.evaluate(() => { + return document.getElementById('default').querySelector('.govuk-details__text').getAttribute('aria-hidden') + }) expect(hiddenContainerAriaHidden).toBe('false') }) it('should indicate the open state of the content', async () => { - await page.goto(baseUrl + '/components/details/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - await page.click('summary') + await page.click('#default summary') - const detailsOpen = await page.evaluate(() => document.body.getElementsByTagName('details')[0].getAttribute('open')) + const detailsOpen = await page.evaluate(() => { + return document.getElementById('default').getAttribute('open') + }) expect(detailsOpen).not.toBeNull() }) }) }) - describe('/components/details/expanded/preview', () => { + describe('expanded', () => { it('should indicate the expanded state of the summary using aria-expanded', async () => { - await page.goto(baseUrl + '/components/details/expanded/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - const summaryAriaExpanded = await page.evaluate(() => document.body.getElementsByTagName('summary')[0].getAttribute('aria-expanded')) + const summaryAriaExpanded = await page.evaluate(() => { + return document.getElementById('expanded').querySelector('summary').getAttribute('aria-expanded') + }) expect(summaryAriaExpanded).toBe('true') }) it('should indicate the visible state of the content using aria-hidden', async () => { - await page.goto(baseUrl + '/components/details/expanded/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - const hiddenContainerAriaHidden = await page.evaluate(() => document.body.getElementsByTagName('details')[0].querySelectorAll('div')[0].getAttribute('aria-hidden')) + const hiddenContainerAriaHidden = await page.evaluate(() => { + return document.getElementById('expanded').querySelector('.govuk-details__text').getAttribute('aria-hidden') + }) expect(hiddenContainerAriaHidden).toBe('false') }) it('should indicate the open state of the content', async () => { - await page.goto(baseUrl + '/components/details/expanded/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - const detailsOpen = await page.evaluate(() => document.body.getElementsByTagName('details')[0].getAttribute('open')) + const detailsOpen = await page.evaluate(() => { + return document.getElementById('expanded').getAttribute('open') + }) expect(detailsOpen).not.toBeNull() }) it('should not be affected when clicking the revealed content', async () => { - await page.goto(baseUrl + '/components/details/expanded/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - await page.click('.govuk-details__text') + await page.click('#expanded .govuk-details__text') - const summaryAriaExpanded = await page.evaluate(() => document.body.getElementsByTagName('summary')[0].getAttribute('aria-expanded')) + const summaryAriaExpanded = await page.evaluate(() => { + return document.getElementById('expanded').querySelector('summary').getAttribute('aria-expanded') + }) expect(summaryAriaExpanded).toBe('true') }) describe('when details is triggered', () => { it('should indicate the expanded state of the summary using aria-expanded', async () => { - await page.goto(baseUrl + '/components/details/expanded/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - await page.click('summary') + await page.click('#expanded summary') - const summaryAriaExpanded = await page.evaluate(() => document.body.getElementsByTagName('summary')[0].getAttribute('aria-expanded')) + const summaryAriaExpanded = await page.evaluate(() => { + return document.getElementById('expanded').querySelector('summary').getAttribute('aria-expanded') + }) expect(summaryAriaExpanded).toBe('false') }) it('should indicate the visible state of the content using aria-hidden', async () => { - await page.goto(baseUrl + '/components/details/expanded/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - await page.click('summary') + await page.click('#expanded summary') - const hiddenContainerAriaHidden = await page.evaluate(() => document.body.getElementsByTagName('details')[0].querySelectorAll('div')[0].getAttribute('aria-hidden')) + const hiddenContainerAriaHidden = await page.evaluate(() => { + return document.getElementById('expanded').querySelector('.govuk-details__text').getAttribute('aria-hidden') + }) expect(hiddenContainerAriaHidden).toBe('true') }) it('should indicate the open state of the content', async () => { - await page.goto(baseUrl + '/components/details/expanded/preview', { waitUntil: 'load' }) + await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' }) - await page.click('summary') + await page.click('#expanded summary') - const detailsOpen = await page.evaluate(() => document.body.getElementsByTagName('details')[0].getAttribute('open')) + const detailsOpen = await page.evaluate(() => { + return document.getElementById('expanded').getAttribute('open') + }) expect(detailsOpen).toBeNull() }) }) From 633fd0f59a391ab702cc4803f959c58b6c0b835c Mon Sep 17 00:00:00 2001 From: Nick Colley Date: Thu, 8 Aug 2019 15:18:55 +0100 Subject: [PATCH 4/5] Add CHANGELOG entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f4cb685a2..0e98270038 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixes +- [Pull request #1523: Improve accessibility of details component by polyfilling only where the native element is not available](https://github.com/alphagov/govuk-frontend/pull/1523). - [Pull request #1512: Update components to only output items when they are defined](https://github.com/alphagov/govuk-frontend/pull/1512). ## 3.0.0 (Breaking release) From e4ed929d519e1503ff118c076122de4f376bf44f Mon Sep 17 00:00:00 2001 From: Nick Colley Date: Wed, 28 Aug 2019 12:32:23 +0100 Subject: [PATCH 5/5] Test details polyfill is not run on native element --- src/govuk/components/details/details.test.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/govuk/components/details/details.test.js b/src/govuk/components/details/details.test.js index e2d7d49b6c..8726334049 100644 --- a/src/govuk/components/details/details.test.js +++ b/src/govuk/components/details/details.test.js @@ -6,6 +6,14 @@ const PORT = configPaths.ports.test const baseUrl = 'http://localhost:' + PORT describe('details', () => { + it('should not polyfill when details element is available', async () => { + await page.goto(baseUrl + '/components/details/preview', { waitUntil: 'load' }) + + const summaryAriaExpanded = await page.evaluate(() => { + return document.querySelector('summary').getAttribute('aria-expanded') + }) + expect(summaryAriaExpanded).toBe(null) + }) describe('/examples/details-polyfill', () => { it('should add to summary the button role', async () => { await page.goto(baseUrl + '/examples/details-polyfill', { waitUntil: 'load' })