From 3fa2650ffaa329fb1aa82ab9dada36cd701fe0e0 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 8 Feb 2022 11:33:13 +0000 Subject: [PATCH 01/16] Make updateContent persist specified classNames Wrap the code that updates the HTML with changes from the server with code that stores and re-applies specified classes. This is to allow other JS to add classes which change the visual state of the HTML without them being considered by the code that diffs our in-page HTML against that from the server. They are called classesToPersist because this should make the visual state they create persist between updates. Includes the addition of tests for updateContent that cover the addition/deletion of elements so we can write a test for classNames persisting through updates. The existing tests only cover updates that change the content of elements. Just adding the test for these changes to those would simulate a scenario that doesn't exist in the app. Writing extra tests for the kind of updates these changes act on keeps them in line with the app code. --- app/assets/javascripts/updateContent.js | 64 ++- tests/javascripts/updateContent.test.js | 590 +++++++++++++++++------- 2 files changed, 464 insertions(+), 190 deletions(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index 2e4bc4758..d6d678e08 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -11,10 +11,37 @@ 1000 )); - var getRenderer = $component => response => morphdom( - $component.get(0), - $(response[$component.data('key')]).get(0) - ); + // Methods to ensure the DOM fragment is clean of classes added by JS before diffing + // and that they are replaced afterwards. + var classesToPersist = { + classNames: [], + $els: [], + remove: function () { + this.classNames.forEach(className => { + var $elsWithClassName = $('.' + className).removeClass(className); + + // store elements for that className at the same index + this.$els.push($elsWithClassName); + }); + }, + replace: function () { + this.classNames.forEach((className, index) => { + this.$els[index].addClass(className); + }); + + // remove references to elements + this.$els = []; + } + }; + + var getRenderer = $component => response => { + classesToPersist.remove(); + morphdom( + $component.get(0), + $(response[$component.data('key')]).get(0) + ); + classesToPersist.replace(); + }; var getQueue = resource => ( queues[resource] = queues[resource] || [] @@ -55,15 +82,26 @@ global.GOVUK.Modules.UpdateContent = function() { - this.start = component => setTimeout( - () => poll( - getRenderer($(component)), - $(component).data('resource'), - getQueue($(component).data('resource')), - $(component).data('form') - ), - defaultInterval - ); + this.start = component => { + var $component = $(component); + + // store any classes that should persist through updates + if ($contents.data('classesToPersist') !== undefined) { + $contents.data('classesToPersist') + .split(' ') + .forEach(className => classesToPersist.classNames.push(className)); + } + + setTimeout( + () => poll( + getRenderer($component), + $component.data('resource'), + getQueue($component.data('resource')), + $component.data('form') + ), + defaultInterval + ); + }; }; diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js index d965906b8..aff63d767 100644 --- a/tests/javascripts/updateContent.test.js +++ b/tests/javascripts/updateContent.test.js @@ -50,56 +50,426 @@ describe('Update content', () => { let HTMLString; let initialHTMLString; - beforeEach(() => { + describe('When updating the contents of DOM nodes', () => { - // store HTML in string to allow use in AJAX responses - HTMLString = ` -
- -
`; + + + `; + }; + var getHTMLString = items => { - initialHTMLString = `
- ${HTMLString} -
`; + var itemsHTMLString = ''; - document.body.innerHTML = initialHTMLString; + items.forEach(item => itemsHTMLString += "\n" + getItemHTMLString(item)); - // default the response to match the content inside div[data-module] - responseObj[updateKey] = HTMLString; + return `
+ ${itemsHTMLString}; +
+
`; + + }; + + test("If the response contains no changes, the DOM should stay the same", () => { + + // store HTML in string to allow use in AJAX responses + HTMLString = getHTMLString([ + { + title: "Gas leak", + hint: "There's a gas leak in the local area. Residents should vacate until further notice.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi B", + "Santa Claus Village, Rovaniemi C" + ] + } + ]); + + initialHTMLString = `
+ ${HTMLString} +
`; + + document.body.innerHTML = initialHTMLString; + + // make the response have an extra item + responseObj[updateKey] = HTMLString; + + // start the module + window.GOVUK.modules.start(); + jest.advanceTimersByTime(2000); + + // check it has the same number of items + expect(document.querySelectorAll('.file-list').length).toEqual(1); + expect(document.querySelectorAll('.file-list h2 a')[0].textContent.trim()).toEqual("Gas leak"); + + }); + + test("If the response adds a node, the DOM should contain that node", () => { + + // store HTML in string to allow use in AJAX responses + HTMLString = getHTMLString([ + { + title: "Gas leak", + hint: "There's a gas leak in the local area. Residents should vacate until further notice.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi B", + "Santa Claus Village, Rovaniemi C" + ] + } + ]); + + initialHTMLString = `
+ ${HTMLString} +
`; + + document.body.innerHTML = initialHTMLString; + + var updatedHTMLString = getHTMLString([ + { + title: "Gas leak", + hint: "There's a gas leak in the local area. Residents should vacate until further notice.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi B", + "Santa Claus Village, Rovaniemi C" + ] + }, + { + title: "Reservoir flooding template", + hint: "The local reservoir has flooded. All people within 5 miles should move to a safer location.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi A", + "Santa Claus Village, Rovaniemi D" + ] + } + ]); + + // make the response have an extra item + responseObj[updateKey] = updatedHTMLString; + + // start the module + window.GOVUK.modules.start(); + jest.advanceTimersByTime(2000); + + // check the node has been added + expect(document.querySelectorAll('.file-list').length).toEqual(2); + expect(document.querySelectorAll('.file-list h2 a')[0].textContent.trim()).toEqual("Gas leak"); + expect(document.querySelectorAll('.file-list h2 a')[1].textContent.trim()).toEqual("Reservoir flooding template"); + + }); + + test("If the response removes a node, the DOM should not contain that node", () => { + + // store HTML in string to allow use in AJAX responses + HTMLString = getHTMLString([ + { + title: "Gas leak", + hint: "There's a gas leak in the local area. Residents should vacate until further notice.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi B", + "Santa Claus Village, Rovaniemi C" + ] + }, + { + title: "Reservoir flooding template", + hint: "The local reservoir has flooded. All people within 5 miles should move to a safer location.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi A", + "Santa Claus Village, Rovaniemi D" + ] + } + ]); + + initialHTMLString = `
+ ${HTMLString} +
`; + + document.body.innerHTML = initialHTMLString; + + var updatedHTMLString = getHTMLString([ + { + title: "Gas leak", + hint: "There's a gas leak in the local area. Residents should vacate until further notice.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi B", + "Santa Claus Village, Rovaniemi C" + ] + } + ]); + + // default the response to match the content inside div[data-module] + responseObj[updateKey] = updatedHTMLString; + + // start the module + window.GOVUK.modules.start(); + jest.advanceTimersByTime(2000); + + // check the node has been removed + expect(document.querySelectorAll('.file-list').length).toEqual(1); + expect(document.querySelectorAll('.file-list h2 a')[0].textContent.trim()).toEqual("Gas leak"); + + }); + + test("If other scripts have added classes to the DOM, they should persist through updates", () => { + + // store HTML in string to allow use in AJAX responses + HTMLString = getHTMLString([ + { + title: "Gas leak", + hint: "There's a gas leak in the local area. Residents should vacate until further notice.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi B", + "Santa Claus Village, Rovaniemi C" + ] + } + ]); + + initialHTMLString = `
+ ${HTMLString} +
`; + + document.body.innerHTML = initialHTMLString; + + // mark classes to persist on the partial + document.querySelector('.ajax-block-container').setAttribute('data-classes-to-persist', 'js-child-has-focus'); + + // Add class to indicate focus state of link on parent heading + document.querySelectorAll('.file-list h2')[0].classList.add('js-child-has-focus'); + + // make the response match the initial HTML to emulate a response with no changes + responseObj[updateKey] = HTMLString; + + // start the module + window.GOVUK.modules.start(); + jest.advanceTimersByTime(2000); + + // check the class is still there + expect(document.querySelectorAll('.file-list h2')[0].classList.contains('js-child-has-focus')).toBe(true); + + }); }); @@ -115,138 +485,4 @@ describe('Update content', () => { }); - test("It should make requests to the URL specified in the data-resource attribute", () => { - - // start the module - window.GOVUK.modules.start(); - jest.advanceTimersByTime(2000); - - expect($.ajax.mock.calls[0][0]).toEqual(resourceURL); - - }); - - test("If the response contains no changes, the DOM should stay the same", () => { - - // send the done callback a response with updates included - responseObj[updateKey] = HTMLString; - - // start the module - window.GOVUK.modules.start(); - jest.advanceTimersByTime(2000); - - // check the right DOM node is updated - expect(document.querySelectorAll('.big-number-number')[0].textContent.trim()).toEqual("0"); - - }); - - test("If the response contains changes, it should update the DOM with them", () => { - - // send the done callback a response with updates included - responseObj[updateKey] = HTMLString.replace(/
0<\/div>{1}/, '
1
'); - - // start the module - window.GOVUK.modules.start(); - jest.advanceTimersByTime(2000); - - // check the right DOM node is updated - expect(document.querySelectorAll('.big-number-number')[0].textContent.trim()).toEqual("1"); - - }); - - describe("By default", () => { - - beforeEach(() => { - - // start the module - window.GOVUK.modules.start(); - - }); - - test("It should use the GET HTTP method", () => { - - jest.advanceTimersByTime(2000); - expect($.ajax.mock.calls[0][1].method).toEqual('get'); - - }); - - test("It shouldn't send any data as part of the requests", () => { - - jest.advanceTimersByTime(2000); - expect($.ajax.mock.calls[0][1].data).toEqual({}); - - }); - - test("It should request updates with a dynamic interval", () => { - - // First call doesn’t happen in the first 2000ms - jest.advanceTimersByTime(1999); - expect($.ajax).toHaveBeenCalledTimes(0); - - // But it happens after 2000ms by default - jest.advanceTimersByTime(1); - expect($.ajax).toHaveBeenCalledTimes(1); - - // It took the server 1000ms to respond to the first call so we - // will back off – the next call shouldn’t happen in the next 6904ms - jest.advanceTimersByTime(6904); - expect($.ajax).toHaveBeenCalledTimes(1); - - // But it should happen after 6905ms - jest.advanceTimersByTime(1); - expect($.ajax).toHaveBeenCalledTimes(2); - - }); - - each([ - [1000, 0], - [1500, 100], - [4590, 500], - [6905, 1000], - [24000, 10000], - ]).test('It calculates a delay of %dms if the API responds in %dms', (waitTime, responseTime) => { - expect( - window.GOVUK.Modules.UpdateContent.calculateBackoff(responseTime) - ).toBe( - waitTime - ); - }); - - }); - - describe("If a form is used as a source for data, referenced in the data-form attribute", () => { - - beforeEach(() => { - - document.body.innerHTML += ` -
- - -
`; - - document.querySelector('[data-module=update-content]').setAttribute('data-form', 'service'); - - // start the module - window.GOVUK.modules.start(); - - }); - - test("requests should use the same HTTP method as the form", () => { - - jest.advanceTimersByTime(2000); - expect($.ajax.mock.calls[0][1].method).toEqual('post'); - - }) - - test("requests should use the data from the form", () => { - - jest.advanceTimersByTime(2000); - expect($.ajax.mock.calls[0][1].data).toEqual(helpers.getFormDataFromPairs([ - ['serviceName', 'Buckhurst surgery'], - ['serviceNumber', serviceNumber] - ])); - - }) - - }); - }); From d76648b67b319dae8264036ca99ed97aca191654 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 8 Feb 2022 11:07:57 +0000 Subject: [PATCH 02/16] Mark focus of link on parent heading in file-list Includes: - JS to add a class to the heading when the link is focused - CSS to apply the enlarged focus style via a selector which uses that class - changes to the partial to hook in the JS to track focus on links and to tell the updateContent JS to persist the classes added between updates to the HTML --- app/assets/javascripts/main.js | 9 +++++++ app/assets/stylesheets/views/dashboard.scss | 27 ++++++++++++------- .../broadcast/partials/dashboard-table.html | 2 +- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index a6cb2ccba..2d056032e 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -24,3 +24,12 @@ $(() => $('.banner-dangerous').eq(0).trigger('focus')); $(() => $('.govuk-header__container').on('click', function() { $(this).css('border-color', '#005ea5'); })); + +$('.js-mark-focus-on-parent').on('focus blur', '*', e => { + $target = $(e.target); + if (e.type === 'focusin') { + $target.parent().addClass('js-child-has-focus'); + } else { + $target.parent().removeClass('js-child-has-focus'); + } +}); diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index f97c98c39..82455a8e5 100644 --- a/app/assets/stylesheets/views/dashboard.scss +++ b/app/assets/stylesheets/views/dashboard.scss @@ -44,6 +44,13 @@ .file-list { + &-hint, + &-hint-large, + &-status { + pointer-events: none; // delegate clicks to the overlapping link + position: relative; // make non-static to sit above the overlapping focus style + } + &-filename { @include bold-19; display: block; @@ -84,7 +91,6 @@ @include core-16; display: block; color: $secondary-text-colour; - pointer-events: none; overflow: hidden; white-space: nowrap; text-overflow: ellipsis; @@ -95,7 +101,6 @@ @include core-19; display: block; color: $secondary-text-colour; - pointer-events: none; overflow: hidden; white-space: nowrap; text-overflow: ellipsis; @@ -110,13 +115,6 @@ /* The focus state for sibling links overlaps the hint so the hint's text colour needs to adapt */ .govuk-link:focus { - &.file-list-filename, - &.file-list-filename-large, - & + .file-list-hint, - & + .file-list-hint-large { - /* link needs non-static position to overlap the cell border, hint so it isn't overlapped itself */ - position: relative; - } &.file-list-filename, &.file-list-filename-large { @@ -131,6 +129,17 @@ } +// Note: this selector should use the :has() pseudo-class in the future when it is supported +// which will remove the need for JS +.file-list .js-child-has-focus + .govuk-grid-row { + + & .file-list-hint-large, + & .file-list-status > .govuk-hint { + color: $govuk-focus-text-colour; + } + +} + .failure-highlight { @include bold-19; color: $error-colour; diff --git a/app/templates/views/broadcast/partials/dashboard-table.html b/app/templates/views/broadcast/partials/dashboard-table.html index 7a05257e9..bc56788cc 100644 --- a/app/templates/views/broadcast/partials/dashboard-table.html +++ b/app/templates/views/broadcast/partials/dashboard-table.html @@ -1,6 +1,6 @@ {% from "components/table.html" import list_table, field, right_aligned_field_heading, row_heading %} -
+
{% for item in broadcasts|sort|reverse|list %}
From 476ed1593cb1e438dc7b7d2a509c54c6f3fef8ff Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 4 Feb 2022 11:48:43 +0000 Subject: [PATCH 03/16] Make updateContent handle all AJAX the same The current updateContent JS replaces the in-page HTML with the HTML from the server the first time an AJAX request is fired, even if the HTML from the server has no changes. This is because the code that compares the two operates on two different things: The HTML in the page is the component HTML, with all the data attributes and the partial HTML (marked with the 'ajax-block-container' class) as its first child: ```
...
``` The HTML from the server only contains the partial: ```
...
``` The diffing code just sees them as different at the top level so replaces the page HTML with the partial from the server. This means all subsequent diffs are between partial HTML and partial HTML so only update on actual changes. These replace the component with the partial, as part of the component initialising. This means all code that runs on an AJAX response will only compare like-for-like so will result in actual changes (or none at all), not just swapping one element out for another. Note: this commit also removes the aria-live="polite" from the ajax_block component. It has always been overwritten by the first response so never announces anything to assistive technologies. Removing it makes this more clear. --- app/assets/javascripts/updateContent.js | 22 +++++++++++++++------- app/templates/components/ajax-block.html | 1 - 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index d6d678e08..b73defac9 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -34,11 +34,11 @@ } }; - var getRenderer = $component => response => { + var getRenderer = ($contents, key) => response => { classesToPersist.remove(); morphdom( - $component.get(0), - $(response[$component.data('key')]).get(0) + $contents.get(0), + $(response[key]).get(0) ); classesToPersist.replace(); }; @@ -84,6 +84,14 @@ this.start = component => { var $component = $(component); + var $contents = $component.children().eq(0); + var key = $component.data('key'); + var resource = $component.data('resource'); + var form = $component.data('form'); + + // replace component with contents + // the renderer does this anyway when diffing against the first response + $component.replaceWith($contents); // store any classes that should persist through updates if ($contents.data('classesToPersist') !== undefined) { @@ -94,10 +102,10 @@ setTimeout( () => poll( - getRenderer($component), - $component.data('resource'), - getQueue($component.data('resource')), - $component.data('form') + getRenderer($contents, key), + resource, + getQueue(resource), + form ), defaultInterval ); diff --git a/app/templates/components/ajax-block.html b/app/templates/components/ajax-block.html index 3638e13d3..c73978725 100644 --- a/app/templates/components/ajax-block.html +++ b/app/templates/components/ajax-block.html @@ -5,7 +5,6 @@ data-resource="{{ url }}" data-key="{{ key }}" data-form="{{ form }}" - aria-live="polite" > {% endif %} {{ partials[key]|safe }} From 99df8542b4b3fa60e779290db5f806b5d9d4f27e Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 11 Feb 2022 12:30:07 +0000 Subject: [PATCH 04/16] Add guard against elements removed from the DOM We can't guarantee that elements we stored a reference to with `classesToPersist.remove` will still exist so we need to guard against this. Note: it checks for whether the node is still attached to the DOM rather than whether it exists because the standard way to delete a node just detaches it from the DOM and relies on garbage collection to delete it from memory. --- app/assets/javascripts/updateContent.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index b73defac9..b53fe9ebf 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -26,7 +26,11 @@ }, replace: function () { this.classNames.forEach((className, index) => { - this.$els[index].addClass(className); + var $el = this.$els[index]; + + if (global.document.body.contains($el.get(0))) { + $el.addClass(className); + } }); // remove references to elements From 51594e7d461922bd6a7d2d7828d641c1a5613e2a Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 11 Feb 2022 15:10:23 +0000 Subject: [PATCH 05/16] Add test for changes to component HTML on start The updateContent JS was changed in this commit so the replacement of the original HTML (with GOVUK modules data-attributes) was moved into the start method rather than being a slightly odd side effect of the render function diffing: https://github.com/alphagov/notifications-admin/pull/4155/commits/476ed1593cb1e438dc7b7d2a509c54c6f3fef8ff This adds a test to make it more clear this happens, as requested in this comment: https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804689618 --- tests/javascripts/updateContent.test.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js index aff63d767..6923105a9 100644 --- a/tests/javascripts/updateContent.test.js +++ b/tests/javascripts/updateContent.test.js @@ -94,7 +94,7 @@ describe('Update content', () => {
`; - initialHTMLString = `
+ initialHTMLString = `
${HTMLString}
`; @@ -105,6 +105,15 @@ describe('Update content', () => { }); + test("It should replace the original HTML with that of the partial, to match that returned from AJAX responses", () => { + + // start the module + window.GOVUK.modules.start(); + + expect(document.querySelector('.ajax-block-container').parentNode.hasAttribute('data-resource')).toBe(false); + + }); + test("It should make requests to the URL specified in the data-resource attribute", () => { // start the module @@ -302,7 +311,7 @@ describe('Update content', () => { } ]); - initialHTMLString = `
+ initialHTMLString = `
${HTMLString}
`; @@ -336,7 +345,7 @@ describe('Update content', () => { } ]); - initialHTMLString = `
+ initialHTMLString = `
${HTMLString}
`; @@ -401,7 +410,7 @@ describe('Update content', () => { } ]); - initialHTMLString = `
+ initialHTMLString = `
${HTMLString}
`; @@ -447,7 +456,7 @@ describe('Update content', () => { } ]); - initialHTMLString = `
+ initialHTMLString = `
${HTMLString}
`; From 4f59396a0017e70168ef3aaf6a033d24c2df2252 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 11 Feb 2022 15:35:04 +0000 Subject: [PATCH 06/16] Change classesToPersist to classesPersister Makes it sounds less like a list and more like a thing that makes classes persist, as noted in: https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804638407 --- app/assets/javascripts/updateContent.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index b53fe9ebf..9343dd7f9 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -13,7 +13,7 @@ // Methods to ensure the DOM fragment is clean of classes added by JS before diffing // and that they are replaced afterwards. - var classesToPersist = { + var classesPersister = { classNames: [], $els: [], remove: function () { @@ -39,12 +39,12 @@ }; var getRenderer = ($contents, key) => response => { - classesToPersist.remove(); + classesPersister.remove(); morphdom( $contents.get(0), $(response[key]).get(0) ); - classesToPersist.replace(); + classesPersister.replace(); }; var getQueue = resource => ( @@ -101,7 +101,7 @@ if ($contents.data('classesToPersist') !== undefined) { $contents.data('classesToPersist') .split(' ') - .forEach(className => classesToPersist.classNames.push(className)); + .forEach(className => classesPersister.classNames.push(className)); } setTimeout( From 9646e17b033b5107697cb578cff23bd88a527aab Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 11 Feb 2022 15:40:47 +0000 Subject: [PATCH 07/16] Make classNames used by classesPersister private Both the remove and replace methods rely on it having parity with the $els array property so it is a good idea to stop it, and $els, being changable by other code. As noted in: https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804639058 --- app/assets/javascripts/updateContent.js | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index 9343dd7f9..1dee22e37 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -14,19 +14,22 @@ // Methods to ensure the DOM fragment is clean of classes added by JS before diffing // and that they are replaced afterwards. var classesPersister = { - classNames: [], - $els: [], + _classNames: [], + _$els: [], + addClassName: function (className) { + this._classNames.push(className); + }, remove: function () { - this.classNames.forEach(className => { + this._classNames.forEach(className => { var $elsWithClassName = $('.' + className).removeClass(className); // store elements for that className at the same index - this.$els.push($elsWithClassName); + this._$els.push($elsWithClassName); }); }, replace: function () { - this.classNames.forEach((className, index) => { - var $el = this.$els[index]; + this._classNames.forEach((className, index) => { + var $el = this._$els[index]; if (global.document.body.contains($el.get(0))) { $el.addClass(className); @@ -101,7 +104,7 @@ if ($contents.data('classesToPersist') !== undefined) { $contents.data('classesToPersist') .split(' ') - .forEach(className => classesPersister.classNames.push(className)); + .forEach(className => classesPersister.addClassName(className)); } setTimeout( From e310ff34696d0f280bbfd45355eed50bc8650cff Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 11 Feb 2022 16:36:49 +0000 Subject: [PATCH 08/16] Quick fixes for updateContent tests To address issues in these comments: - https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804641078 - https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804642005 --- tests/javascripts/updateContent.test.js | 27 ++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js index 6923105a9..d33c730b5 100644 --- a/tests/javascripts/updateContent.test.js +++ b/tests/javascripts/updateContent.test.js @@ -317,7 +317,7 @@ describe('Update content', () => { document.body.innerHTML = initialHTMLString; - // make the response have an extra item + // make a response with no changes responseObj[updateKey] = HTMLString; // start the module @@ -468,8 +468,29 @@ describe('Update content', () => { // Add class to indicate focus state of link on parent heading document.querySelectorAll('.file-list h2')[0].classList.add('js-child-has-focus'); - // make the response match the initial HTML to emulate a response with no changes - responseObj[updateKey] = HTMLString; + var updatedHTMLString = getHTMLString([ + { + title: "Gas leak", + hint: "There's a gas leak in the local area. Residents should vacate until further notice.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi B", + "Santa Claus Village, Rovaniemi C" + ] + }, + { + title: "Reservoir flooding template", + hint: "The local reservoir has flooded. All people within 5 miles should move to a safer location.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi A", + "Santa Claus Village, Rovaniemi D" + ] + } + ]); + + // make the response have an extra item + responseObj[updateKey] = updatedHTMLString; // start the module window.GOVUK.modules.start(); From 467be3a3394f7e46e6740c2fbb7061c7abeddc17 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 15 Feb 2022 11:05:28 +0000 Subject: [PATCH 09/16] Fixes for updateContent based on PR comments Includes the following: Guard for adding duplicate classNames Stops the code that allows classNames to persist across updates to the component HTML from adding a className multiple times to the list of those to persist. From this comment on the associated pull request: https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804639058 Add comment explaining guard for operations on elements no longer in the DOM The value of this guard can be unclear and why it is needed so we add a comment to explain this. From this comment on the associated pull request: https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804697189 --- app/assets/javascripts/updateContent.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index 1dee22e37..c21287bfc 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -17,7 +17,9 @@ _classNames: [], _$els: [], addClassName: function (className) { - this._classNames.push(className); + if (this._classNames.indexOf(className) === -1) { + this._classNames.push(className); + } }, remove: function () { this._classNames.forEach(className => { @@ -31,6 +33,8 @@ this._classNames.forEach((className, index) => { var $el = this._$els[index]; + // Avoid updating elements that are no longer present. + // elements removed will still exist in memory but won't be attached to the DOM any more if (global.document.body.contains($el.get(0))) { $el.addClass(className); } From 776f210e7b64e088d7ffaa8527c9d87c67aab9b8 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 15 Feb 2022 11:26:17 +0000 Subject: [PATCH 10/16] Add comments about deleting this code in future If the :has pseudo-class becomes available in the browsers we support in future, the code this branch/pull request adds will be redundant as its behaviour will be possible only using CSS. This adds comments to note this to the parts of the code you'd need to remove. --- app/assets/javascripts/main.js | 4 ++++ app/assets/javascripts/updateContent.js | 14 +++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index 2d056032e..fb7869922 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -25,6 +25,10 @@ $(() => $('.govuk-header__container').on('click', function() { $(this).css('border-color', '#005ea5'); })); +// Applies our expanded focus style to the siblings of links when that link is wrapped in a heading. +// +// This will be possible in CSS in the future, using the :has pseudo-class. When :has is available +// in the browsers we support, this code can be replaced with a CSS-only solution. $('.js-mark-focus-on-parent').on('focus blur', '*', e => { $target = $(e.target); if (e.type === 'focusin') { diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index c21287bfc..5c4f8fa6d 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -13,6 +13,10 @@ // Methods to ensure the DOM fragment is clean of classes added by JS before diffing // and that they are replaced afterwards. + // + // Added to allow the use of JS, in main.js, to apply styles which in future could be + // achieved with the :has pseudo-class. If :has is available in our supported browsers, + // this can be removed in favour of a CSS-only solution. var classesPersister = { _classNames: [], _$els: [], @@ -100,11 +104,15 @@ var resource = $component.data('resource'); var form = $component.data('form'); - // replace component with contents - // the renderer does this anyway when diffing against the first response + // Replace component with contents. + // The renderer does this anyway when diffing against the first response $component.replaceWith($contents); - // store any classes that should persist through updates + // Store any classes that should persist through updates + // + // Added to allow the use of JS, in main.js, to apply styles which in future could be + // achieved with the :has pseudo-class. If :has is available in our supported browsers, + // this can be removed in favour of a CSS-only solution. if ($contents.data('classesToPersist') !== undefined) { $contents.data('classesToPersist') .split(' ') From 1d69f7ab6104e3978d84ff63657f6e130d85ca59 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 15 Feb 2022 13:02:39 +0000 Subject: [PATCH 11/16] Fix focus style gap at top in keyline-block File-list items contained by div.keyline-block's are pushed down 3px more than elsewhere which creates a gap between the top of the focus style and the keyline above. This extends the part of the focus style made up by a box-shadow by 3px so it covers the gap. --- app/assets/stylesheets/views/dashboard.scss | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index 82455a8e5..a63d728ef 100644 --- a/app/assets/stylesheets/views/dashboard.scss +++ b/app/assets/stylesheets/views/dashboard.scss @@ -120,6 +120,11 @@ &.file-list-filename-large { /* override box-shadow to push underline down a bit */ box-shadow: 0 -2px $govuk-focus-colour, 0 5px $govuk-focus-text-colour; + + // File-list items contained by keyline-blocks have more spacing at the top so adapt to cover it + .keyline-block > .file-list & { + box-shadow: 0 -5px $govuk-focus-colour, 0 5px $govuk-focus-text-colour; + } } & + .file-list-hint, From 8c253309e4870d63cd5f76a48afb956071a2c5b7 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 15 Feb 2022 16:35:45 +0000 Subject: [PATCH 12/16] Fix error in updateContent code This was missed from these changes: https://github.com/alphagov/notifications-admin/pull/4155/commits/9646e17b033b5107697cb578cff23bd88a527aab --- app/assets/javascripts/updateContent.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index 5c4f8fa6d..8931f285e 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -45,7 +45,7 @@ }); // remove references to elements - this.$els = []; + this._$els = []; } }; From 41ee340b45ac8f1c6385c8d227d4570984b211b5 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 16 Feb 2022 11:25:50 +0000 Subject: [PATCH 13/16] Rewrite updateContent tests --- tests/javascripts/updateContent.test.js | 448 +++++++++++------------- 1 file changed, 199 insertions(+), 249 deletions(-) diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js index d33c730b5..7db5ebacc 100644 --- a/tests/javascripts/updateContent.test.js +++ b/tests/javascripts/updateContent.test.js @@ -47,108 +47,20 @@ afterAll(() => { describe('Update content', () => { - let HTMLString; - let initialHTMLString; + const getInitialHTMLString = partial => ` +
+ ${partial} +
`; - describe('When updating the contents of DOM nodes', () => { + describe("All variations", () => { beforeEach(() => { - // store HTML in string to allow use in AJAX responses - HTMLString = ` -
- -
`; - - - initialHTMLString = `
- ${HTMLString} -
`; - - document.body.innerHTML = initialHTMLString; + // Intentionally basic example because we're not testing changes to the partial + document.body.innerHTML = getInitialHTMLString(`

