From 8fc810845429f7305423344ef93b6645b35fdb7d Mon Sep 17 00:00:00 2001 From: Nolan Lawson Date: Sun, 24 Mar 2019 15:08:34 -0700 Subject: [PATCH] fix: back button dismisses the modal dialog (#826) * fix: back button dismisses the modal dialog fixes #60 * try to manage nested modals * seems working now * fix modal timing issue * fix test flakiness * improve test flakiness again * fix muting timing issue * Revert "fix muting timing issue" * remove setTimeout from MediaDialog * refactor --- package.json | 2 +- .../dialog/components/ModalDialog.html | 35 ++- .../settings/instance/InstanceActions.html | 5 +- tests/spec/014-compose-post-privacy.js | 20 +- tests/spec/029-back-button-modal.js | 261 ++++++++++++++++++ tests/spec/030-shortcuts-modal.js | 24 +- tests/spec/114-mute-unmute.js | 6 + tests/utils.js | 5 +- yarn.lock | 4 +- 9 files changed, 324 insertions(+), 38 deletions(-) create mode 100644 tests/spec/029-back-button-modal.js diff --git a/package.json b/package.json index 92f825c..c5baa02 100644 --- a/package.json +++ b/package.json @@ -90,7 +90,7 @@ "rollup": "^1.7.0", "rollup-plugin-replace": "^2.1.1", "rollup-plugin-terser": "^4.0.4", - "sapper": "nolanlawson/sapper#for-pinafore-10", + "sapper": "nolanlawson/sapper#for-pinafore-14", "stringz": "^1.0.0", "svelte": "^2.16.1", "svelte-extras": "^2.0.2", diff --git a/src/routes/_components/dialog/components/ModalDialog.html b/src/routes/_components/dialog/components/ModalDialog.html index 966e91c..d338465 100644 --- a/src/routes/_components/dialog/components/ModalDialog.html +++ b/src/routes/_components/dialog/components/ModalDialog.html @@ -142,11 +142,13 @@ import { on, emit } from '../../../_utils/eventBus' import { pushShortcutScope, - popShortcutScope } from '../../../_utils/shortcuts' + popShortcutScope + } from '../../../_utils/shortcuts' export default { oncreate () { let { id } = this.get() + this.onPopState = this.onPopState.bind(this) let dialogElement = this.refs.node.parentElement this._a11yDialog = new A11yDialog(dialogElement) this._a11yDialog.on('hide', () => { @@ -161,7 +163,12 @@ pushShortcutScope(`modal-${id}`) }, ondestroy () { - let { id } = this.get() + window.removeEventListener('popstate', this.onPopState) + let { statePopped, statePushed, id } = this.get() + if (statePushed && !statePopped) { + // If we weren't closed due to popstate, then pop state to ensure the correct history. + window.history.back() + } popShortcutScope(`modal-${id}`) }, components: { Shortcut, SvgIcon }, @@ -192,25 +199,35 @@ } }, methods: { - showDialog (thisId) { + showDialog (otherId) { let { id } = this.get() - if (id !== thisId) { + if (otherId !== id) { return } + window.addEventListener('popstate', this.onPopState) + this.set({ statePushed: true }) + window.history.pushState({ modal: id }, null, location.href) document.body.classList.toggle('modal-open', true) this._a11yDialog.show() requestAnimationFrame(() => { this.set({ fadedIn: true }) }) }, - closeDialog (thisId) { + onPopState (event) { let { id } = this.get() - if (id !== thisId) { + if (!(event.state && event.state.modal === id)) { + // If the new state is not us, just assume that we need to be closed. + // This will only fail if modals are ever nested more than 2 levels deep. + this.set({ statePopped: true }) + this.closeDialog(id) + } + }, + closeDialog (otherId) { + let { id } = this.get() + if (id !== otherId) { return } - setTimeout(() => { // use setTimeout to workaround svelte timing issue - this._a11yDialog.hide() - }, 0) + this._a11yDialog.hide() } } } diff --git a/src/routes/_components/settings/instance/InstanceActions.html b/src/routes/_components/settings/instance/InstanceActions.html index 16a08e7..bb5ade5 100644 --- a/src/routes/_components/settings/instance/InstanceActions.html +++ b/src/routes/_components/settings/instance/InstanceActions.html @@ -40,7 +40,10 @@ showTextConfirmationDialog({ text: `Log out of ${instanceName}?` }).on('positive', () => { - /* no await */ logOutOfInstance(instanceName) + // TODO: dumb timing hack because the modal navigates back while we're trying to navigate forward + setTimeout(() => { + /* no await */logOutOfInstance(instanceName) + }, 200) }) } } diff --git a/tests/spec/014-compose-post-privacy.js b/tests/spec/014-compose-post-privacy.js index bbc8568..309469e 100644 --- a/tests/spec/014-compose-post-privacy.js +++ b/tests/spec/014-compose-post-privacy.js @@ -1,4 +1,11 @@ -import { getNthPostPrivacyOptionInDialog, postPrivacyButton } from '../utils' +import { + composeButton, + composeModalPostPrivacyButton, + getNthPostPrivacyOptionInDialog, + postPrivacyButton, postPrivacyDialogButtonUnlisted, + scrollToStatus, + sleep +} from '../utils' import { loginAsFoobar } from '../roles' fixture`014-compose-post-privacy.js` @@ -15,3 +22,14 @@ test('Changes post privacy', async t => { .click(getNthPostPrivacyOptionInDialog(1)) .expect(postPrivacyButton.getAttribute('aria-label')).eql('Adjust privacy (currently Public)') }) + +test('can use privacy dialog within compose dialog', async t => { + await loginAsFoobar(t) + await scrollToStatus(t, 16) + await t.expect(composeButton.getAttribute('aria-label')).eql('Compose') + await sleep(2000) + await t.click(composeButton) + .click(composeModalPostPrivacyButton) + .click(postPrivacyDialogButtonUnlisted) + .expect(composeModalPostPrivacyButton.getAttribute('aria-label')).eql('Adjust privacy (currently Unlisted)') +}) diff --git a/tests/spec/029-back-button-modal.js b/tests/spec/029-back-button-modal.js new file mode 100644 index 0000000..c70ed12 --- /dev/null +++ b/tests/spec/029-back-button-modal.js @@ -0,0 +1,261 @@ +import { + closeDialogButton, + composeButton, + composeModalInput, + composeModalPostPrivacyButton, + dialogOptionsOption, + getNthStatus, + getNthStatusMediaButton, + getNthStatusOptionsButton, + getUrl, + goBack, + goForward, + homeNavButton, + modalDialog, + modalDialogBackdrop, + notificationsNavButton, + postPrivacyDialogButtonUnlisted, + scrollToStatus, + sleep, + visibleModalDialog +} from '../utils' +import { loginAsFoobar } from '../roles' +import { indexWhere } from '../../src/routes/_utils/arrays' +import { homeTimeline } from '../fixtures' + +fixture`029-back-button-modal.js` + .page`http://localhost:4002` + +test('Back button dismisses the modal', async t => { + await loginAsFoobar(t) + let idx = indexWhere(homeTimeline, _ => (_.content || '').includes('2 kitten photos')) + await scrollToStatus(t, idx + 1) + await t + .expect(getUrl()).eql('http://localhost:4002/') + .hover(getNthStatus(idx + 1)) + .click(getNthStatusMediaButton(idx + 1)) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(getUrl()).eql('http://localhost:4002/') + await goBack() + await t + .expect(modalDialog.exists).notOk() + .expect(getUrl()).eql('http://localhost:4002/') +}) + +test('Back button dismisses a nested modal', async t => { + await loginAsFoobar(t) + await t + .expect(getUrl()).eql('http://localhost:4002/') + .hover(getNthStatus(1)) + .click(getNthStatusOptionsButton(1)) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .click(dialogOptionsOption.withText('Report')) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(modalDialog.innerText).contains('Report') + .expect(getUrl()).eql('http://localhost:4002/') + await goBack() + await t + .expect(modalDialog.exists).notOk() + .expect(getUrl()).eql('http://localhost:4002/') +}) + +test('Forward and back buttons', async t => { + await loginAsFoobar(t) + await t + .expect(getUrl()).eql('http://localhost:4002/') + .hover(getNthStatus(1)) + .click(getNthStatusOptionsButton(1)) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(getUrl()).eql('http://localhost:4002/') + await goBack() + await t + .expect(modalDialog.exists).notOk() + .expect(getUrl()).eql('http://localhost:4002/') + await goForward() + await t + .expect(modalDialog.exists).notOk() + .expect(getUrl()).eql('http://localhost:4002/') +}) + +test('Closing dialog pops history state', async t => { + await loginAsFoobar(t) + await t + .hover(getNthStatus(1)) + .click(getNthStatus(1)) + .expect(getUrl()).contains('/status') + .click(getNthStatusOptionsButton(1)) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(getUrl()).contains('/status') + .click(closeDialogButton) + .expect(modalDialog.exists).notOk() + .expect(getUrl()).contains('/status') + await goBack() + await t + .expect(getUrl()).eql('http://localhost:4002/') +}) + +test('Pressing backspace pops history state', async t => { + await loginAsFoobar(t) + await t + .hover(getNthStatus(1)) + .click(getNthStatus(1)) + .expect(getUrl()).contains('/status') + .click(getNthStatusOptionsButton(1)) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(getUrl()).contains('/status') + await sleep(1000) + await t + .pressKey('backspace') + .expect(modalDialog.exists).notOk() + .expect(getUrl()).contains('/status') + await goBack() + await t + .expect(getUrl()).eql('http://localhost:4002/') +}) + +test('Pressing Esc pops history state', async t => { + await loginAsFoobar(t) + await t + .hover(getNthStatus(1)) + .click(getNthStatus(1)) + .expect(getUrl()).contains('/status') + .click(getNthStatusOptionsButton(1)) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(getUrl()).contains('/status') + await sleep(1000) + await t + .pressKey('esc') + .expect(modalDialog.exists).notOk() + .expect(getUrl()).contains('/status') + await goBack() + await t + .expect(getUrl()).eql('http://localhost:4002/') +}) + +test('Clicking outside dialog pops history state', async t => { + await loginAsFoobar(t) + await t + .hover(getNthStatus(1)) + .click(getNthStatus(1)) + .expect(getUrl()).contains('/status') + .click(getNthStatusOptionsButton(1)) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(getUrl()).contains('/status') + .click(modalDialogBackdrop, { + offsetX: 1, + offsetY: 1 + }) + .expect(modalDialog.exists).notOk() + .expect(getUrl()).contains('/status') + await goBack() + await t + .expect(getUrl()).eql('http://localhost:4002/') +}) + +test('Closing nested modal pops history state', async t => { + await loginAsFoobar(t) + await t + .hover(getNthStatus(1)) + .click(getNthStatus(1)) + .expect(getUrl()).contains('/status') + .click(getNthStatusOptionsButton(1)) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(getUrl()).contains('/status') + await sleep(1000) + await t + .click(dialogOptionsOption.withText('Report')) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(modalDialog.innerText).contains('Report') + .click(closeDialogButton) + .expect(modalDialog.exists).notOk() + .expect(getUrl()).contains('/status') + await goBack() + await t + .expect(getUrl()).eql('http://localhost:4002/') +}) + +test('History works correctly for nested modal', async t => { + await loginAsFoobar(t) + await t + .click(notificationsNavButton) + .click(homeNavButton) + await scrollToStatus(t, 10) + await t + .click(composeButton) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(composeModalInput.exists).ok() + .click(composeModalPostPrivacyButton) + .expect(visibleModalDialog.textContent).contains('Post privacy') + await sleep(1000) + await t + .pressKey('backspace') + await t + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(composeModalInput.exists).ok() + await sleep(1000) + await t + .pressKey('backspace') + .expect(modalDialog.exists).notOk() + .expect(getUrl()).eql('http://localhost:4002/') + await goBack() + await t + .expect(getUrl()).contains('/notifications') +}) + +test('History works correctly for nested modal 2', async t => { + await loginAsFoobar(t) + await t + .click(notificationsNavButton) + .click(homeNavButton) + await scrollToStatus(t, 10) + await t + .click(composeButton) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(composeModalInput.exists).ok() + .click(composeModalPostPrivacyButton) + .expect(visibleModalDialog.textContent).contains('Post privacy') + await sleep(1000) + await t + .click(postPrivacyDialogButtonUnlisted) + await t + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(composeModalInput.exists).ok() + .expect(composeModalPostPrivacyButton.getAttribute('aria-label')).eql('Adjust privacy (currently Unlisted)') + await sleep(1000) + await goBack() + await t + .expect(modalDialog.exists).notOk() + .expect(getUrl()).eql('http://localhost:4002/') + await goBack() + await t + .expect(getUrl()).contains('/notifications') +}) + +test('History works correctly for nested modal 3', async t => { + await loginAsFoobar(t) + await t + .click(notificationsNavButton) + .click(homeNavButton) + await scrollToStatus(t, 10) + await t + .click(composeButton) + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(composeModalInput.exists).ok() + .click(composeModalPostPrivacyButton) + .expect(visibleModalDialog.textContent).contains('Post privacy') + await sleep(1000) + await t + .click(closeDialogButton) + await t + .expect(modalDialog.hasAttribute('aria-hidden')).notOk() + .expect(composeModalInput.exists).ok() + await sleep(1000) + await t + .click(closeDialogButton) + .expect(modalDialog.exists).notOk() + await t + .expect(getUrl()).eql('http://localhost:4002/') + await goBack() + await t + .expect(getUrl()).contains('/notifications') +}) diff --git a/tests/spec/030-shortcuts-modal.js b/tests/spec/030-shortcuts-modal.js index 3e9b732..a39e1d8 100644 --- a/tests/spec/030-shortcuts-modal.js +++ b/tests/spec/030-shortcuts-modal.js @@ -1,8 +1,7 @@ import { - composeButton, composeModalInput, composeModalPostPrivacyButton, getMediaScrollLeft, getNthStatusMediaButton, getNthStatusOptionsButton, - modalDialog, scrollToStatus, sleep, visibleModalDialog + modalDialog, scrollToStatus, sleep } from '../utils' import { loginAsFoobar } from '../roles' import { indexWhere } from '../../src/routes/_utils/arrays' @@ -52,24 +51,3 @@ test('Left/right changes active media in modal', async t => { .pressKey('backspace') .expect(modalDialog.exists).false }) - -test('Backspace works correctly for nested modal', async t => { - await loginAsFoobar(t) - await scrollToStatus(t, 10) - await t - .click(composeButton) - .expect(modalDialog.hasAttribute('aria-hidden')).notOk() - .expect(composeModalInput.exists).ok() - .click(composeModalPostPrivacyButton) - .expect(visibleModalDialog.textContent).contains('Post privacy') - await sleep(1000) - await t - .pressKey('backspace') - await t - .expect(modalDialog.hasAttribute('aria-hidden')).notOk() - .expect(composeModalInput.exists).ok() - await sleep(1000) - await t - .pressKey('backspace') - .expect(modalDialog.exists).notOk() -}) diff --git a/tests/spec/114-mute-unmute.js b/tests/spec/114-mute-unmute.js index 533e13f..9832c94 100644 --- a/tests/spec/114-mute-unmute.js +++ b/tests/spec/114-mute-unmute.js @@ -28,6 +28,8 @@ test('Can mute and unmute an account', async t => { .expect(getNthDialogOptionsOption(1).innerText).contains('Unfollow @admin') .expect(getNthDialogOptionsOption(2).innerText).contains('Block @admin') .expect(getNthDialogOptionsOption(3).innerText).contains('Mute @admin') + await sleep(1000) + await t .click(getNthDialogOptionsOption(3)) await sleep(1000) await t @@ -45,6 +47,8 @@ test('Can mute and unmute an account', async t => { .expect(getNthDialogOptionsOption(2).innerText).contains('Unfollow @admin') .expect(getNthDialogOptionsOption(3).innerText).contains('Block @admin') .expect(getNthDialogOptionsOption(4).innerText).contains('Unmute @admin') + await sleep(1000) + await t .click(getNthDialogOptionsOption(4)) await sleep(1000) await t @@ -53,6 +57,8 @@ test('Can mute and unmute an account', async t => { .expect(getNthDialogOptionsOption(2).innerText).contains('Unfollow @admin') .expect(getNthDialogOptionsOption(3).innerText).contains('Block @admin') .expect(getNthDialogOptionsOption(4).innerText).contains('Mute @admin') + await sleep(1000) + await t .click(closeDialogButton) .expect(accountProfileFollowButton.getAttribute('aria-label')).eql('Unfollow') }) diff --git a/tests/utils.js b/tests/utils.js index 204de0b..3a01e01 100644 --- a/tests/utils.js +++ b/tests/utils.js @@ -7,7 +7,8 @@ export const instanceInput = $('#instanceInput') export const modalDialog = $('.modal-dialog') export const visibleModalDialog = $('.modal-dialog:not([aria-hidden="true"])') export const modalDialogContents = $('.modal-dialog-contents') -export const closeDialogButton = $('.close-dialog-button') +export const modalDialogBackdrop = $('.modal-dialog-backdrop') +export const closeDialogButton = $('.modal-dialog:not([aria-hidden="true"]) .close-dialog-button') export const notificationsNavButton = $('nav a[href="/notifications"]') export const homeNavButton = $('nav a[href="/"]') export const localTimelineNavButton = $('nav a[href="/local"]') @@ -57,6 +58,8 @@ export const composeModalContentWarningInput = $('.modal-dialog .content-warning export const composeModalEmojiButton = $('.modal-dialog .compose-box-toolbar button:nth-child(1)') export const composeModalPostPrivacyButton = $('.modal-dialog .compose-box-toolbar button:nth-child(3)') +export const postPrivacyDialogButtonUnlisted = $('[aria-label="Post privacy dialog"] li:nth-child(2) button') + export function getComposeModalNthMediaAltInput (n) { return $(`.modal-dialog .compose-media:nth-child(${n}) .compose-media-alt input`) } diff --git a/yarn.lock b/yarn.lock index 43a8114..89e7516 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6433,9 +6433,9 @@ sanitize-filename@^1.6.0: dependencies: truncate-utf8-bytes "^1.0.0" -sapper@nolanlawson/sapper#for-pinafore-10: +sapper@nolanlawson/sapper#for-pinafore-14: version "0.25.0" - resolved "https://codeload.github.com/nolanlawson/sapper/tar.gz/22160f7a7b8fbd39ed037150338236074d69fc2d" + resolved "https://codeload.github.com/nolanlawson/sapper/tar.gz/8bf2d62a9911b58d3004a716b2e30d431f6f4b2a" dependencies: html-minifier "^3.5.20" http-link-header "^1.0.2"