fix: align shift-click range selection in admin lists and files table

packages/web-app-admin-settings/src/components/Groups/GroupsList.vue

@@ -182,7 +182,12 @@ const {
 
 const keyActions = useKeyboardActions()
 useKeyboardTableNavigation(keyActions, paginatedItems, selectedGroups, lastSelectedGroupId)
-useKeyboardTableMouseActions(keyActions, paginatedItems, selectedGroups, lastSelectedGroupId)
+const { setSelectionStart } = useKeyboardTableMouseActions(
+  keyActions,
+  paginatedItems,
+  selectedGroups,
+  lastSelectedGroupId
+)
 
 const readOnlyLabel = computed(() => $gettext("This group is read-only and can't be edited"))
 // the selection is kept across pages, so only the groups of the current page count
@@ -229,8 +234,7 @@ function isGroupSelected(group: Group) {
 }
 
 function selectGroup(group: Group) {
-  lastSelectedGroupId.value = group.id
-  keyActions.resetSelectionCursor()
+  setSelectionStart(group)
 
   if (!isGroupSelected(group)) {
     return groupSettingsStore.addSelectedGroup(group)
@@ -302,6 +306,7 @@ function showContextMenuOnRightClick(event: MouseEvent, group: Group) {
   event.preventDefault()
   if (!isGroupSelected(group)) {
     groupSettingsStore.setSelectedGroups([group])
+    setSelectionStart(group)
   }
   const anchorElement = createVirtualCursorElement(event)
   unref(contextMenuDrops)[group.id]?.show({ anchorElement })

packages/web-app-admin-settings/src/components/Spaces/SpacesList.vue

@@ -228,7 +228,12 @@ const {
 
 const keyActions = useKeyboardActions()
 useKeyboardTableNavigation(keyActions, paginatedItems, selectedSpaces, lastSelectedSpaceId)
-useKeyboardTableMouseActions(keyActions, paginatedItems, selectedSpaces, lastSelectedSpaceId)
+const { setSelectionStart } = useKeyboardTableMouseActions(
+  keyActions,
+  paginatedItems,
+  selectedSpaces,
+  lastSelectedSpaceId
+)
 
 const { loadPreview } = useLoadPreview()
 
@@ -436,6 +441,7 @@ function showContextMenuOnRightClick(event: MouseEvent, space: SpaceResource) {
   event.preventDefault()
   if (!isSpaceSelected(space)) {
     spaceSettingsStore.setSelectedSpaces([space])
+    setSelectionStart(space)
   }
   const anchorElement = createVirtualCursorElement(event as MouseEvent)
   unref(contextMenuDrops)[space.id]?.show({ anchorElement })
@@ -451,8 +457,7 @@ function showDetailsForSpace(space: SpaceResource) {
 }
 
 function selectSpace(selectedSpace: SpaceResource) {
-  lastSelectedSpaceId.value = selectedSpace.id
-  keyActions.resetSelectionCursor()
+  setSelectionStart(selectedSpace)
 
   const isSpaceSelected = unref(selectedSpaces).find((space) => space.id === selectedSpace.id)
   if (!isSpaceSelected) {

packages/web-app-admin-settings/src/components/Users/UsersList.vue

@@ -219,7 +219,12 @@ const {
 
 const keyActions = useKeyboardActions()
 useKeyboardTableNavigation(keyActions, paginatedItems, selectedUsers, lastSelectedUserId)
-useKeyboardTableMouseActions(keyActions, paginatedItems, selectedUsers, lastSelectedUserId)
+const { setSelectionStart } = useKeyboardTableMouseActions(
+  keyActions,
+  paginatedItems,
+  selectedUsers,
+  lastSelectedUserId
+)
 
 // the selection is kept across pages, so only the users of the current page count
 const allUsersSelected = computed(
@@ -290,8 +295,7 @@ function isUserSelected(user: User) {
 }
 
 function selectUser(user: User) {
-  lastSelectedUserId.value = user.id
-  keyActions.resetSelectionCursor()
+  setSelectionStart(user)
 
   if (!isUserSelected(user)) {
     return userSettingsStore.addSelectedUser(user)
@@ -373,6 +377,7 @@ function showContextMenuOnRightClick(event: MouseEvent, user: User) {
   event.preventDefault()
   if (!isUserSelected(user)) {
     userSettingsStore.setSelectedUsers([user])
+    setSelectionStart(user)
   }
   const anchorElement = createVirtualCursorElement(event)
   unref(contextMenuDrops)[user.id]?.show({ anchorElement })

packages/web-app-admin-settings/src/composables/keyboardActions/useKeyboardTableMouseActions.ts

@@ -1,6 +1,6 @@
 import { onBeforeUnmount, onMounted, unref, Ref } from 'vue'
 import { eventBus, KeyboardActions } from '@opencloud-eu/web-pkg'
-import { findIndex, find } from 'lodash-es'
+import { findIndex } from 'lodash-es'
 import { Item, Resource } from '@opencloud-eu/web-client'
 
 export function useKeyboardTableMouseActions(
@@ -11,6 +11,14 @@ export function useKeyboardTableMouseActions(
 ) {
   let resourceListClickedMetaEvent: string
   let resourceListClickedShiftEvent: string
+  let shiftSelectionAnchorId: string | null = null
+
+  // makes the item the start of the next shift-click or shift + arrow range
+  function setSelectionStart(item: Item) {
+    lastSelectedRowId.value = String(item.id)
+    keyActions.resetSelectionCursor()
+    shiftSelectionAnchorId = null
+  }
 
   function handleCtrlClickAction(resource: Resource) {
     const rowIndex = findIndex(unref(selectedRows), { id: resource.id })
@@ -19,39 +27,45 @@ export function useKeyboardTableMouseActions(
     } else {
       selectedRows.value.push(resource)
     }
-    keyActions.resetSelectionCursor()
+    setSelectionStart(resource)
+  }
 
-    lastSelectedRowId.value = String(resource.id)
+  function getShiftSelectionAnchorId(resource: Item) {
+    const selectedIds = unref(selectedRows).map(({ id }) => id)
+    if (shiftSelectionAnchorId && selectedIds.includes(shiftSelectionAnchorId)) {
+      return shiftSelectionAnchorId
+    }
+    const latestId = unref(lastSelectedRowId)
+    if (selectedIds.includes(latestId)) {
+      return latestId
+    }
+    // unknown latest row, the range starts at the first row
+    if (!latestId && selectedIds.length) {
+      return null
+    }
+    return resource.id
   }
 
   function handleShiftClickAction({ resource }: { resource: Item }) {
-    const targetNode = document.querySelector(`[data-item-id='${resource.id}']`)
-    const parent = targetNode?.closest('tr') || targetNode?.parentElement
+    const resources = unref(paginatedResources)
+    const clickedIndex = resources.findIndex(({ id }) => id === resource.id)
+    if (clickedIndex === -1) {
+      return
+    }
 
-    const resourceNodes = Object.values(parent.parentNode.children)
-    const latestNode = resourceNodes.find(
-      (r) => r.getAttribute('data-item-id') === unref(lastSelectedRowId)
+    const anchorId = getShiftSelectionAnchorId(resource)
+    // anchor may be unknown or on another page
+    const anchorIndex = Math.max(
+      resources.findIndex(({ id }) => id === anchorId),
+      0
     )
-    const clickedNode = resourceNodes.find((r) => r.getAttribute('data-item-id') === resource.id)
-
-    let latestNodeIndex = resourceNodes.indexOf(latestNode)
-    latestNodeIndex = latestNodeIndex === -1 ? 0 : latestNodeIndex
-
-    const clickedNodeIndex = resourceNodes.indexOf(clickedNode)
-    const minIndex = Math.min(latestNodeIndex, clickedNodeIndex)
-    const maxIndex = Math.max(latestNodeIndex, clickedNodeIndex)
-
-    for (let i = minIndex; i <= maxIndex; i++) {
-      const nodeId = resourceNodes[i].getAttribute('data-item-id')
-      const selectedRowIndex = findIndex(unref(selectedRows), { id: nodeId })
-      if (selectedRowIndex === -1) {
-        const selectedRow = find(unref(paginatedResources), { id: nodeId })
-        selectedRows.value.push(selectedRow)
-      }
-    }
+    const minIndex = Math.min(anchorIndex, clickedIndex)
+    const maxIndex = Math.max(anchorIndex, clickedIndex)
+    selectedRows.value = resources.slice(minIndex, maxIndex + 1)
+    shiftSelectionAnchorId = resources[anchorIndex].id
 
     lastSelectedRowId.value = String(resource.id)
-    keyActions.selectionCursor.value = clickedNodeIndex - latestNodeIndex
+    keyActions.selectionCursor.value = clickedIndex - anchorIndex
   }
 
   onMounted(() => {
@@ -69,4 +83,6 @@ export function useKeyboardTableMouseActions(
     eventBus.unsubscribe('app.admin-settings.list.clicked.meta', resourceListClickedMetaEvent)
     eventBus.unsubscribe('app.admin-settings.list.clicked.shift', resourceListClickedShiftEvent)
   })
+
+  return { setSelectionStart }
 }

packages/web-app-admin-settings/tests/unit/components/Groups/GroupsList.spec.ts

@@ -171,6 +171,15 @@ describe('GroupsList', () => {
       document.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowUp', shiftKey: true }))
       expect(getSelectedIds()).toEqual(['1', '2'])
     })
+    it('starts the shift click range at a right clicked row', async () => {
+      const { wrapper } = getWrapper({ groups, realCheckboxes: true })
+      await getCheckboxInput(wrapper, groups[0]).trigger('click')
+      await wrapper.find(`tr[data-item-id="${groups[1].id}"]`).trigger('contextmenu')
+      expect(getSelectedIds()).toEqual(['2'])
+
+      await getCheckboxInput(wrapper, groups[2]).trigger('click', { shiftKey: true })
+      expect(getSelectedIds()).toEqual(['2', '3'])
+    })
   })
 })
 

packages/web-app-admin-settings/tests/unit/composables/keyboardActions/useKeyboardTableMouseActions.spec.ts

@@ -3,6 +3,7 @@ import { mock } from 'vitest-mock-extended'
 import { eventBus, Key, KeyboardActions, Modifier } from '@opencloud-eu/web-pkg'
 import { Item } from '@opencloud-eu/web-client'
 import { getComposableWrapper } from '@opencloud-eu/web-test-helpers'
+import { enableAutoUnmount } from '@vue/test-utils'
 import {
   useKeyboardTableMouseActions,
   useKeyboardTableNavigation
@@ -14,35 +15,73 @@ vi.mock('@opencloud-eu/web-pkg', async (importOriginal) => ({
   focusCheckbox: vi.fn()
 }))
 
-const items: Item[] = [{ id: '1' }, { id: '2' }, { id: '3' }] as Item[]
+enableAutoUnmount(afterEach)
 
-describe('useKeyboardTableMouseActions', () => {
-  beforeEach(() => {
-    document.body.innerHTML = `<table><tbody>${items
-      .map(({ id }) => `<tr data-item-id="${id}"><td>item ${id}</td></tr>`)
-      .join('')}</tbody></table>`
-  })
+const items: Item[] = [{ id: '1' }, { id: '2' }, { id: '3' }, { id: '4' }, { id: '5' }] as Item[]
+
+function shiftClick(index: number) {
+  eventBus.publish('app.admin-settings.list.clicked.shift', { resource: items[index] })
+}
 
+describe('useKeyboardTableMouseActions', () => {
   describe('shift click', () => {
     it('selects the range between the last selected and the clicked row', () => {
-      const { selectedRows } = getWrapper()
-      eventBus.publish('app.admin-settings.list.clicked.shift', {
-        resource: items[2]
-      })
-      expect(selectedRows.value.map(({ id }) => id)).toEqual(['1', '2', '3'])
+      const { selectedIds } = getWrapper()
+      shiftClick(2)
+      expect(selectedIds()).toEqual(['1', '2', '3'])
+    })
+    it('keeps the anchor fixed on consecutive shift clicks', () => {
+      const { selectedIds } = getWrapper()
+      shiftClick(2)
+      shiftClick(4)
+      expect(selectedIds()).toEqual(['1', '2', '3', '4', '5'])
+      shiftClick(1)
+      expect(selectedIds()).toEqual(['1', '2'])
+    })
+    it('replaces the existing selection with the range', () => {
+      const { selectedIds, selectedRows } = getWrapper(2)
+      selectedRows.value.push(items[4])
+      shiftClick(0)
+      expect(selectedIds()).toEqual(['1', '2', '3'])
+    })
+    it('uses the first row as anchor if the last selected row is unknown', () => {
+      const { selectedIds, selectedRows, lastSelectedRowId } = getWrapper(1)
+      selectedRows.value = [items[1], items[3]]
+      lastSelectedRowId.value = null
+      shiftClick(2)
+      expect(selectedIds()).toEqual(['1', '2', '3'])
+    })
+    it('uses the clicked row as anchor if nothing is selected', () => {
+      const { selectedIds, selectedRows } = getWrapper()
+      selectedRows.value = []
+      shiftClick(2)
+      expect(selectedIds()).toEqual(['3'])
+    })
+    it('starts the range at the row passed to setSelectionStart', () => {
+      const { selectedIds, selectedRows, setSelectionStart } = getWrapper()
+      shiftClick(2)
+      selectedRows.value.push(items[3])
+      setSelectionStart(items[3])
+      shiftClick(4)
+      expect(selectedIds()).toEqual(['4', '5'])
+    })
+    it('moves the anchor to the ctrl clicked row', () => {
+      const { selectedIds } = getWrapper()
+      shiftClick(1)
+      eventBus.publish('app.admin-settings.list.clicked.meta', items[3])
+      shiftClick(4)
+      expect(selectedIds()).toEqual(['4', '5'])
     })
     it.each([
       { anchor: 0, target: 2, keys: [Key.ArrowUp, Key.ArrowDown] },
       { anchor: 2, target: 0, keys: [Key.ArrowDown, Key.ArrowUp] }
     ])(
       'keeps a contiguous range on shift + arrow afterwards (row $anchor to row $target)',
       ({ anchor, target, keys }) => {
-        const { selectedRows, triggerKey } = getWrapper(anchor)
-        eventBus.publish('app.admin-settings.list.clicked.shift', {
-          resource: items[target]
-        })
+        const { selectedIds, triggerKey } = getWrapper(anchor)
+        shiftClick(target)
         keys.forEach((key) => triggerKey(key, Modifier.Shift))
-        expect(selectedRows.value.map(({ id }) => id).sort()).toEqual(['1', '2', '3'])
+        expect(selectedIds().sort()).toEqual(['1', '2', '3'])
       }
     )
   })
@@ -52,20 +91,33 @@ function getWrapper(anchor = 0) {
   const selectedRows = ref<Item[]>([items[anchor]])
   const lastSelectedRowId = ref(items[anchor].id)
   const callbacks: Record<string, (event?: KeyboardEvent) => void> = {}
+  const selectionCursor = ref(0)
   const keyActions = mock<KeyboardActions>({
-    selectionCursor: ref(0),
+    selectionCursor,
+    resetSelectionCursor: () => {
+      selectionCursor.value = 0
+    },
     bindKeyAction: (keys, callback) => {
       callbacks[`${keys.modifier || ''}${keys.primary}`] = callback
       return ''
     }
   })
+  let mouseActions: ReturnType<typeof useKeyboardTableMouseActions>
   const wrapper = getComposableWrapper(() => {
-    useKeyboardTableMouseActions(keyActions, ref(items), selectedRows, lastSelectedRowId)
+    mouseActions = useKeyboardTableMouseActions(
+      keyActions,
+      ref(items),
+      selectedRows,
+      lastSelectedRowId
+    )
     useKeyboardTableNavigation(keyActions, ref(items), selectedRows, lastSelectedRowId)
   })
   return {
     wrapper,
+    ...mouseActions,
     selectedRows,
+    lastSelectedRowId,
+    selectedIds: () => selectedRows.value.map(({ id }) => id),
     triggerKey: (primary: Key, modifier?: Modifier) => callbacks[`${modifier || ''}${primary}`]()
   }
 }

packages/web-app-files/src/composables/keyboardActions/useKeyboardFileMouseActions.ts

@@ -21,10 +21,12 @@ export const useKeyboardFileMouseActions = (
 
   const handleCtrlClickAction = (resource: Resource) => {
     resourcesStore.toggleSelection(resource.id)
+    keyActions.resetSelectionCursor()
+    shiftSelectionAnchorId = null
   }
 
   const getShiftSelectionAnchorId = (resource: Resource) => {
-    if (shiftSelectionAnchorId) {
+    if (shiftSelectionAnchorId && unref(selectedIds).includes(shiftSelectionAnchorId)) {
       return shiftSelectionAnchorId
     }
 
@@ -71,6 +73,16 @@ export const useKeyboardFileMouseActions = (
     }
 
     resourcesStore.setLastSelectedId(resource.id)
+
+    // lets shift + arrow keys shrink or extend the range from the anchor.
+    // shift + arrow skips processing rows, so only count rows it can land on.
+    const resourcesById = new Map(resourcesStore.resources.map((r) => [r.id, r]))
+    const steps = resourceNodes
+      .slice(minIndex + 1, maxIndex + 1)
+      .filter(
+        (node) => resourcesById.get(node.getAttribute('data-item-id'))?.processing !== true
+      ).length
+    keyActions.selectionCursor.value = clickedIndex < anchorIndex ? -steps : steps
   }
 
   const handleTilesShiftClickAction = ({