Sending

`); // default the response to match the content inside div[data-module] - responseObj[updateKey] = HTMLString; - - }); - - test("It should replace the original HTML with that of the partial, to match that returned from AJAX responses", () => { - - // start the module - window.GOVUK.modules.start(); - - expect(document.querySelector('.ajax-block-container').parentNode.hasAttribute('data-resource')).toBe(false); - - }); - - test("It should make requests to the URL specified in the data-resource attribute", () => { - - // start the module - window.GOVUK.modules.start(); - jest.advanceTimersByTime(2000); - - expect($.ajax.mock.calls[0][0]).toEqual(resourceURL); - - }); - - test("If the response contains no changes, the DOM should stay the same", () => { - - // send the done callback a response with updates included - responseObj[updateKey] = HTMLString; - - // start the module - window.GOVUK.modules.start(); - jest.advanceTimersByTime(2000); - - // check a sample DOM node is unchanged - expect(document.querySelectorAll('.big-number-number')[0].textContent.trim()).toEqual("0"); - - }); - - test("If the response contains changes, it should update the DOM with them", () => { - - // send the done callback a response with updates included - responseObj[updateKey] = HTMLString.replace(/
0<\/div>{1}/, '
1
'); - - // start the module - window.GOVUK.modules.start(); - jest.advanceTimersByTime(2000); - - // check the right DOM node is updated - expect(document.querySelectorAll('.big-number-number')[0].textContent.trim()).toEqual("1"); + responseObj[updateKey] = `

