From 956f5d4c3ec5aeacaf20f6cbadea8e1812e1f3a9 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 15 Sep 2020 12:53:58 +0100 Subject: [PATCH 1/2] Make copy to clipboard work with prefixes The prefix was being included in the selection copied. --- app/assets/javascripts/apiKey.js | 20 +++++++++- tests/javascripts/apiKey.test.js | 39 ++++++++++++++++--- .../support/helpers/dom_interfaces.js | 2 +- 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/apiKey.js b/app/assets/javascripts/apiKey.js index 42e8c6cd4..cdd7aaf0e 100644 --- a/app/assets/javascripts/apiKey.js +++ b/app/assets/javascripts/apiKey.js @@ -27,11 +27,27 @@ ` }; + this.getRangeFromElement = function (keyElement) { + const range = document.createRange(); + let prefixIndex = -1; + + Array.from(keyElement.childNodes).forEach((el, idx) => { + if ((el.nodeType === 1) && el.classList.contains('govuk-visually-hidden')) { + prefixIndex = idx; + } + }); + + range.selectNodeContents(keyElement); + if (prefixIndex !== -1) { range.setStart(keyElement, prefixIndex + 1); } + + return range; + }; + this.copyKey = function(keyElement, callback) { var selection = window.getSelection ? window.getSelection() : document.selection, - range = document.createRange(); + range = this.getRangeFromElement(keyElement); + selection.removeAllRanges(); - range.selectNodeContents(keyElement); selection.addRange(range); document.execCommand('copy'); selection.removeAllRanges(); diff --git a/tests/javascripts/apiKey.test.js b/tests/javascripts/apiKey.test.js index c801bcd7e..8aa20c7f0 100644 --- a/tests/javascripts/apiKey.test.js +++ b/tests/javascripts/apiKey.test.js @@ -33,16 +33,12 @@ describe('API key', () => { }; - beforeAll(() => { + beforeEach(() => { // mock objects used to manipulate the page selection selectionMock = new helpers.SelectionMock(jest); rangeMock = new helpers.RangeMock(jest); - }); - - beforeEach(() => { - // plug gaps in JSDOM's API for manipulation of selections window.getSelection = jest.fn(() => selectionMock); document.createRange = jest.fn(() => rangeMock); @@ -325,7 +321,7 @@ describe('API key', () => { describe("If it's one of many in the page", () => { - test("the button should have a hidden suffix naming the id it is for", () => { + beforeEach(() => { // If 'thing' (what the id is) and 'name' (its specific idenifier on the page) are // different, it will be one of others called the same 'thing'. @@ -338,12 +334,31 @@ describe('API key', () => { helpers.triggerEvent(component.querySelector('button'), 'click'); + }); + + test("the button should have a hidden suffix naming the id it is for", () => { + const buttonSuffix = component.querySelector('button .govuk-visually-hidden'); expect(buttonSuffix).not.toBeNull(); expect(buttonSuffix.textContent).toEqual(' for Default'); }); + test("the copied selection shouldn't include the prefix of the id", () => { + + // that selection (a range) should have a startOffset past the first two nodes: + // index 0: text node containing the whitespace before the prefix + // index 1: the prefix node + expect(rangeMock.setStart).toHaveBeenCalled(); + expect(rangeMock.setStart.mock.calls[0][1]).toEqual(2); + + // reset any methods in the global space + window.queryCommandSupported = undefined; + window.getSelection = undefined; + document.createRange = undefined; + + }); + }); describe("If it's the only one on the page", () => { @@ -373,6 +388,18 @@ describe('API key', () => { }) + test("the copied selection should match the id", () => { + + // that selection (a range) shouldn't call setStart to avoid the prefix: + expect(rangeMock.setStart).not.toHaveBeenCalled(); + + // reset any methods in the global space + window.queryCommandSupported = undefined; + window.getSelection = undefined; + document.createRange = undefined; + + }); + }); }); diff --git a/tests/javascripts/support/helpers/dom_interfaces.js b/tests/javascripts/support/helpers/dom_interfaces.js index 903a1da42..30596a0a3 100644 --- a/tests/javascripts/support/helpers/dom_interfaces.js +++ b/tests/javascripts/support/helpers/dom_interfaces.js @@ -30,7 +30,7 @@ class DOMInterfaceMock { class RangeMock extends DOMInterfaceMock { constructor (jest) { - super(jest, { props: [], methods: ['selectNodeContents'] }); + super(jest, { props: [], methods: ['selectNodeContents', 'setStart'] }); } } From b4d985e0712c2111438bd1f139ed7e8372123403 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 15 Sep 2020 17:07:01 +0100 Subject: [PATCH 2/2] Rewrite test descriptions They originals didn't explain what the tests do. We could test the contents of the range but that would be testing the Range API, rather than our use of it. The tests test how we use that API for these scenarios so their descriptions should say this. --- tests/javascripts/apiKey.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/javascripts/apiKey.test.js b/tests/javascripts/apiKey.test.js index 8aa20c7f0..08f974928 100644 --- a/tests/javascripts/apiKey.test.js +++ b/tests/javascripts/apiKey.test.js @@ -344,7 +344,7 @@ describe('API key', () => { }); - test("the copied selection shouldn't include the prefix of the id", () => { + test("the copied selection (range) should start after the prefix of the id", () => { // that selection (a range) should have a startOffset past the first two nodes: // index 0: text node containing the whitespace before the prefix @@ -388,7 +388,7 @@ describe('API key', () => { }) - test("the copied selection should match the id", () => { + test("the copied selection (range) should start at the default position", () => { // that selection (a range) shouldn't call setStart to avoid the prefix: expect(rangeMock.setStart).not.toHaveBeenCalled();