From 42e466f3c277429f199f60c864b3fde8effa5180 Mon Sep 17 00:00:00 2001
From: Nolan Lawson <nolan@nolanlawson.com>
Date: Thu, 21 Feb 2019 23:50:27 -0800
Subject: [PATCH] fix: j/k shortcuts correctly set active element (#1028)

fixes #1018
---
 src/routes/_components/list/ListItem.html     |  8 ---
 src/routes/_components/list/listStore.js      |  1 -
 .../shortcut/ScrollListShortcuts.html         | 21 ++++++--
 .../_components/status/Notification.html      | 15 +++---
 src/routes/_components/status/Status.html     |  8 +--
 .../timeline/NotificationVirtualListItem.html |  1 -
 .../timeline/StatusVirtualListItem.html       |  1 -
 .../_components/virtualList/VirtualList.html  |  6 ++-
 .../virtualList/VirtualListItem.html          |  5 +-
 .../virtualList/virtualListStore.js           |  1 -
 tests/spec/024-shortcuts-navigation.js        |  4 +-
 tests/spec/025-shortcuts-status.js            | 49 +++++++++----------
 tests/spec/026-shortcuts-notification.js      | 16 +++---
 tests/utils.js                                | 11 +++++
 14 files changed, 79 insertions(+), 68 deletions(-)

diff --git a/src/routes/_components/list/ListItem.html b/src/routes/_components/list/ListItem.html
index bea70643..62c3c77c 100644
--- a/src/routes/_components/list/ListItem.html
+++ b/src/routes/_components/list/ListItem.html
@@ -5,13 +5,5 @@
               virtualIndex={index}
               virtualLength={length}
               virtualKey={key}
-              active={active}
   />
 </div>
-<script>
-  export default {
-    computed: {
-      'active': ({ $activeItem, key }) => $activeItem === key
-    }
-  }
-</script>
diff --git a/src/routes/_components/list/listStore.js b/src/routes/_components/list/listStore.js
index 2ee464a6..0668c8de 100644
--- a/src/routes/_components/list/listStore.js
+++ b/src/routes/_components/list/listStore.js
@@ -8,7 +8,6 @@ class ListStore extends RealmStore {
 
 const listStore = new ListStore()
 
-listStore.computeForRealm('activeItem', null)
 listStore.computeForRealm('intersectionStates', {})
 
 if (process.browser && process.env.NODE_ENV !== 'production') {
diff --git a/src/routes/_components/shortcut/ScrollListShortcuts.html b/src/routes/_components/shortcut/ScrollListShortcuts.html
index 2ded3188..f4b23463 100644
--- a/src/routes/_components/shortcut/ScrollListShortcuts.html
+++ b/src/routes/_components/shortcut/ScrollListShortcuts.html
@@ -15,8 +15,10 @@
   export default {
     data: () => ({
       scope: 'global',
-      itemToKey: (item) => item,
-      keyToElement: (key) => document.getElementById('list-item-' + key),
+      itemToKey: (item) => item.key,
+      keyToElement: (key) => {
+        return document.querySelector(`[shortcut-key=${JSON.stringify(key)}]`)
+      },
       activeItemChangeTime: 0
     }),
     computed: {
@@ -94,7 +96,11 @@
         }
       },
       checkActiveItem (timeStamp) {
-        let { activeItem } = this.store.get()
+        let activeElement = document.activeElement
+        if (!activeElement) {
+          return null
+        }
+        let activeItem = activeElement.getAttribute('shortcut-key')
         if (!activeItem) {
           return null
         }
@@ -108,7 +114,14 @@
       },
       setActiveItem (key, timeStamp) {
         this.set({ activeItemChangeTime: timeStamp })
-        this.store.setForRealm({ activeItem: key })
+        let { keyToElement } = this.get()
+        try {
+          keyToElement(key).focus({
+            preventScroll: true
+          })
+        } catch (err) {
+          console.error(err)
+        }
       }
     }
   }
diff --git a/src/routes/_components/status/Notification.html b/src/routes/_components/status/Notification.html
index d2e938b0..b9f8be91 100644
--- a/src/routes/_components/status/Notification.html
+++ b/src/routes/_components/status/Notification.html
@@ -1,6 +1,6 @@
 {#if status}
   <Status {index} {length} {timelineType} {timelineValue} {focusSelector}
-          {status} {notification} {active} {shortcutScope} on:recalculateHeight
+          {status} {notification} {shortcutScope} on:recalculateHeight
   />
 {:else}
   <article class={className}
@@ -8,7 +8,8 @@
            aria-posinset={index}
            aria-setsize={length}
            aria-label={ariaLabel}
-           status-active={active}
+           focus-key={focusKey}
+           shortcut-key={shortcutScope}
   >
     <StatusHeader {notification} {notificationId} {status} {statusId} {timelineType}
                   {account} {accountId} {uuid} isStatusInNotification="true" />
@@ -25,8 +26,9 @@
     padding: 10px 20px;
     border-bottom: 1px solid var(--main-border);
   }
-  .notification-article.status-active {
+  .notification-article:focus {
     background-color: var(--status-active-background);
+    outline: none;
   }
   @media (max-width: 767px) {
     .notification-article {
@@ -53,7 +55,6 @@
       Shortcut
     },
     data: () => ({
-      active: false,
       shortcutScope: null
     }),
     store: () => store,
@@ -69,11 +70,11 @@
       ariaLabel: ({ status, account, $omitEmojiInDisplayNames }) => (
         !status && `${getAccountAccessibleName(account, $omitEmojiInDisplayNames)} followed you, @${account.acct}`
       ),
-      className: ({ active, $underlineLinks }) => (classname(
+      className: ({ $underlineLinks }) => (classname(
         'notification-article',
-        active && 'status-active',
         $underlineLinks && 'underline-links'
-      ))
+      )),
+      focusKey: ({ uuid }) => `notification-follower-${uuid}`
     },
     methods: {
       openAuthorProfile () {
diff --git a/src/routes/_components/status/Status.html b/src/routes/_components/status/Status.html
index 57376c2f..dbf90575 100644
--- a/src/routes/_components/status/Status.html
+++ b/src/routes/_components/status/Status.html
@@ -2,6 +2,7 @@
          tabindex="0"
          delegate-key={delegateKey}
          focus-key={delegateKey}
+         shortcut-key={shortcutScope}
          aria-posinset={index}
          aria-setsize={length}
          aria-label={ariaLabel}
@@ -74,8 +75,9 @@
     background-color: var(--status-direct-background);
   }
 
-  .status-article.status-active {
+  .status-article:focus {
     background-color: var(--status-active-background);
+    outline: none;
   }
 
   .status-article.status-in-own-thread {
@@ -171,7 +173,6 @@
       Shortcut
     },
     data: () => ({
-      active: false,
       notification: void 0,
       replyVisibility: void 0,
       contentPreloaded: false,
@@ -272,12 +273,11 @@
         status.reblog ||
         timelineType === 'pinned'
       ),
-      className: ({ visibility, timelineType, isStatusInOwnThread, active, $underlineLinks, $disableTapOnStatus }) => (classname(
+      className: ({ visibility, timelineType, isStatusInOwnThread, $underlineLinks, $disableTapOnStatus }) => (classname(
         'status-article',
         visibility === 'direct' && 'status-direct',
         timelineType !== 'search' && 'status-in-timeline',
         isStatusInOwnThread && 'status-in-own-thread',
-        active && 'status-active',
         $underlineLinks && 'underline-links',
         !$disableTapOnStatus && 'tap-on-status'
       )),
diff --git a/src/routes/_components/timeline/NotificationVirtualListItem.html b/src/routes/_components/timeline/NotificationVirtualListItem.html
index e42a00c5..d7ab0d34 100644
--- a/src/routes/_components/timeline/NotificationVirtualListItem.html
+++ b/src/routes/_components/timeline/NotificationVirtualListItem.html
@@ -6,7 +6,6 @@
   shortcutScope={virtualKey}
   index={virtualIndex}
   length={virtualLength}
-  {active}
   on:recalculateHeight />
 <script>
   import Notification from '../status/Notification.html'
diff --git a/src/routes/_components/timeline/StatusVirtualListItem.html b/src/routes/_components/timeline/StatusVirtualListItem.html
index 228b47c2..4f7744c4 100644
--- a/src/routes/_components/timeline/StatusVirtualListItem.html
+++ b/src/routes/_components/timeline/StatusVirtualListItem.html
@@ -5,7 +5,6 @@
         shortcutScope={virtualKey}
         index={virtualIndex}
         length={virtualLength}
-        active={active}
         on:recalculateHeight />
 <script>
   import Status from '../status/Status.html'
diff --git a/src/routes/_components/virtualList/VirtualList.html b/src/routes/_components/virtualList/VirtualList.html
index f82b3d0d..1dd36750 100644
--- a/src/routes/_components/virtualList/VirtualList.html
+++ b/src/routes/_components/virtualList/VirtualList.html
@@ -18,7 +18,7 @@
     {/if}
   </div>
 </VirtualListContainer>
-<ScrollListShortcuts bind:items=$visibleItems
+<ScrollListShortcuts items={$visibleItems}
                      itemToKey={(visibleItem) => visibleItem.key} />
 <style>
   .virtual-list {
@@ -122,7 +122,9 @@
           return
         }
         mark('calculateListOffset')
-        let listOffset = node.offsetParent.offsetTop
+        let { offsetParent } = node
+        // TODO: offsetParent is null sometimes in testcafe tests
+        let listOffset = offsetParent ? offsetParent.offsetTop : 0
         this.store.setForRealm({ listOffset })
         stop('calculateListOffset')
       }
diff --git a/src/routes/_components/virtualList/VirtualListItem.html b/src/routes/_components/virtualList/VirtualListItem.html
index 80f61ba7..3c655b18 100644
--- a/src/routes/_components/virtualList/VirtualListItem.html
+++ b/src/routes/_components/virtualList/VirtualListItem.html
@@ -1,5 +1,4 @@
 <div class="virtual-list-item list-item {shown ? 'shown' : ''}"
-     id={`list-item-${key}`}
      aria-hidden={!shown}
      ref:node
      style="transform: translateY({offset}px);" >
@@ -8,7 +7,6 @@
               virtualIndex={index}
               virtualLength={$length}
               virtualKey={key}
-              active={active}
               on:recalculateHeight="doRecalculateHeight()"/>
 </div>
 <style>
@@ -52,8 +50,7 @@
     },
     store: () => virtualListStore,
     computed: {
-      'shown': ({ $itemHeights, key }) => $itemHeights[key] > 0,
-      'active': ({ $activeItem, key }) => $activeItem === key
+      'shown': ({ $itemHeights, key }) => $itemHeights[key] > 0
     },
     methods: {
       doRecalculateHeight () {
diff --git a/src/routes/_components/virtualList/virtualListStore.js b/src/routes/_components/virtualList/virtualListStore.js
index 1738e7d4..c7a2a6cd 100644
--- a/src/routes/_components/virtualList/virtualListStore.js
+++ b/src/routes/_components/virtualList/virtualListStore.js
@@ -22,7 +22,6 @@ virtualListStore.computeForRealm('scrollHeight', 0)
 virtualListStore.computeForRealm('offsetHeight', 0)
 virtualListStore.computeForRealm('listOffset', 0)
 virtualListStore.computeForRealm('itemHeights', {})
-virtualListStore.computeForRealm('activeItem', null)
 
 virtualListStore.compute('rawVisibleItems',
   ['items', 'scrollTop', 'itemHeights', 'offsetHeight', 'showHeader', 'headerHeight', 'listOffset'],
diff --git a/tests/spec/024-shortcuts-navigation.js b/tests/spec/024-shortcuts-navigation.js
index f96e5ff8..aab9adab 100644
--- a/tests/spec/024-shortcuts-navigation.js
+++ b/tests/spec/024-shortcuts-navigation.js
@@ -63,7 +63,7 @@ test('Shortcut backspace goes back from favorites', async t => {
     .expect(getUrl()).contains('/federated')
     .pressKey('g f')
     .expect(getUrl()).contains('/favorites')
-    .pressKey('Backspace')
+    .pressKey('backspace')
     .expect(getUrl()).contains('/federated')
 })
 
@@ -88,7 +88,7 @@ test('Global shortcut has no effects while in modal dialog', async t => {
     .expect(modalDialogContents.exists).ok()
     .pressKey('s') // does nothing
     .expect(getUrl()).contains('/favorites')
-    .pressKey('Backspace')
+    .pressKey('backspace')
     .expect(modalDialogContents.exists).notOk()
     .pressKey('s') // now works
     .expect(getUrl()).contains('/search')
diff --git a/tests/spec/025-shortcuts-status.js b/tests/spec/025-shortcuts-status.js
index 455015c2..7728ae05 100644
--- a/tests/spec/025-shortcuts-status.js
+++ b/tests/spec/025-shortcuts-status.js
@@ -1,4 +1,3 @@
-import { Selector as $ } from 'testcafe'
 import {
   closeDialogButton,
   composeModalInput,
@@ -9,7 +8,8 @@ import {
   getNthStatusSensitiveMediaButton,
   getNthStatusSpoiler,
   getUrl, modalDialog,
-  scrollToStatus
+  scrollToStatus,
+  isNthStatusActive, getActiveElementRectTop
 } from '../utils'
 import { homeTimeline } from '../fixtures'
 import { loginAsFoobar } from '../roles'
@@ -18,16 +18,12 @@ import { indexWhere } from '../../src/routes/_utils/arrays'
 fixture`025-shortcuts-status.js`
   .page`http://localhost:4002`
 
-function isNthStatusActive (idx) {
-  return getNthStatus(idx).hasClass('status-active')
-}
-
 async function activateStatus (t, idx) {
   let timeout = 20000
   for (let i = 0; i <= idx; i++) {
     await t.expect(getNthStatus(i).exists).ok({ timeout })
       .pressKey('j')
-      .expect(getNthStatus(i).hasClass('status-active')).ok()
+      .expect(isNthStatusActive(i)()).ok()
   }
 }
 
@@ -36,24 +32,24 @@ test('Shortcut j/k change the active status', async t => {
   await t
     .expect(getUrl()).eql('http://localhost:4002/')
     .expect(getNthStatus(0).exists).ok({ timeout: 30000 })
-    .expect(isNthStatusActive(0)).notOk()
+    .expect(isNthStatusActive(0)()).notOk()
     .pressKey('j')
-    .expect(isNthStatusActive(0)).ok()
+    .expect(isNthStatusActive(0)()).ok()
     .pressKey('j')
-    .expect(isNthStatusActive(1)).ok()
+    .expect(isNthStatusActive(1)()).ok()
     .pressKey('j')
-    .expect(isNthStatusActive(2)).ok()
+    .expect(isNthStatusActive(2)()).ok()
     .pressKey('j')
-    .expect(isNthStatusActive(3)).ok()
+    .expect(isNthStatusActive(3)()).ok()
     .pressKey('k')
-    .expect(isNthStatusActive(2)).ok()
+    .expect(isNthStatusActive(2)()).ok()
     .pressKey('k')
-    .expect(isNthStatusActive(1)).ok()
+    .expect(isNthStatusActive(1)()).ok()
     .pressKey('k')
-    .expect(isNthStatusActive(0)).ok()
-    .expect(isNthStatusActive(1)).notOk()
-    .expect(isNthStatusActive(2)).notOk()
-    .expect(isNthStatusActive(3)).notOk()
+    .expect(isNthStatusActive(0)()).ok()
+    .expect(isNthStatusActive(1)()).notOk()
+    .expect(isNthStatusActive(2)()).notOk()
+    .expect(isNthStatusActive(3)()).notOk()
 })
 
 test('Shortcut j goes to the first visible status', async t => {
@@ -64,8 +60,7 @@ test('Shortcut j goes to the first visible status', async t => {
   await t
     .expect(getNthStatus(10).exists).ok({ timeout: 30000 })
     .pressKey('j')
-    .expect($('.status-active').exists).ok()
-    .expect($('.status-active').getBoundingClientRectProperty('top')).gte(0)
+    .expect(getActiveElementRectTop()).gte(0)
 })
 
 test('Shortcut o opens active status, backspace goes back', async t => {
@@ -76,11 +71,11 @@ test('Shortcut o opens active status, backspace goes back', async t => {
     .pressKey('j') // activates status 0
     .pressKey('j') // activates status 1
     .pressKey('j') // activates status 2
-    .expect(isNthStatusActive(2)).ok()
+    .expect(isNthStatusActive(2)()).ok()
     .pressKey('o')
     .expect(getUrl()).contains('/statuses/')
     .pressKey('Backspace')
-    .expect(isNthStatusActive(2)).ok()
+    .expect(isNthStatusActive(2)()).ok()
 })
 
 test('Shortcut x shows/hides spoilers', async t => {
@@ -90,7 +85,7 @@ test('Shortcut x shows/hides spoilers', async t => {
     .expect(getUrl()).eql('http://localhost:4002/')
   await activateStatus(t, idx)
   await t
-    .expect(getNthStatus(idx).hasClass('status-active')).ok()
+    .expect(isNthStatusActive(idx)()).ok()
     .expect(getNthStatusSpoiler(idx).innerText).contains('kitten CW')
     .expect(getNthStatusContent(idx).hasClass('shown')).notOk()
     .pressKey('x')
@@ -106,7 +101,7 @@ test('Shortcut y shows/hides sensitive image', async t => {
     .expect(getUrl()).eql('http://localhost:4002/')
   await activateStatus(t, idx)
   await t
-    .expect(getNthStatus(idx).hasClass('status-active')).ok()
+    .expect(isNthStatusActive(idx)()).ok()
     .expect(getNthStatusSensitiveMediaButton(idx).exists).ok()
     .expect(getNthStatusMedia(idx).exists).notOk()
     .pressKey('y')
@@ -123,7 +118,7 @@ test('Shortcut f toggles favorite status', async t => {
     .expect(getNthStatus(idx).exists).ok({ timeout: 30000 })
     .expect(getNthFavorited(idx)).eql('false')
     .pressKey('j '.repeat(idx + 1))
-    .expect(getNthStatus(idx).hasClass('status-active')).ok()
+    .expect(isNthStatusActive(idx)()).ok()
     .pressKey('f')
     .expect(getNthFavorited(idx)).eql('true')
     .pressKey('f')
@@ -137,7 +132,7 @@ test('Shortcut p toggles profile', async t => {
     .expect(getUrl()).eql('http://localhost:4002/')
     .expect(getNthStatus(idx).exists).ok({ timeout: 30000 })
     .pressKey('j '.repeat(idx + 1))
-    .expect(getNthStatus(idx).hasClass('status-active')).ok()
+    .expect(isNthStatusActive(idx)()).ok()
     .pressKey('p')
     .expect(getUrl()).contains('/accounts/3')
 })
@@ -149,7 +144,7 @@ test('Shortcut m toggles mention', async t => {
     .expect(getUrl()).eql('http://localhost:4002/')
     .expect(getNthStatus(idx).exists).ok({ timeout: 30000 })
     .pressKey('j '.repeat(idx + 1))
-    .expect(getNthStatus(idx).hasClass('status-active')).ok()
+    .expect(isNthStatusActive(idx)()).ok()
     .pressKey('m')
     .expect(composeModalInput.value).eql('@quux ')
     .click(closeDialogButton)
diff --git a/tests/spec/026-shortcuts-notification.js b/tests/spec/026-shortcuts-notification.js
index 527c7ec3..3d09cf55 100644
--- a/tests/spec/026-shortcuts-notification.js
+++ b/tests/spec/026-shortcuts-notification.js
@@ -3,7 +3,8 @@ import {
   composeModalInput,
   getNthFavorited,
   getNthStatus,
-  getUrl, modalDialog, notificationsNavButton
+  getUrl, modalDialog, notificationsNavButton,
+  isNthStatusActive, goBack
 } from '../utils'
 import { loginAsFoobar } from '../roles'
 
@@ -20,7 +21,7 @@ test('Shortcut f toggles favorite status in notification', async t => {
     .expect(getNthStatus(idx).exists).ok({ timeout: 30000 })
     .expect(getNthFavorited(idx)).eql('false')
     .pressKey('j '.repeat(idx + 1))
-    .expect(getNthStatus(idx).hasClass('status-active')).ok()
+    .expect(isNthStatusActive(idx)()).ok()
     .pressKey('f')
     .expect(getNthFavorited(idx)).eql('true')
     .pressKey('f')
@@ -36,9 +37,12 @@ test('Shortcut p toggles profile in a follow notification', async t => {
     .expect(getUrl()).contains('/notifications')
     .expect(getNthStatus(0).exists).ok({ timeout: 30000 })
     .pressKey('j '.repeat(idx + 1))
-    .expect(getNthStatus(idx).hasClass('status-active')).ok()
+    .expect(isNthStatusActive(idx)()).ok()
     .pressKey('p')
     .expect(getUrl()).contains('/accounts/3')
+  await goBack()
+  await t
+    .expect(isNthStatusActive(idx)()).ok() // focus preserved
 })
 
 test('Shortcut m toggles mention in a follow notification', async t => {
@@ -50,7 +54,7 @@ test('Shortcut m toggles mention in a follow notification', async t => {
     .expect(getUrl()).contains('/notifications')
     .expect(getNthStatus(0).exists).ok({ timeout: 30000 })
     .pressKey('j '.repeat(idx + 1))
-    .expect(getNthStatus(idx).hasClass('status-active')).ok()
+    .expect(isNthStatusActive(idx)()).ok()
     .pressKey('m')
     .expect(composeModalInput.value).eql('@quux ')
     .click(closeDialogButton)
@@ -66,7 +70,7 @@ test('Shortcut p refers to booster in a boost notification', async t => {
     .expect(getUrl()).contains('/notifications')
     .expect(getNthStatus(0).exists).ok({ timeout: 30000 })
     .pressKey('j '.repeat(idx + 1))
-    .expect(getNthStatus(idx).hasClass('status-active')).ok()
+    .expect(isNthStatusActive(idx)()).ok()
     .pressKey('p')
     .expect(getUrl()).contains('/accounts/1')
 })
@@ -80,7 +84,7 @@ test('Shortcut m refers to favoriter in a favorite notification', async t => {
     .expect(getUrl()).contains('/notifications')
     .expect(getNthStatus(0).exists).ok({ timeout: 30000 })
     .pressKey('j '.repeat(idx + 1))
-    .expect(getNthStatus(idx).hasClass('status-active')).ok()
+    .expect(isNthStatusActive(idx)()).ok()
     .pressKey('m')
     .expect(composeModalInput.value).eql('@admin ')
     .click(closeDialogButton)
diff --git a/tests/utils.js b/tests/utils.js
index 161df581..5675b8b0 100644
--- a/tests/utils.js
+++ b/tests/utils.js
@@ -88,6 +88,10 @@ export const getActiveElementInnerText = exec(() =>
   (document.activeElement && document.activeElement.innerText) || ''
 )
 
+export const getActiveElementRectTop = exec(() => (
+  (document.activeElement && document.activeElement.getBoundingClientRect().top) || -1
+))
+
 export const getActiveElementInsideNthStatus = exec(() => {
   let element = document.activeElement
   while (element) {
@@ -151,6 +155,13 @@ export const focus = (selector) => (exec(() => {
   }
 }))
 
+export const isNthStatusActive = (idx) => (exec(() => {
+  return document.activeElement &&
+    document.activeElement.getAttribute('aria-posinset') === idx.toString()
+}, {
+  dependencies: { idx }
+}))
+
 export const scrollToBottom = exec(() => {
   document.scrollingElement.scrollTop = document.scrollingElement.scrollHeight
 })