Sending

`; }); @@ -216,12 +128,14 @@ describe('Update content', () => { beforeEach(() => { + // Add a form to the page document.body.innerHTML += `
`; + // Link the component to the form document.querySelector('[data-module=update-content]').setAttribute('data-form', 'service'); // start the module @@ -250,40 +164,156 @@ describe('Update content', () => { }); - describe("When adding or removing DOM nodes", () => { - var getItemHTMLString = content => { - var areas = ''; + describe('When updating the contents of DOM nodes', () => { - content.areas.forEach(area => - areas += "\n" + `
  • ${area}
    • ` - ); + let partialData; + + const getPartial = items => { + let pillsHTML = ''; + + items.forEach(item => { + pillsHTML += ` +
  • +
    +
    +
    ${item.count}
    +
    +
    ${item.label}
    +
    +
    • `; + }); return ` -
    -
    -

    - ${content.title} -

    -
    -
    - - ${content.hint} - -
    -
    -

    - ${content.status} -

    -
    -
    -
      - ${areas} -
    -
    +
    +
      + ${pillsHTML} +
    `; }; - var getHTMLString = items => { + beforeEach(() => { + + partialData = [ + { + count: 0, + label: 'total', + selected: true + }, + { + count: 0, + label: 'sending', + selected: false + }, + { + count: 0, + label: 'delivered', + selected: false + }, + { + count: 0, + label: 'failed', + selected: false + } + ]; + + document.body.innerHTML = getInitialHTMLString(getPartial(partialData)); + + }); + + test("It should replace the original HTML with that of the partial, to match that returned from AJAX responses", () => { + + // default the response to match the content inside div[data-module] + responseObj[updateKey] = getPartial(partialData); + + // start the module + window.GOVUK.modules.start(); + + expect(document.querySelector('.ajax-block-container').parentNode.hasAttribute('data-resource')).toBe(false); + + }); + + test("It should make requests to the URL specified in the data-resource attribute", () => { + + // default the response to match the content inside div[data-module] + responseObj[updateKey] = getPartial(partialData); + + // start the module + window.GOVUK.modules.start(); + jest.advanceTimersByTime(2000); + + expect($.ajax.mock.calls[0][0]).toEqual(resourceURL); + + }); + + test("If the response contains no changes, the DOM should stay the same", () => { + + // send the done callback a response with updates included + responseObj[updateKey] = getPartial(partialData); + + // start the module + window.GOVUK.modules.start(); + jest.advanceTimersByTime(2000); + + // check a sample DOM node is unchanged + expect(document.querySelectorAll('.big-number-number')[0].textContent.trim()).toEqual("0"); + + }); + + test("If the response contains changes, it should update the DOM with them", () => { + + partialData[0].count = 1; + + // send the done callback a response with updates included + responseObj[updateKey] = getPartial(partialData); + + // start the module + window.GOVUK.modules.start(); + jest.advanceTimersByTime(2000); + + // check the right DOM node is updated + expect(document.querySelectorAll('.big-number-number')[0].textContent.trim()).toEqual("1"); + + }); + + }); + + describe("When adding or removing DOM nodes", () => { + + let partialData; + + const getPartial = items => { + + const getItemHTMLString = content => { + var areas = ''; + + content.areas.forEach(area => + areas += "\n" + `
  • ${area}
    • ` + ); + + return ` +
    +
    +

    + ${content.title} +

    +
    +
    + + ${content.hint} + +
    +
    +

    + ${content.status} +

    +
    +
    +
      + ${areas} +
    +
    +
    `; + }; var itemsHTMLString = ''; @@ -296,10 +326,9 @@ describe('Update content', () => { }; - test("If the response contains no changes, the DOM should stay the same", () => { + beforeEach(() => { - // store HTML in string to allow use in AJAX responses - HTMLString = getHTMLString([ + partialData = [ { title: "Gas leak", hint: "There's a gas leak in the local area. Residents should vacate until further notice.", @@ -309,16 +338,16 @@ describe('Update content', () => { "Santa Claus Village, Rovaniemi C" ] } - ]); + ]; - initialHTMLString = `
    - ${HTMLString} -
    `; + }); - document.body.innerHTML = initialHTMLString; + test("If the response contains no changes, the DOM should stay the same", () => { + + document.body.innerHTML = getInitialHTMLString(getPartial(partialData)); // make a response with no changes - responseObj[updateKey] = HTMLString; + responseObj[updateKey] = getPartial(partialData); // start the module window.GOVUK.modules.start(); @@ -332,48 +361,20 @@ describe('Update content', () => { test("If the response adds a node, the DOM should contain that node", () => { - // store HTML in string to allow use in AJAX responses - HTMLString = getHTMLString([ - { - title: "Gas leak", - hint: "There's a gas leak in the local area. Residents should vacate until further notice.", - status: "Waiting for approval", - areas: [ - "Santa Claus Village, Rovaniemi B", - "Santa Claus Village, Rovaniemi C" - ] - } - ]); + document.body.innerHTML = getInitialHTMLString(getPartial(partialData)); - initialHTMLString = `
    - ${HTMLString} -
    `; - - document.body.innerHTML = initialHTMLString; - - var updatedHTMLString = getHTMLString([ - { - title: "Gas leak", - hint: "There's a gas leak in the local area. Residents should vacate until further notice.", - status: "Waiting for approval", - areas: [ - "Santa Claus Village, Rovaniemi B", - "Santa Claus Village, Rovaniemi C" - ] - }, - { - title: "Reservoir flooding template", - hint: "The local reservoir has flooded. All people within 5 miles should move to a safer location.", - status: "Waiting for approval", - areas: [ - "Santa Claus Village, Rovaniemi A", - "Santa Claus Village, Rovaniemi D" - ] - } - ]); + partialData.push({ + title: "Reservoir flooding template", + hint: "The local reservoir has flooded. All people within 5 miles should move to a safer location.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi A", + "Santa Claus Village, Rovaniemi D" + ] + }); // make the response have an extra item - responseObj[updateKey] = updatedHTMLString; + responseObj[updateKey] = getPartial(partialData); // start the module window.GOVUK.modules.start(); @@ -388,48 +389,24 @@ describe('Update content', () => { test("If the response removes a node, the DOM should not contain that node", () => { - // store HTML in string to allow use in AJAX responses - HTMLString = getHTMLString([ - { - title: "Gas leak", - hint: "There's a gas leak in the local area. Residents should vacate until further notice.", - status: "Waiting for approval", - areas: [ - "Santa Claus Village, Rovaniemi B", - "Santa Claus Village, Rovaniemi C" - ] - }, - { - title: "Reservoir flooding template", - hint: "The local reservoir has flooded. All people within 5 miles should move to a safer location.", - status: "Waiting for approval", - areas: [ - "Santa Claus Village, Rovaniemi A", - "Santa Claus Village, Rovaniemi D" - ] - } - ]); + // add another item so we start with 2 + partialData.push({ + title: "Reservoir flooding template", + hint: "The local reservoir has flooded. All people within 5 miles should move to a safer location.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi A", + "Santa Claus Village, Rovaniemi D" + ] + }); - initialHTMLString = `
    - ${HTMLString} -
    `; + document.body.innerHTML = getInitialHTMLString(getPartial(partialData)); - document.body.innerHTML = initialHTMLString; - - var updatedHTMLString = getHTMLString([ - { - title: "Gas leak", - hint: "There's a gas leak in the local area. Residents should vacate until further notice.", - status: "Waiting for approval", - areas: [ - "Santa Claus Village, Rovaniemi B", - "Santa Claus Village, Rovaniemi C" - ] - } - ]); + // remove the last item + partialData.pop(); // default the response to match the content inside div[data-module] - responseObj[updateKey] = updatedHTMLString; + responseObj[updateKey] = getPartial(partialData); // start the module window.GOVUK.modules.start(); @@ -443,24 +420,7 @@ describe('Update content', () => { test("If other scripts have added classes to the DOM, they should persist through updates", () => { - // store HTML in string to allow use in AJAX responses - HTMLString = getHTMLString([ - { - title: "Gas leak", - hint: "There's a gas leak in the local area. Residents should vacate until further notice.", - status: "Waiting for approval", - areas: [ - "Santa Claus Village, Rovaniemi B", - "Santa Claus Village, Rovaniemi C" - ] - } - ]); - - initialHTMLString = `
    - ${HTMLString} -
    `; - - document.body.innerHTML = initialHTMLString; + document.body.innerHTML = getInitialHTMLString(getPartial(partialData)); // mark classes to persist on the partial document.querySelector('.ajax-block-container').setAttribute('data-classes-to-persist', 'js-child-has-focus'); @@ -468,29 +428,19 @@ describe('Update content', () => { // Add class to indicate focus state of link on parent heading document.querySelectorAll('.file-list h2')[0].classList.add('js-child-has-focus'); - var updatedHTMLString = getHTMLString([ - { - title: "Gas leak", - hint: "There's a gas leak in the local area. Residents should vacate until further notice.", - status: "Waiting for approval", - areas: [ - "Santa Claus Village, Rovaniemi B", - "Santa Claus Village, Rovaniemi C" - ] - }, - { - title: "Reservoir flooding template", - hint: "The local reservoir has flooded. All people within 5 miles should move to a safer location.", - status: "Waiting for approval", - areas: [ - "Santa Claus Village, Rovaniemi A", - "Santa Claus Village, Rovaniemi D" - ] - } - ]); + // Add an item to trigger an update + partialData.push({ + title: "Reservoir flooding template", + hint: "The local reservoir has flooded. All people within 5 miles should move to a safer location.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi A", + "Santa Claus Village, Rovaniemi D" + ] + }); // make the response have an extra item - responseObj[updateKey] = updatedHTMLString; + responseObj[updateKey] = getPartial(partialData); // start the module window.GOVUK.modules.start(); From 3a86bd1685939f759a3ee64535135469ab0b45b3 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 16 Feb 2022 15:56:52 +0000 Subject: [PATCH 14/16] Change internals of classesPersister The assumption that the classes you want to persist will always have parity with the elements that have those classes, at that point, won't always be true. Because of that, this changes the way elements with those classes are stored, to be in a map between classes and the elements with them (at that point). Also includes an extra test for a scenario where more than one updating component is in the page with classes that need to persist through updates. --- app/assets/javascripts/updateContent.js | 30 ++++++++++------- tests/javascripts/updateContent.test.js | 44 ++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index 8931f285e..9ef216099 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -19,33 +19,39 @@ // this can be removed in favour of a CSS-only solution. var classesPersister = { _classNames: [], - _$els: [], + _classesTo$ElsMap: {}, addClassName: function (className) { if (this._classNames.indexOf(className) === -1) { this._classNames.push(className); } }, remove: function () { + // Store references to any elements with class names to persist this._classNames.forEach(className => { var $elsWithClassName = $('.' + className).removeClass(className); - // store elements for that className at the same index - this._$els.push($elsWithClassName); + if ($elsWithClassName.length > 0) { + this._classesTo$ElsMap[className] = $elsWithClassName; + } }); }, replace: function () { - this._classNames.forEach((className, index) => { - var $el = this._$els[index]; + var className; - // Avoid updating elements that are no longer present. - // elements removed will still exist in memory but won't be attached to the DOM any more - if (global.document.body.contains($el.get(0))) { - $el.addClass(className); - } - }); + for (className in this._classesTo$ElsMap) { + this._classesTo$ElsMap[className].each((idx, el) => { + + // Avoid updating elements that are no longer present. + // elements removed will still exist in memory but won't be attached to the DOM any more + if (global.document.body.contains(el)) { + $(el).addClass(className); + } + + }); + } // remove references to elements - this._$els = []; + this._classesTo$ElsMap = {}; } }; diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js index 7db5ebacc..7c5492d7e 100644 --- a/tests/javascripts/updateContent.test.js +++ b/tests/javascripts/updateContent.test.js @@ -418,7 +418,7 @@ describe('Update content', () => { }); - test("If other scripts have added classes to the DOM, they should persist through updates", () => { + test("If other scripts have added classes to the DOM, they should persist through updates to a single component", () => { document.body.innerHTML = getInitialHTMLString(getPartial(partialData)); @@ -451,6 +451,48 @@ describe('Update content', () => { }); + test("If other scripts have added classes to the DOM, they should persist through updates to multiple components", () => { + + // Create duplicate components in the page + document.body.innerHTML = getInitialHTMLString(getPartial(partialData)) + "\n" + getInitialHTMLString(getPartial(partialData)); + + var partialsInPage = document.querySelectorAll('.ajax-block-container'); + + // Mark classes to persist on the partials (2nd is made up) + partialsInPage[0].setAttribute('data-classes-to-persist', 'js-child-has-focus'); + partialsInPage[1].setAttribute('data-classes-to-persist', 'js-2nd-child-has-focus'); + + // Add examples of those classes on each partial (2nd is made up) + partialsInPage[0].querySelectorAll('.file-list h2')[0].classList.add('js-child-has-focus'); + partialsInPage[1].querySelectorAll('.file-list h2')[0].classList.add('js-2nd-child-has-focus'); + + // Add an item to trigger an update + partialData.push({ + title: "Reservoir flooding template", + hint: "The local reservoir has flooded. All people within 5 miles should move to a safer location.", + status: "Waiting for approval", + areas: [ + "Santa Claus Village, Rovaniemi A", + "Santa Claus Village, Rovaniemi D" + ] + }); + + // make all responses have an extra item + responseObj[updateKey] = getPartial(partialData); + + // start the module + window.GOVUK.modules.start(); + jest.advanceTimersByTime(2000); + + // re-select in case nodes in partialsInPage have changed + partialsInPage = document.querySelectorAll('.ajax-block-container'); + + // check the classes are still there + expect(partialsInPage[0].querySelectorAll('.file-list h2')[0].classList.contains('js-child-has-focus')).toBe(true); + expect(partialsInPage[1].querySelectorAll('.file-list h2')[0].classList.contains('js-2nd-child-has-focus')).toBe(true); + + }); + }); afterEach(() => { From 85e14d302c655a4465a79c53d57aceceb12fb940 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 16 Feb 2022 16:08:48 +0000 Subject: [PATCH 15/16] Make a classesPersister per component instance It makes more sense for something that operates on a single component to be local to it. --- app/assets/javascripts/updateContent.js | 76 +++++++++++++------------ 1 file changed, 39 insertions(+), 37 deletions(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index 9ef216099..b08bceaac 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -17,45 +17,46 @@ // Added to allow the use of JS, in main.js, to apply styles which in future could be // achieved with the :has pseudo-class. If :has is available in our supported browsers, // this can be removed in favour of a CSS-only solution. - var classesPersister = { - _classNames: [], - _classesTo$ElsMap: {}, - addClassName: function (className) { - if (this._classNames.indexOf(className) === -1) { - this._classNames.push(className); - } - }, - remove: function () { - // Store references to any elements with class names to persist - this._classNames.forEach(className => { - var $elsWithClassName = $('.' + className).removeClass(className); - - if ($elsWithClassName.length > 0) { - this._classesTo$ElsMap[className] = $elsWithClassName; - } - }); - }, - replace: function () { - var className; - - for (className in this._classesTo$ElsMap) { - this._classesTo$ElsMap[className].each((idx, el) => { - - // Avoid updating elements that are no longer present. - // elements removed will still exist in memory but won't be attached to the DOM any more - if (global.document.body.contains(el)) { - $(el).addClass(className); - } - - }); - } - - // remove references to elements - this._classesTo$ElsMap = {}; + var ClassesPersister = function ($contents) { + this._$contents = $contents; + this._classNames = []; + this._classesTo$ElsMap = {}; + }; + ClassesPersister.prototype.addClassName = function (className) { + if (this._classNames.indexOf(className) === -1) { + this._classNames.push(className); } }; + ClassesPersister.prototype.remove = function () { + // Store references to any elements with class names to persist + this._classNames.forEach(className => { + var $elsWithClassName = $('.' + className, this._$contents).removeClass(className); - var getRenderer = ($contents, key) => response => { + if ($elsWithClassName.length > 0) { + this._classesTo$ElsMap[className] = $elsWithClassName; + } + }); + }; + ClassesPersister.prototype.replace = function () { + var className; + + for (className in this._classesTo$ElsMap) { + this._classesTo$ElsMap[className].each((idx, el) => { + + // Avoid updating elements that are no longer present. + // elements removed will still exist in memory but won't be attached to the DOM any more + if (global.document.body.contains(el)) { + $(el).addClass(className); + } + + }); + } + + // remove references to elements + this._classesTo$ElsMap = {}; + }; + + var getRenderer = ($contents, key, classesPersister) => response => { classesPersister.remove(); morphdom( $contents.get(0), @@ -109,6 +110,7 @@ var key = $component.data('key'); var resource = $component.data('resource'); var form = $component.data('form'); + var classesPersister = new ClassesPersister($contents); // Replace component with contents. // The renderer does this anyway when diffing against the first response @@ -127,7 +129,7 @@ setTimeout( () => poll( - getRenderer($contents, key), + getRenderer($contents, key, classesPersister), resource, getQueue(resource), form From 8521d1e45f43cdbe7a07e0e988ddb6b1f7ac868a Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 18 Feb 2022 11:55:13 +0000 Subject: [PATCH 16/16] Add assertions against stray classes In previous iterations of the classPersister, we found issues with the implementation meant classes it should have added back to elements were also added to other elements. This adds tests for this scenario to ensure it doesn't happen again. Also includes changes to fix a linting error with the JS which complained about a function being defined in a loop while referencing variables in the outer scope. --- app/assets/javascripts/updateContent.js | 19 ++++++++++--------- tests/javascripts/updateContent.test.js | 4 ++++ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index b08bceaac..96e5fef33 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -38,18 +38,19 @@ }); }; ClassesPersister.prototype.replace = function () { + var replaceClasses = (idx, el) => { + + // Avoid updating elements that are no longer present. + // elements removed will still exist in memory but won't be attached to the DOM any more + if (global.document.body.contains(el)) { + $(el).addClass(className); + } + + }; var className; for (className in this._classesTo$ElsMap) { - this._classesTo$ElsMap[className].each((idx, el) => { - - // Avoid updating elements that are no longer present. - // elements removed will still exist in memory but won't be attached to the DOM any more - if (global.document.body.contains(el)) { - $(el).addClass(className); - } - - }); + this._classesTo$ElsMap[className].each(replaceClasses); } // remove references to elements diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js index 7c5492d7e..557f1799e 100644 --- a/tests/javascripts/updateContent.test.js +++ b/tests/javascripts/updateContent.test.js @@ -491,6 +491,10 @@ describe('Update content', () => { expect(partialsInPage[0].querySelectorAll('.file-list h2')[0].classList.contains('js-child-has-focus')).toBe(true); expect(partialsInPage[1].querySelectorAll('.file-list h2')[0].classList.contains('js-2nd-child-has-focus')).toBe(true); + // check each heading only has the classes assigned to it before updates occurred + expect(partialsInPage[0].querySelectorAll('.file-list h2')[0].classList.contains('js-2nd-child-has-focus')).toBe(false); + expect(partialsInPage[1].querySelectorAll('.file-list h2')[0].classList.contains('js-child-has-focus')).toBe(false); + }); });