From c46dd53254f24824acb8d5a655e461863609ff06 Mon Sep 17 00:00:00 2001 From: Sage Abdullah Date: Fri, 12 Jul 2024 15:59:22 +0100 Subject: [PATCH] Do not swap sessions list HTML until focus has left the container Prevent disrupting the flow of keyboard users when they are focusing on the sessions list. --- client/src/controllers/SwapController.test.js | 324 ++++++++++++++++++ client/src/controllers/SwapController.ts | 78 ++++- .../shared/editing_sessions/module.html | 1 + 3 files changed, 388 insertions(+), 15 deletions(-) diff --git a/client/src/controllers/SwapController.test.js b/client/src/controllers/SwapController.test.js index 8b59867143..d54fa67b54 100644 --- a/client/src/controllers/SwapController.test.js +++ b/client/src/controllers/SwapController.test.js @@ -1643,4 +1643,328 @@ describe('SwapController', () => { ); }); }); + + describe('deferring the content update until the focus is out of the target container', () => { + const getMockResultsWithButtons = (total) => + getMockResults({ total }) + + ``; + + beforeEach(() => { + document.body.innerHTML = /* html */ ` +
+
+ + + +
+
+
+ + +
+
+
+ `; + }); + + it('should wait until the target loses focus and continue execution immediately after', async () => { + const onSuccess = new Promise((resolve) => { + document.addEventListener('w-swap:success', resolve); + }); + + const beginEventHandler = jest.fn(); + document.addEventListener('w-swap:begin', beginEventHandler); + + fetch.mockResponseSuccessText(getMockResultsWithButtons(5)); + + expect(handleError).not.toHaveBeenCalled(); + expect(global.fetch).not.toHaveBeenCalled(); + + document.getElementById('submit').click(); + + // focus on an element inside the target container + document.getElementById('button1').focus(); + + expect(beginEventHandler).not.toHaveBeenCalled(); + + jest.runAllTimers(); // submit is debounced + + // should fire a begin event before the request is made + expect(beginEventHandler).toHaveBeenCalledTimes(1); + expect(beginEventHandler.mock.calls[0][0].detail).toEqual({ + requestUrl: '/path/to/editing-sessions/?title=&type=some-type', + }); + + // visual loading state should be active + await Promise.resolve(); // trigger next rendering + + expect(handleError).not.toHaveBeenCalled(); + expect(global.fetch).toHaveBeenCalledWith( + '/path/to/editing-sessions/?title=&type=some-type', + expect.any(Object), + ); + + // simulate the request completing + await Promise.resolve(); + + // should not update HTML just yet + expect( + document.getElementById('editing-sessions').querySelectorAll('li') + .length, + ).toEqual(0); + + await flushPromises(); + + // should still not update HTML just yet + expect( + document.getElementById('editing-sessions').querySelectorAll('li') + .length, + ).toEqual(0); + + // switch focus to a different element but still inside the target container + document.getElementById('button2').focus(); + + await flushPromises(); + + // should still not update HTML just yet + expect( + document.getElementById('editing-sessions').querySelectorAll('li') + .length, + ).toEqual(0); + + // switch focus to an element outside the target container + document.getElementById('submit').focus(); + + const successEvent = await onSuccess; + + // should dispatch success event + expect(successEvent.detail).toEqual({ + requestUrl: '/path/to/editing-sessions/?title=&type=some-type', + results: expect.any(String), + }); + + // should update HTML + expect( + document.getElementById('editing-sessions').querySelectorAll('li') + .length, + ).toEqual(5); + + await flushPromises(); + + // should NOT update the current URL + // as the reflect-value attribute is not set + expect(window.location.search).toEqual(''); + }); + + it('should not wait until the target loses focus if defer value is set to false', async () => { + // unset the defer value (default is false) + const form = document.querySelector('form'); + form.removeAttribute('data-w-swap-defer-value'); + + const onSuccess = new Promise((resolve) => { + document.addEventListener('w-swap:success', resolve); + }); + + const beginEventHandler = jest.fn(); + document.addEventListener('w-swap:begin', beginEventHandler); + + fetch.mockResponseSuccessText(getMockResultsWithButtons(5)); + + expect(handleError).not.toHaveBeenCalled(); + expect(global.fetch).not.toHaveBeenCalled(); + + document.getElementById('submit').click(); + + // focus on an element inside the target container + document.getElementById('button1').focus(); + + expect(beginEventHandler).not.toHaveBeenCalled(); + + jest.runAllTimers(); // submit is debounced + + // should fire a begin event before the request is made + expect(beginEventHandler).toHaveBeenCalledTimes(1); + expect(beginEventHandler.mock.calls[0][0].detail).toEqual({ + requestUrl: '/path/to/editing-sessions/?title=&type=some-type', + }); + + // visual loading state should be active + await Promise.resolve(); // trigger next rendering + + expect(handleError).not.toHaveBeenCalled(); + expect(global.fetch).toHaveBeenCalledWith( + '/path/to/editing-sessions/?title=&type=some-type', + expect.any(Object), + ); + + // simulate the request completing + await Promise.resolve(); + + // should not update HTML just yet + expect( + document.getElementById('editing-sessions').querySelectorAll('li') + .length, + ).toEqual(0); + + await flushPromises(); + + // if the deferred write happens (which should not), the test will + // time out because the focus is still inside the target container + const successEvent = await onSuccess; + + // should dispatch success event + expect(successEvent.detail).toEqual({ + requestUrl: '/path/to/editing-sessions/?title=&type=some-type', + results: expect.any(String), + }); + + // should update HTML + expect( + document.getElementById('editing-sessions').querySelectorAll('li') + .length, + ).toEqual(5); + + await flushPromises(); + + // should NOT update the current URL + // as the reflect-value attribute is not set + expect(window.location.search).toEqual(''); + }); + + it('should write immediately if there is a deferred write but we no longer need to defer', async () => { + const successEvents = []; + const onSuccess = new Promise((resolve) => { + document.addEventListener('w-swap:success', (event) => { + successEvents.push(event); + resolve(event); + }); + }); + + const beginEventHandler = jest.fn(); + document.addEventListener('w-swap:begin', beginEventHandler); + + fetch.mockResponseSuccessText(getMockResultsWithButtons(5)); + + expect(handleError).not.toHaveBeenCalled(); + expect(global.fetch).not.toHaveBeenCalled(); + + document.getElementById('submit').click(); + + // focus on an element inside the target container + document.getElementById('button1').focus(); + + expect(beginEventHandler).not.toHaveBeenCalled(); + + jest.runAllTimers(); // submit is debounced + + // should fire a begin event before the request is made + expect(beginEventHandler).toHaveBeenCalledTimes(1); + expect(beginEventHandler.mock.calls[0][0].detail).toEqual({ + requestUrl: '/path/to/editing-sessions/?title=&type=some-type', + }); + + // visual loading state should be active + await Promise.resolve(); // trigger next rendering + + expect(handleError).not.toHaveBeenCalled(); + expect(global.fetch).toHaveBeenCalledWith( + '/path/to/editing-sessions/?title=&type=some-type', + expect.any(Object), + ); + + // simulate the request completing + await Promise.resolve(); + + // should not update HTML just yet + expect( + document.getElementById('editing-sessions').querySelectorAll('li') + .length, + ).toEqual(0); + + await flushPromises(); + + // should still not update HTML just yet + expect( + document.getElementById('editing-sessions').querySelectorAll('li') + .length, + ).toEqual(0); + + // instead of switching the focus outside, we set the defer value + // to false, so we can test the case where a new update is not deferred but + // there is a deferred write and the focus is still inside the target container + const form = document.querySelector('form'); + form.setAttribute('data-w-swap-defer-value', 'false'); + + // change the input so we can trigger a new request with a different URL to check + document.querySelector('input[name="title"]').value = 'newvalue'; + + // submit the form again + form.submit(); + fetch.mockResponseSuccessText(getMockResultsWithButtons(8)); + jest.runAllTimers(); + await flushPromises(); + + expect(global.fetch).toHaveBeenCalledWith( + '/path/to/editing-sessions/?title=newvalue&type=some-type', + expect.any(Object), + ); + + const successEvent = await onSuccess; + + // should dispatch success event + expect(successEvent.detail).toEqual({ + requestUrl: '/path/to/editing-sessions/?title=newvalue&type=some-type', + results: expect.any(String), + }); + + // should skip the deferred write and instead write the last request's response (8 items) + expect( + document.getElementById('editing-sessions').querySelectorAll('li') + .length, + ).toEqual(8); + + await flushPromises(); + + // should still use the last request's response instead of running the deferred write + expect( + document.getElementById('editing-sessions').querySelectorAll('li') + .length, + ).toEqual(8); + + // Simulate triggering the unlikely edge case of the focusout event being triggered, + // with no deferred writes left, which theoretically could only happen if the + // defer value was changed to false at midpoint like in this test case. + // We need to regain focus on an element in the target container first, + // because the previous element that was focused is no longer in the DOM. + document.getElementById('button1').focus(); + // then we focus out + document.getElementById('submit').focus(); + + jest.runAllTimers(); + await flushPromises(); + + // should still use the last request's response instead of running the deferred write + expect( + document.getElementById('editing-sessions').querySelectorAll('li') + .length, + ).toEqual(8); + + // the success event should be dispatched only once as the deferred write was skipped + expect(successEvents).toHaveLength(1); + expect(successEvents[0].detail.requestUrl).toEqual( + '/path/to/editing-sessions/?title=newvalue&type=some-type', + ); + + // should NOT update the current URL + // as the reflect-value attribute is not set + expect(window.location.search).toEqual(''); + }); + }); }); diff --git a/client/src/controllers/SwapController.ts b/client/src/controllers/SwapController.ts index 9e0c9377a5..5f02bc0323 100644 --- a/client/src/controllers/SwapController.ts +++ b/client/src/controllers/SwapController.ts @@ -47,6 +47,7 @@ export class SwapController extends Controller< icon: { default: '', type: String }, loading: { default: false, type: Boolean }, reflect: { default: false, type: Boolean }, + defer: { default: false, type: Boolean }, src: { default: '', type: String }, jsonPath: { default: '', type: String }, target: { default: '#listing-results', type: String }, @@ -62,6 +63,8 @@ export class SwapController extends Controller< declare iconValue: string; declare loadingValue: boolean; declare reflectValue: boolean; + /** Defer writing the results until the focus has left the target container */ + declare deferValue: boolean; declare srcValue: string; declare jsonPathValue: string; declare targetValue: string; @@ -77,6 +80,8 @@ export class SwapController extends Controller< searchLazy?: { (...args: any[]): void; cancel(): void }; /** Debounced function to submit the serialised form and then replace the DOM */ submitLazy?: { (...args: any[]): void; cancel(): void }; + /** A function that writes the HTML to the target */ + writeDeferred?: () => Promise; connect() { this.srcValue = @@ -314,26 +319,69 @@ export class SwapController extends Controller< return response.text(); }) .then((results) => { - target.innerHTML = results; + const write = async () => { + target.innerHTML = results; - if (this.reflectValue) { - const event = this.dispatch('reflect', { - cancelable: true, - detail: { requestUrl }, + if (this.reflectValue) { + const event = this.dispatch('reflect', { + cancelable: true, + detail: { requestUrl }, + target, + }); + if (!event.defaultPrevented) { + this.reflectParams(requestUrl); + } + } + + this.dispatch('success', { + cancelable: false, + detail: { requestUrl, results }, target, }); - if (!event.defaultPrevented) { - this.reflectParams(requestUrl); - } + + return results; + }; + + // If the currently focused element is within the target container, + // defer the write until the focus has left the container. + if ( + this.deferValue && + document.activeElement && + target.contains(document.activeElement) + ) { + return new Promise((resolve, reject) => { + this.writeDeferred = write; + + const handleFocusOut = (event: FocusEvent) => { + // If the new focus is still within the target container, do nothing + if (target.contains(event.relatedTarget as Node | null)) { + return; + } + + if (this.writeDeferred) { + this.writeDeferred().then(resolve).catch(reject); + // Clear the deferred write to ensure it's not called again + this.writeDeferred = undefined; + } else { + // The deferred write has been cleared but this listener is still + // triggered, which is unlikely to happen but possible + // (e.g. another request was made but the focus is still here and + // deferValue is set to false), so just resolve + resolve(results); + } + + // Done for the current data, remove the listener + target.removeEventListener('focusout', handleFocusOut); + }; + + target.addEventListener('focusout', handleFocusOut); + }); } - this.dispatch('success', { - cancelable: false, - detail: { requestUrl, results }, - target, - }); - - return results; + // If there's a previously deferred write, and we're not in a state that + // needs a deferred write, clear the previous one and write immediately. + this.writeDeferred = undefined; + return write(); }) .catch((error) => { if (signal.aborted) return; diff --git a/wagtail/admin/templates/wagtailadmin/shared/editing_sessions/module.html b/wagtail/admin/templates/wagtailadmin/shared/editing_sessions/module.html index 806fe90a8c..615c706a15 100644 --- a/wagtail/admin/templates/wagtailadmin/shared/editing_sessions/module.html +++ b/wagtail/admin/templates/wagtailadmin/shared/editing_sessions/module.html @@ -28,6 +28,7 @@ data-w-swap-target-value="#w-editing-sessions" data-w-swap-src-value="{{ ping_url }}" data-w-swap-json-path-value="html" + data-w-swap-defer-value="true" data-w-action-continue-value="true" data-w-action-url-value="{{ release_url }}" data-w-session-w-dialog-outlet="[data-edit-form] [data-controller='w-dialog']#w-overwrite-changes-dialog"