From 0888dc83170584fe72dd435cd6a68c0eaf598e72 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Mon, 3 Jun 2024 17:15:28 +0100 Subject: [PATCH 1/9] Simplify checkIfAllSectionsOpen Rather than checking how many sections have the expanded class, check if all sections have the class. --- .../src/govuk/components/accordion/accordion.mjs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs index 5ca6bb5741..0c196ace79 100644 --- a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs +++ b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs @@ -491,13 +491,9 @@ export class Accordion extends GOVUKFrontendComponent { * @returns {boolean} True if all sections are open */ checkIfAllSectionsOpen() { - const sectionsCount = this.$sections.length - const expandedSectionCount = this.$module.querySelectorAll( - `.${this.sectionExpandedClass}` - ).length - const areAllSectionsOpen = sectionsCount === expandedSectionCount - - return areAllSectionsOpen + return Array.from(this.$sections).every(($section) => + this.isExpanded($section) + ) } /** From 86332556b85351e362fbfd717ab8884c4384431c Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Tue, 4 Jun 2024 15:41:58 +0100 Subject: [PATCH 2/9] Prefer `name` and `value` properties for Attr `Element.attributes` returns a NamedNodeMap [1] which represents a collection of of `Attr` objects [1]. `Attr` objects are a specialised class of `Node` which have `name` and `value` properties [2]. Prefer the specialised `name` and `value` properties over the `nodeName` and `nodeValue` properties which are inherited from `Node` [3]. Both `nodeName` and `name` return the qualified name of the attribute (including any namespace) [4]. As Typescript knows that `Attr.value` will always return a string, we no longer need to cast it using the template literal. [1]: https://developer.mozilla.org/en-US/docs/Web/API/Element/attributes [2]: https://dom.spec.whatwg.org/#attr [3]: https://dom.spec.whatwg.org/#node [4]: https://dom.spec.whatwg.org/#dom-node-nodename --- .../src/govuk/components/accordion/accordion.mjs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs index 0c196ace79..61d60bea80 100644 --- a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs +++ b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs @@ -263,8 +263,8 @@ export class Accordion extends GOVUKFrontendComponent { // Copy all attributes from $span to $button (except `id`, which gets added // to the `$headingText` element) for (const attr of Array.from($span.attributes)) { - if (attr.nodeName !== 'id') { - $button.setAttribute(attr.nodeName, `${attr.nodeValue}`) + if (attr.name !== 'id') { + $button.setAttribute(attr.name, attr.value) } } @@ -326,7 +326,7 @@ export class Accordion extends GOVUKFrontendComponent { // Get original attributes, and pass them to the replacement for (const attr of Array.from($summary.attributes)) { - $summarySpan.setAttribute(attr.nodeName, `${attr.nodeValue}`) + $summarySpan.setAttribute(attr.name, attr.value) } // Copy original contents of summary to the new summary span From 2eff6a31d377d96d0ea5e69026c485e04c6fc3a8 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Tue, 4 Jun 2024 15:43:08 +0100 Subject: [PATCH 3/9] Set text using textContent over innerHTML --- .../govuk-frontend/src/govuk/components/accordion/accordion.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs index 61d60bea80..64d779232a 100644 --- a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs +++ b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs @@ -580,7 +580,7 @@ export class Accordion extends GOVUKFrontendComponent { 'govuk-visually-hidden', this.sectionHeadingDividerClass ) - $punctuationEl.innerHTML = ', ' + $punctuationEl.textContent = ', ' return $punctuationEl } From d755e592c7e7722759b8b92be0b3d19fe5ac433e Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Tue, 4 Jun 2024 15:48:20 +0100 Subject: [PATCH 4/9] Pass expanded state in to storeState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We ‘know’ the expanded state in both places we call storeState, so pass it to the function rather than reading the state back out of the DOM. --- .../govuk/components/accordion/accordion.mjs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs index 64d779232a..85194d67b1 100644 --- a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs +++ b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs @@ -373,11 +373,11 @@ export class Accordion extends GOVUKFrontendComponent { * @param {Element} $section - Section element */ onSectionToggle($section) { - const expanded = this.isExpanded($section) - this.setExpanded(!expanded, $section) + const nowExpanded = !this.isExpanded($section) + this.setExpanded(nowExpanded, $section) // Store the state in sessionStorage when a change is triggered - this.storeState($section) + this.storeState($section, nowExpanded) } /** @@ -390,7 +390,7 @@ export class Accordion extends GOVUKFrontendComponent { this.$sections.forEach(($section) => { this.setExpanded(nowExpanded, $section) - this.storeState($section) + this.storeState($section, nowExpanded) }) this.updateShowAllButton(nowExpanded) @@ -519,8 +519,9 @@ export class Accordion extends GOVUKFrontendComponent { * * @private * @param {Element} $section - Section element + * @param {boolean} isExpanded - Whether the section is expanded */ - storeState($section) { + storeState($section, isExpanded) { if (this.browserSupportsSessionStorage && this.config.rememberExpanded) { // We need a unique way of identifying each content in the Accordion. // Since an `#id` should be unique and an `id` is required for `aria-` @@ -529,12 +530,9 @@ export class Accordion extends GOVUKFrontendComponent { if ($button) { const contentId = $button.getAttribute('aria-controls') - const contentState = $button.getAttribute('aria-expanded') - // Only set the state when both `contentId` and `contentState` are taken - // from the DOM. - if (contentId && contentState) { - window.sessionStorage.setItem(contentId, contentState) + if (contentId) { + window.sessionStorage.setItem(contentId, isExpanded.toString()) } } } From 3d5aa0353736f160e9c51c5c497761da28ed9a90 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Tue, 4 Jun 2024 15:51:39 +0100 Subject: [PATCH 5/9] Move children using appendChild rather than innerHTML MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’d be nice to use `replaceChildren`, but alas the browser support is not there. --- .../src/govuk/components/accordion/accordion.mjs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs index 85194d67b1..6658c6d388 100644 --- a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs +++ b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs @@ -282,7 +282,9 @@ export class Accordion extends GOVUKFrontendComponent { $headingText.appendChild($headingTextFocus) // span could contain HTML elements // (see https://www.w3.org/TR/2011/WD-html5-20110525/content-models.html#phrasing-content) - $headingTextFocus.innerHTML = $span.innerHTML + Array.from($span.childNodes).forEach(($child) => + $headingTextFocus.appendChild($child) + ) // Create container for show / hide icons and text. const $showHideToggle = document.createElement('span') @@ -330,7 +332,9 @@ export class Accordion extends GOVUKFrontendComponent { } // Copy original contents of summary to the new summary span - $summarySpanFocus.innerHTML = $summary.innerHTML + Array.from($summary.childNodes).forEach(($child) => + $summarySpanFocus.appendChild($child) + ) // Replace the original summary `div` with the new summary `span` $summary.parentNode.replaceChild($summarySpan, $summary) From 53398892d5633d02c9d1e16d5ad1529b2b85df1a Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Tue, 4 Jun 2024 15:56:06 +0100 Subject: [PATCH 6/9] Avoid unnecessary replacement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no point replacing the existing `$summary` with the `$summarySpan` [^1] as we end up moving the `$summarySpan` into the `$button` on the next line anyway. All we really need to do is remove the existing summary, which is much simpler. Because we don’t need to traverse to the parent we can also simplify the guard for this block of code by removing the check for `$summary.parentNode`. [^1]: Note that the argument order for `Node.replaceChild` may not be what you expect (`replaceChild(newChild, oldChild)`) --- .../src/govuk/components/accordion/accordion.mjs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs index 6658c6d388..20019e1ddf 100644 --- a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs +++ b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs @@ -314,7 +314,7 @@ export class Accordion extends GOVUKFrontendComponent { $button.appendChild(this.getButtonPunctuationEl()) // If summary content exists add to DOM in correct order - if ($summary?.parentNode) { + if ($summary) { // Create a new `span` element and copy the summary line content from the // original `div` to the new `span`. This is because the summary line text // is now inside a button element, which can only contain phrasing @@ -337,7 +337,7 @@ export class Accordion extends GOVUKFrontendComponent { ) // Replace the original summary `div` with the new summary `span` - $summary.parentNode.replaceChild($summarySpan, $summary) + $summary.remove() $button.appendChild($summarySpan) $button.appendChild(this.getButtonPunctuationEl()) From 46abfb29f8d4d9205d165b68f94a688ce5375922 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Tue, 4 Jun 2024 16:39:21 +0100 Subject: [PATCH 7/9] Avoid unnecessary areAllSectionsOpen variables --- .../src/govuk/components/accordion/accordion.mjs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs index 20019e1ddf..b2f8648ac5 100644 --- a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs +++ b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs @@ -151,9 +151,7 @@ export class Accordion extends GOVUKFrontendComponent { this.initControls() this.initSectionHeaders() - // See if "Show all sections" button text should be updated - const areAllSectionsOpen = this.checkIfAllSectionsOpen() - this.updateShowAllButton(areAllSectionsOpen) + this.updateShowAllButton(this.checkIfAllSectionsOpen()) } /** @@ -473,8 +471,7 @@ export class Accordion extends GOVUKFrontendComponent { } // See if "Show all sections" button text should be updated - const areAllSectionsOpen = this.checkIfAllSectionsOpen() - this.updateShowAllButton(areAllSectionsOpen) + this.updateShowAllButton(this.checkIfAllSectionsOpen()) } /** From d2dcd624d431269508fc13b97c5a8e43a563a483 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Tue, 4 Jun 2024 16:39:51 +0100 Subject: [PATCH 8/9] Rename checkIfAllSectionsOpen to areAllSectionsOpen --- .../src/govuk/components/accordion/accordion.mjs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs index b2f8648ac5..f6d2910137 100644 --- a/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs +++ b/packages/govuk-frontend/src/govuk/components/accordion/accordion.mjs @@ -151,7 +151,7 @@ export class Accordion extends GOVUKFrontendComponent { this.initControls() this.initSectionHeaders() - this.updateShowAllButton(this.checkIfAllSectionsOpen()) + this.updateShowAllButton(this.areAllSectionsOpen()) } /** @@ -388,7 +388,7 @@ export class Accordion extends GOVUKFrontendComponent { * @private */ onShowOrHideAllToggle() { - const nowExpanded = !this.checkIfAllSectionsOpen() + const nowExpanded = !this.areAllSectionsOpen() this.$sections.forEach(($section) => { this.setExpanded(nowExpanded, $section) @@ -471,7 +471,7 @@ export class Accordion extends GOVUKFrontendComponent { } // See if "Show all sections" button text should be updated - this.updateShowAllButton(this.checkIfAllSectionsOpen()) + this.updateShowAllButton(this.areAllSectionsOpen()) } /** @@ -491,7 +491,7 @@ export class Accordion extends GOVUKFrontendComponent { * @private * @returns {boolean} True if all sections are open */ - checkIfAllSectionsOpen() { + areAllSectionsOpen() { return Array.from(this.$sections).every(($section) => this.isExpanded($section) ) From bed231c602ce9f6c91258f0b12fca15dd32be8ce Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Tue, 11 Jun 2024 13:24:42 +0100 Subject: [PATCH 9/9] Document in CHANGELOG --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e284670423..0818fb132d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ For advice on how to use these release notes see [our guidance on staying up to ## Unreleased +### Fixes + +We've made fixes to GOV.UK Frontend in the following pull requests: + +- [#5043: Refactor the accordion JavaScript](https://github.com/alphagov/govuk-frontend/pull/5043) + ## 5.4.0 (Feature release) To install this version with npm, run `npm install govuk-frontend@5.4.0`. You can also find more information about [how to stay up to date](https://frontend.design-system.service.gov.uk/staying-up-to-date/#updating-to-the-latest-version) in our documentation.