fix: j/k shortcuts correctly set active element (#1028)

fixes #1018
This commit is contained in:
Nolan Lawson 2019-02-21 23:50:27 -08:00 committed by GitHub
parent 5d703e9612
commit 42e466f3c2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 79 additions and 68 deletions

View file

@ -5,13 +5,5 @@
virtualIndex={index}
virtualLength={length}
virtualKey={key}
active={active}
/>
</div>
<script>
export default {
computed: {
'active': ({ $activeItem, key }) => $activeItem === key
}
}
</script>

View file

@ -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') {

View file

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

View file

@ -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 () {

View file

@ -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'
)),

View file

@ -6,7 +6,6 @@
shortcutScope={virtualKey}
index={virtualIndex}
length={virtualLength}
{active}
on:recalculateHeight />
<script>
import Notification from '../status/Notification.html'

View file

@ -5,7 +5,6 @@
shortcutScope={virtualKey}
index={virtualIndex}
length={virtualLength}
active={active}
on:recalculateHeight />
<script>
import Status from '../status/Status.html'

View file

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

View file

@ -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 () {

View file

@ -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'],

View file

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

View file

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

View file

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

View file

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