diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index a6cb2ccba..fb7869922 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -24,3 +24,16 @@ $(() => $('.banner-dangerous').eq(0).trigger('focus')); $(() => $('.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') { + $target.parent().addClass('js-child-has-focus'); + } else { + $target.parent().removeClass('js-child-has-focus'); + } +}); diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index 2e4bc4758..96e5fef33 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -11,10 +11,60 @@ 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. + // + // 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 = 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); + + if ($elsWithClassName.length > 0) { + this._classesTo$ElsMap[className] = $elsWithClassName; + } + }); + }; + 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(replaceClasses); + } + + // remove references to elements + this._classesTo$ElsMap = {}; + }; + + var getRenderer = ($contents, key, classesPersister) => response => { + classesPersister.remove(); + morphdom( + $contents.get(0), + $(response[key]).get(0) + ); + classesPersister.replace(); + }; var getQueue = resource => ( queues[resource] = queues[resource] || [] @@ -55,15 +105,39 @@ 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); + var $contents = $component.children().eq(0); + 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 + $component.replaceWith($contents); + + // 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(' ') + .forEach(className => classesPersister.addClassName(className)); + } + + setTimeout( + () => poll( + getRenderer($contents, key, classesPersister), + resource, + getQueue(resource), + form + ), + defaultInterval + ); + }; }; diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index f97c98c39..a63d728ef 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,18 +115,16 @@ /* 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 { /* 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, @@ -131,6 +134,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/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 }} 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 %}
diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js index d965906b8..557f1799e 100644 --- a/tests/javascripts/updateContent.test.js +++ b/tests/javascripts/updateContent.test.js @@ -47,59 +47,455 @@ afterAll(() => { describe('Update content', () => { - let HTMLString; - let initialHTMLString; + const getInitialHTMLString = partial => ` +
+ ${partial} +
`; - beforeEach(() => { + describe("All variations", () => { - // store HTML in string to allow use in AJAX responses - HTMLString = ` -
- -
`; +
    + ${areas} +
+
+
`; + }; + var itemsHTMLString = ''; - initialHTMLString = `
- ${HTMLString} -
`; + items.forEach(item => itemsHTMLString += "\n" + getItemHTMLString(item)); - document.body.innerHTML = initialHTMLString; + return `
+ ${itemsHTMLString}; +
+
`; - // default the response to match the content inside div[data-module] - responseObj[updateKey] = HTMLString; + }; + + beforeEach(() => { + + partialData = [ + { + 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" + ] + } + ]; + + }); + + 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] = getPartial(partialData); + + // 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", () => { + + document.body.innerHTML = getInitialHTMLString(getPartial(partialData)); + + 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] = getPartial(partialData); + + // 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", () => { + + // 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" + ] + }); + + document.body.innerHTML = getInitialHTMLString(getPartial(partialData)); + + // remove the last item + partialData.pop(); + + // 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); + + // 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 to a single component", () => { + + 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'); + + // Add class to indicate focus state of link on parent heading + document.querySelectorAll('.file-list h2')[0].classList.add('js-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 the response have an extra item + responseObj[updateKey] = getPartial(partialData); + + // 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); + + }); + + 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); + + // 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); + + }); }); @@ -115,138 +511,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] - ])); - - }) - - }); - });