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

@@ -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",

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()
-          this._a11yDialog.hide()
-        }, 0)
       }
     }
   }

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)
         })
       }
     }

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)')
+})

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')
+})

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()
-})

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')
 })

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`)
 }

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"