fix: fix keyboard shortcuts for pinned toots (#1033)

* fix: fix keyboard shortcuts for pinned toots

fixes #908

* fix test

src/routes/_components/list/List.html

@@ -10,7 +10,6 @@
     />
   {/each}
 </div>
-<ScrollListShortcuts bind:items=safeItems/>
 <style>
   .the-list {
     position: relative;
@@ -18,7 +17,6 @@
 </style>
 <script>
   import ListLazyItem from './ListLazyItem.html'
-  import ScrollListShortcuts from '../shortcut/ScrollListShortcuts.html'
   import { listStore } from './listStore'
   import { getScrollContainer } from '../../_utils/scrollContainer'
   import { getMainTopMargin } from '../../_utils/getMainTopMargin'
@@ -64,8 +62,7 @@
       length: ({ safeItems }) => safeItems.length
     },
     components: {
-      ListLazyItem,
-      ScrollListShortcuts
+      ListLazyItem
     },
     store: () => listStore
   }

src/routes/_components/shortcut/ScrollListShortcuts.html

@@ -12,24 +12,19 @@
 
   const VISIBILITY_CHECK_DELAY_MS = 600
 
+  const keyToElement = key => document.querySelector(`[shortcut-key=${JSON.stringify(key)}]`)
+  const elementToKey = element => element.getAttribute('shortcut-key')
+  const scope = 'global'
+
   export default {
     data: () => ({
-      scope: 'global',
-      itemToKey: (item) => item,
-      keyToElement: (key) => {
-        return document.querySelector(`[shortcut-key=${JSON.stringify(key)}]`)
-      },
-      activeItemChangeTime: 0
+      activeItemChangeTime: 0,
+      elements: document.getElementsByClassName('shortcut-list-item')
     }),
-    computed: {
-      itemToElement: ({ keyToElement, itemToKey }) => (item) => keyToElement(itemToKey(item))
-    },
     oncreate () {
-      let { scope } = this.get()
       addShortcutFallback(scope, this)
     },
     ondestroy () {
-      let { scope } = this.get()
       removeShortcutFallback(scope, this)
     },
     methods: {
@@ -48,10 +43,10 @@
         }
         let activeItemKey = this.checkActiveItem(event.timeStamp)
         if (!activeItemKey) {
-          let { items, itemToKey, itemToElement } = this.get()
-          let index = firstVisibleElementIndex(items, itemToElement).first
+          let { elements } = this.get()
+          let index = firstVisibleElementIndex(elements).first
           if (index >= 0) {
-            activeItemKey = itemToKey(items[index])
+            activeItemKey = elementToKey(elements[index])
           }
         }
         if (activeItemKey) {
@@ -59,18 +54,14 @@
         }
       },
       changeActiveItem (movement, timeStamp) {
-        let {
-          items,
-          itemToElement,
-          itemToKey,
-          keyToElement } = this.get()
+        let { elements } = this.get()
         let index = -1
         let activeItemKey = this.checkActiveItem(timeStamp)
         if (activeItemKey) {
-          let len = items.length
+          let len = elements.length
           let i = -1
           while (++i < len) {
-            if (itemToKey(items[i]) === activeItemKey) {
+            if (elementToKey(elements[i]) === activeItemKey) {
               index = i
               break
             }
@@ -83,14 +74,13 @@
           return
         }
         if (index === -1) {
-          let { first, firstComplete } = firstVisibleElementIndex(
-            items, itemToElement)
+          let { first, firstComplete } = firstVisibleElementIndex(elements)
           index = (movement > 0) ? firstComplete : first
         } else {
           index += movement
         }
-        if (index >= 0 && index < items.length) {
-          activeItemKey = itemToKey(items[index])
+        if (index >= 0 && index < elements.length) {
+          activeItemKey = elementToKey(elements[index])
           this.setActiveItem(activeItemKey, timeStamp)
           scrollIntoViewIfNeeded(keyToElement(activeItemKey))
         }
@@ -104,7 +94,7 @@
         if (!activeItem) {
           return null
         }
-        let { activeItemChangeTime, keyToElement } = this.get()
+        let { activeItemChangeTime } = this.get()
         if ((timeStamp - activeItemChangeTime) > VISIBILITY_CHECK_DELAY_MS &&
             !isVisible(keyToElement(activeItem))) {
           this.setActiveItem(null, 0)
@@ -114,7 +104,6 @@
       },
       setActiveItem (key, timeStamp) {
         this.set({ activeItemChangeTime: timeStamp })
-        let { keyToElement } = this.get()
         try {
           keyToElement(key).focus({
             preventScroll: true

src/routes/_components/status/Notification.html

@@ -1,6 +1,6 @@
 {#if status}
   <Status {index} {length} {timelineType} {timelineValue} {focusSelector}
-          {status} {notification} {shortcutScope} on:recalculateHeight
+          {status} {notification} {enableShortcuts} on:recalculateHeight
   />
 {:else}
   <article class={className}
@@ -8,17 +8,17 @@
            aria-posinset={index}
            aria-setsize={length}
            aria-label={ariaLabel}
-           focus-key={focusKey}
-           shortcut-key={shortcutScope}
+           focus-key={uuid}
+           shortcut-key={uuid}
            on:focus="onFocus()"
            on:blur="onBlur()"
            ref:article
   >
     <StatusHeader {notification} {notificationId} {status} {statusId} {timelineType}
                   {account} {accountId} {uuid} isStatusInNotification="true" />
-    {#if shortcutScope}
-      <Shortcut scope={shortcutScope} key="p" on:pressed="openAuthorProfile()" />
-      <Shortcut scope={shortcutScope} key="m" on:pressed="mentionAuthor()" />
+    {#if enableShortcuts}
+      <Shortcut scope={uuid} key="p" on:pressed="openAuthorProfile()" />
+      <Shortcut scope={uuid} key="m" on:pressed="mentionAuthor()" />
     {/if}
   </article>
 {/if}
@@ -57,7 +57,7 @@
       Shortcut
     },
     data: () => ({
-      shortcutScope: null
+      enableShortcuts: null
     }),
     store: () => store,
     computed: {
@@ -74,9 +74,9 @@
       ),
       className: ({ $underlineLinks }) => (classname(
         'notification-article',
+        'shortcut-list-item',
         $underlineLinks && 'underline-links'
-      )),
-      focusKey: ({ uuid }) => `notification-follower-${uuid}`
+      ))
     },
     methods: {
       openAuthorProfile () {

src/routes/_components/status/Status.html

@@ -1,8 +1,8 @@
 <article class={className}
          tabindex="0"
-         delegate-key={delegateKey}
-         focus-key={delegateKey}
-         shortcut-key={shortcutScope}
+         delegate-key={uuid}
+         focus-key={uuid}
+         shortcut-key={uuid}
          aria-posinset={index}
          aria-setsize={length}
          aria-label={ariaLabel}
@@ -40,10 +40,10 @@
     <StatusComposeBox {...params} on:recalculateHeight />
   {/if}
 </article>
-{#if shortcutScope}
-  <Shortcut scope={shortcutScope} key="o" on:pressed="open()" />
-  <Shortcut scope={shortcutScope} key="p" on:pressed="openAuthorProfile()" />
-  <Shortcut scope={shortcutScope} key="m" on:pressed="mentionAuthor()" />
+{#if enableShortcuts}
+  <Shortcut scope={uuid} key="o" on:pressed="open()" />
+  <Shortcut scope={uuid} key="p" on:pressed="openAuthorProfile()" />
+  <Shortcut scope={uuid} key="m" on:pressed="mentionAuthor()" />
 {/if}
 
 <style>
@@ -146,11 +146,11 @@
 
   export default {
     oncreate () {
-      let { delegateKey, isStatusInOwnThread, showContent } = this.get()
+      let { uuid, isStatusInOwnThread, showContent } = this.get()
       let { disableTapOnStatus } = this.store.get()
       if (!isStatusInOwnThread && !disableTapOnStatus) {
         // the whole <article> is clickable in this case
-        registerClickDelegate(this, delegateKey, (e) => this.onClickOrKeydown(e))
+        registerClickDelegate(this, uuid, (e) => this.onClickOrKeydown(e))
       }
       if (!showContent) {
         scheduleIdleTask(() => {
@@ -179,7 +179,7 @@
       notification: void 0,
       replyVisibility: void 0,
       contentPreloaded: false,
-      shortcutScope: null
+      enableShortcuts: null
     }),
     store: () => store,
     methods: {
@@ -248,7 +248,6 @@
       uuid: ({ $currentInstance, timelineType, timelineValue, notificationId, statusId }) => (
         `${$currentInstance}/${timelineType}/${timelineValue}/${notificationId || ''}/${statusId}`
       ),
-      delegateKey: ({ uuid }) => `status-${uuid}`,
       isStatusInOwnThread: ({ timelineType, timelineValue, originalStatusId }) => (
         (timelineType === 'status' || timelineType === 'reply') && timelineValue === originalStatusId
       ),
@@ -285,6 +284,7 @@
       ),
       className: ({ visibility, timelineType, isStatusInOwnThread, $underlineLinks, $disableTapOnStatus }) => (classname(
         'status-article',
+        'shortcut-list-item',
         visibility === 'direct' && 'status-direct',
         timelineType !== 'search' && 'status-in-timeline',
         isStatusInOwnThread && 'status-in-own-thread',
@@ -297,7 +297,7 @@
         account, accountId, uuid, isStatusInNotification, isStatusInOwnThread,
         originalAccount, originalAccountId, spoilerShown, visibility, replyShown,
         replyVisibility, spoilerText, originalStatus, originalStatusId, inReplyToId,
-        createdAtDate, timeagoFormattedDate, shortcutScope, absoluteFormattedDate }) => ({
+        createdAtDate, timeagoFormattedDate, enableShortcuts, absoluteFormattedDate }) => ({
         notification,
         notificationId,
         status,
@@ -320,7 +320,7 @@
         inReplyToId,
         createdAtDate,
         timeagoFormattedDate,
-        shortcutScope,
+        enableShortcuts,
         absoluteFormattedDate
       })
     }

src/routes/_components/status/StatusMediaAttachments.html

@@ -31,8 +31,8 @@
   {/if}
   </div>
 </div>
-{#if shortcutScope}
-<Shortcut scope="{shortcutScope}" key="y" on:pressed="toggleSensitiveMedia()"/>
+{#if enableShortcuts}
+  <Shortcut scope={uuid} key="y" on:pressed="toggleSensitiveMedia()"/>
 {/if}
 {:else}
   <MediaAttachments {mediaAttachments} {sensitive} {uuid} />

src/routes/_components/status/StatusSpoiler.html

@@ -6,8 +6,8 @@
     {spoilerShown ? 'Show less' : 'Show more'}
   </button>
 </div>
-{#if shortcutScope}
-<Shortcut scope="{shortcutScope}" key="x" on:pressed="toggleSpoilers()"/>
+{#if enableShortcuts}
+<Shortcut scope={uuid} key="x" on:pressed="toggleSpoilers()"/>
 {/if}
 <style>
   .status-spoiler {

src/routes/_components/status/StatusToolbar.html

@@ -31,10 +31,10 @@
     delegateKey={optionsKey}
   />
 </div>
-{#if shortcutScope}
-<Shortcut scope="{shortcutScope}" key="f" on:pressed="toggleFavorite()"/>
-<Shortcut scope="{shortcutScope}" key="r" on:pressed="reply()"/>
-<Shortcut scope="{shortcutScope}" key="b" on:pressed="reblog()"/>
+{#if enableShortcuts}
+  <Shortcut scope={uuid} key="f" on:pressed="toggleFavorite()"/>
+  <Shortcut scope={uuid} key="r" on:pressed="reply()"/>
+  <Shortcut scope={uuid} key="b" on:pressed="reblog()"/>
 {/if}
 <style>
   .status-toolbar {

src/routes/_components/timeline/NotificationVirtualListItem.html

@@ -3,7 +3,7 @@
   timelineType={virtualProps.timelineType}
   timelineValue={virtualProps.timelineValue}
   focusSelector={virtualProps.focusSelector}
-  shortcutScope={virtualKey}
+  enableShortcuts={true}
   index={virtualIndex}
   length={virtualLength}
   on:recalculateHeight />

src/routes/_components/timeline/PinnedStatuses.html

@@ -2,15 +2,24 @@
   <h1 class="sr-only">Pinned statuses</h1>
   <div role="feed" aria-label="Pinned statuses" class="pinned-statuses">
     {#each pinnedStatuses as status, index (status.id)}
+      <div class="pinned-status-wrapper">
+        <!-- empty div used because we assume the parent of the <article> gets the focus outline -->
         <Status {status}
                 timelineType="pinned"
                 timelineValue={accountId}
                 {index}
                 length={pinnedStatuses.length}
+                enableShortcuts={true}
         />
+      </div>
     {/each}
   </div>
 {/if}
+<style>
+  .pinned-status-wrapper:first-child {
+    margin: 2px 0; /* gives room for the focus outline */
+  }
+</style>
 <script>
   import { store } from '../../_store/store'
   import Status from '../status/Status.html'

src/routes/_components/timeline/StatusVirtualListItem.html

@@ -2,7 +2,7 @@
         timelineType={virtualProps.timelineType}
         timelineValue={virtualProps.timelineValue}
         focusSelector={virtualProps.focusSelector}
-        shortcutScope={virtualKey}
+        enableShortcuts={true}
         index={virtualIndex}
         length={virtualLength}
         on:recalculateHeight />

src/routes/_components/timeline/Timeline.html

@@ -35,11 +35,13 @@
     <div>Error: component failed to load! Try reloading. {error}</div>
   {/await}
 </div>
+<ScrollListShortcuts />
 <script>
   import { store } from '../../_store/store'
   import Status from '../status/Status.html'
   import LoadingFooter from './LoadingFooter.html'
   import MoreHeaderVirtualWrapper from './MoreHeaderVirtualWrapper.html'
+  import ScrollListShortcuts from '../shortcut/ScrollListShortcuts.html'
   import {
     importVirtualList,
     importList,
@@ -293,6 +295,9 @@
         console.log('timeline preinitialized')
         this.store.set({ timelinePreinitialized: true })
       }
+    },
+    components: {
+      ScrollListShortcuts
     }
   }
 </script>

src/routes/_components/virtualList/VirtualList.html

@@ -18,7 +18,6 @@
     {/if}
   </div>
 </VirtualListContainer>
-<ScrollListShortcuts items={visibleItemKeys} />
 <style>
   .virtual-list {
     position: relative;
@@ -29,7 +28,6 @@
   import VirtualListLazyItem from './VirtualListLazyItem'
   import VirtualListFooter from './VirtualListFooter.html'
   import VirtualListHeader from './VirtualListHeader.html'
-  import ScrollListShortcuts from '../shortcut/ScrollListShortcuts.html'
   import { virtualListStore } from './virtualListStore'
   import throttle from 'lodash-es/throttle'
   import { mark, stop } from '../../_utils/marks'
@@ -101,8 +99,7 @@
       VirtualListContainer,
       VirtualListLazyItem,
       VirtualListFooter,
-      VirtualListHeader,
-      ScrollListShortcuts
+      VirtualListHeader
     },
     computed: {
       distanceFromBottom: ({ $scrollHeight, $scrollTop, $offsetHeight }) => {

src/routes/_utils/scrollIntoView.js

@@ -21,15 +21,15 @@ export function isVisible (element) {
   return rect.top < offsetHeight && rect.bottom >= topOverlay
 }
 
-export function firstVisibleElementIndex (items, itemElementFunction) {
+export function firstVisibleElementIndex (elements) {
   let offsetHeight = getOffsetHeight()
   let topOverlay = getTopOverlay()
   let first = -1
   let firstComplete = -1
-  let len = items.length
+  let len = elements.length
   let i = -1
   while (++i < len) {
-    let element = itemElementFunction(items[i])
+    let element = elements[i]
     if (!element) {
       continue
     }

tests/spec/010-focus.js

@@ -1,7 +1,7 @@
 import {
-  getNthStatus, scrollToStatus, closeDialogButton, modalDialogContents, getActiveElementClass, goBack, getUrl,
+  getNthStatus, scrollToStatus, closeDialogButton, modalDialogContents, goBack, getUrl,
   goBackButton, getActiveElementInnerText, getNthReplyButton, getActiveElementInsideNthStatus, focus,
-  getNthStatusSelector, getActiveElementTagName
+  getNthStatusSelector, getActiveElementTagName, getActiveElementClassList
 } from '../utils'
 import { loginAsFoobar } from '../roles'
 import { Selector as $ } from 'testcafe'
@@ -23,7 +23,7 @@ test('modal preserves focus', async t => {
   await t.click($(`${getNthStatusSelector(idx)} .play-video-button`))
     .click(closeDialogButton)
     .expect(modalDialogContents.exists).notOk()
-    .expect(getActiveElementClass()).contains('play-video-button')
+    .expect(getActiveElementClassList()).contains('play-video-button')
     .expect(getActiveElementInsideNthStatus()).eql(idx.toString())
 })
 
@@ -37,7 +37,8 @@ test('timeline preserves focus', async t => {
 
   await goBack()
   await t.expect(getUrl()).eql('http://localhost:4002/')
-    .expect(getActiveElementClass()).contains('status-article status-in-timeline')
+    .expect(getActiveElementClassList()).contains('status-article')
+    .expect(getActiveElementClassList()).contains('status-in-timeline')
     .expect(getActiveElementInsideNthStatus()).eql('0')
 })
 
@@ -55,7 +56,7 @@ test('timeline link preserves focus', async t => {
     .expect(getUrl()).contains('/accounts/')
     .click(goBackButton)
     .expect(getUrl()).eql('http://localhost:4002/')
-    .expect(getActiveElementClass()).contains('status-sidebar')
+    .expect(getActiveElementClassList()).contains('status-sidebar')
     .expect(getActiveElementInsideNthStatus()).eql('0')
 })
 
@@ -73,8 +74,7 @@ test('notification timeline preserves focus', async t => {
     .expect(getActiveElementInsideNthStatus()).eql('5')
 })
 
-// TODO: this test is really flakey in CI for some reason
-test.skip('thread preserves focus', async t => {
+test('thread preserves focus', async t => {
   await loginAsFoobar(t)
   await t
     .navigateTo('/accounts/3')
@@ -87,14 +87,15 @@ test.skip('thread preserves focus', async t => {
     .click(goBackButton)
     .expect(getUrl()).contains('/statuses/')
     .expect(getNthStatus(24).exists).ok()
-    .expect(getActiveElementClass()).contains('status-sidebar')
+    .expect(getActiveElementClassList()).contains('status-sidebar')
     .expect(getActiveElementInsideNthStatus()).eql('24')
     .hover(getNthStatus(23))
     .click(getNthStatus(23))
     .expect($(`${getNthStatusSelector(23)} .status-absolute-date`).exists).ok()
   await goBack()
   await t.expect($(`${getNthStatusSelector(24)} .status-absolute-date`).exists).ok()
-    .expect(getActiveElementClass()).contains('status-article status-in-timeline')
+    .expect(getActiveElementClassList()).contains('status-article')
+    .expect(getActiveElementClassList()).contains('status-in-timeline')
     .expect(getActiveElementInsideNthStatus()).eql('23')
 })
 
@@ -103,7 +104,7 @@ test('reply preserves focus and moves focus to the text input', async t => {
   await t
     .expect(getNthStatus(1).exists).ok({ timeout: 20000 })
     .click(getNthReplyButton(1))
-    .expect(getActiveElementClass()).contains('compose-box-input')
+    .expect(getActiveElementClassList()).contains('compose-box-input')
 })
 
 test('focus main content element on index page load', async t => {

tests/spec/025-shortcuts-status.js

@@ -9,7 +9,7 @@ import {
   getNthStatusSpoiler,
   getUrl, modalDialog,
   scrollToStatus,
-  isNthStatusActive, getActiveElementRectTop, scrollToTop
+  isNthStatusActive, getActiveElementRectTop, scrollToTop, isActiveStatusPinned
 } from '../utils'
 import { homeTimeline } from '../fixtures'
 import { loginAsFoobar } from '../roles'
@@ -183,3 +183,34 @@ test('Shortcut j/k change the active status on a thread', async t => {
     .expect(isNthStatusActive(2)()).notOk()
     .expect(isNthStatusActive(3)()).notOk()
 })
+
+test('Shortcut j/k change the active status on pinned statuses', async t => {
+  await loginAsFoobar(t)
+  await t
+    .click($('a').withText('quux'))
+    .expect(getUrl()).contains('/accounts')
+  await t
+    .expect(getNthStatus(0).exists).ok({ timeout: 30000 })
+    .expect(isNthStatusActive(0)()).notOk()
+    .pressKey('j')
+    .expect(isNthStatusActive(0)()).ok()
+    .expect(isActiveStatusPinned()).eql(true)
+    .pressKey('j')
+    .expect(isNthStatusActive(1)()).ok()
+    .expect(isActiveStatusPinned()).eql(true)
+    .pressKey('j')
+    .expect(isNthStatusActive(0)()).ok()
+    .expect(isActiveStatusPinned()).eql(false)
+    .pressKey('j')
+    .expect(isNthStatusActive(1)()).ok()
+    .expect(isActiveStatusPinned()).eql(false)
+    .pressKey('k')
+    .expect(isNthStatusActive(0)()).ok()
+    .expect(isActiveStatusPinned()).eql(false)
+    .pressKey('k')
+    .expect(isNthStatusActive(1)()).ok()
+    .expect(isActiveStatusPinned()).eql(true)
+    .pressKey('k')
+    .expect(isNthStatusActive(0)()).ok()
+    .expect(isActiveStatusPinned()).eql(true)
+})

tests/spec/111-focus.js

@@ -1,5 +1,5 @@
 import {
-  composeInput, getActiveElementClass,
+  composeInput, getActiveElementClassList,
   getNthComposeReplyButton,
   getNthComposeReplyInput, getNthReplyButton,
   getNthStatusSelector
@@ -19,5 +19,5 @@ test('replying to a toot returns focus to reply button', async t => {
     .click(getNthReplyButton(0))
     .typeText(getNthComposeReplyInput(0), 'How strange was it?', { paste: true })
     .click(getNthComposeReplyButton(0))
-    .expect(getActiveElementClass()).contains('status-toolbar-reply-button', { timeout: 20000 })
+    .expect(getActiveElementClassList()).contains('status-toolbar-reply-button', { timeout: 20000 })
 })

tests/utils.js

@@ -76,8 +76,8 @@ export const sleep = timeout => new Promise(resolve => setTimeout(resolve, timeo
 
 export const getUrl = exec(() => window.location.href)
 
-export const getActiveElementClass = exec(() =>
-  (document.activeElement && document.activeElement.getAttribute('class')) || ''
+export const getActiveElementClassList = exec(() =>
+  (document.activeElement && (document.activeElement.getAttribute('class') || '').split(/\s+/)) || []
 )
 
 export const getActiveElementTagName = exec(() =>
@@ -162,6 +162,11 @@ export const isNthStatusActive = (idx) => (exec(() => {
   dependencies: { idx }
 }))
 
+export const isActiveStatusPinned = exec(() => {
+  return document.activeElement &&
+    document.activeElement.getAttribute('delegate-key').includes('pinned')
+})
+
 export const scrollToBottom = exec(() => {
   document.scrollingElement.scrollTop = document.scrollingElement.scrollHeight
 })