fix: keyboard actions for deactivated resources

Prevents actions for disabled resources from being accessible via keyboard navigation. This is achieved by either disabling them (checkboxes) or hiding them (rename action, quick actions).
owncloud · Aug 8, 2024 · 8900fea · 8900fea
1 parent fa528a6
commit 8900fea
Show file tree

Hide file tree

Showing 8 changed files with 105 additions and 14 deletions.
diff --git a/changelog/unreleased/bugfix-keyboard-actions-for-disabled-resources b/changelog/unreleased/bugfix-keyboard-actions-for-disabled-resources
@@ -0,0 +1,6 @@
+Bugfix: Keyboard actions for disabled resources
+
+We've fixed an issue where certain actions such as rename or select were still possible for disabled resources when navigating via keyboard.
+
+https://github.com/owncloud/web/pull/11342
+https://github.com/owncloud/web/issues/11335
diff --git a/packages/design-system/src/components/OcStatusIndicators/OcStatusIndicators.spec.ts b/packages/design-system/src/components/OcStatusIndicators/OcStatusIndicators.spec.ts
@@ -46,4 +46,17 @@ describe('OcStatusIndicators', () => {
     })
     expect(wrapper.find(`#${indicator.id}`).exists()).toBeTruthy()
   })
+  it('does not render a button if disableHandler is set', () => {
+    const wrapper = mount(StatusIndicators, {
+      props: {
+        resource: fileResource,
+        indicators: [indicator],
+        disableHandler: true
+      },
+      global: {
+        plugins: [...defaultPlugins()]
+      }
+    })
+    expect(wrapper.find('button').exists()).toBeFalsy()
+  })
 })
diff --git a/packages/design-system/src/components/OcStatusIndicators/OcStatusIndicators.vue b/packages/design-system/src/components/OcStatusIndicators/OcStatusIndicators.vue
@@ -2,7 +2,7 @@
   <div class="oc-status-indicators">
     <template v-for="indicator in indicators">
       <oc-button
-        v-if="hasHandler(indicator)"
+        v-if="hasHandler(indicator) && !disableHandler"
         :id="indicator.id"
         :key="`${indicator.id}-handler`"
         v-oc-tooltip="$gettext(indicator.label)"
@@ -97,6 +97,13 @@ export default defineComponent({
     indicators: {
       type: Array as PropType<Indicator[]>,
       required: true
+    },
+    /**
+     * Disables the handler for all indicators. This is useful e.g. for disabled resources.
+     */
+    disableHandler: {
+      type: Boolean,
+      default: false
     }
   },
 

diff --git a/packages/web-pkg/src/components/FilesList/ResourceTable.vue b/packages/web-pkg/src/components/FilesList/ResourceTable.vue
@@ -41,6 +41,7 @@
         <oc-checkbox
           id="resource-table-select-all"
           size="large"
+          :disabled="resources.length === disabledResources.length"
           :label="allResourcesCheckboxLabel"
           :hide-label="true"
           :model-value="areAllResourcesSelected"
@@ -54,6 +55,7 @@
         :label="getResourceCheckboxLabel(item)"
         :hide-label="true"
         size="large"
+        :disabled="isResourceDisabled(item)"
         :model-value="isResourceSelected(item)"
         :outline="isLatestSelectedItem(item)"
         @click.stop="toggleSelection(item.id)"
@@ -155,6 +157,7 @@
         v-if="item.indicators.length"
         :resource="item"
         :indicators="item.indicators"
+        :disable-handler="isResourceDisabled(item)"
       />
     </template>
     <template #sdate="{ item }">
@@ -204,7 +207,7 @@
       </oc-button>
     </template>
     <template #actions="{ item }">
-      <div class="resource-table-actions">
+      <div v-if="!isResourceDisabled(item)" class="resource-table-actions">
         <!-- @slot Add quick actions before the `context-menu / three dot` button in the actions column -->
         <slot name="quickActions" :resource="item" />
         <context-menu-quick-action

diff --git a/packages/web-pkg/src/components/FilesList/ResourceTiles.vue b/packages/web-pkg/src/components/FilesList/ResourceTiles.vue
@@ -68,6 +68,7 @@
               :hide-label="true"
               size="large"
               class="oc-flex-inline oc-p-s"
+              :disabled="isResourceDisabled(resource)"
               :model-value="isResourceSelected(resource)"
               @click.stop.prevent="toggleTile([resource, $event])"
             />
@@ -87,6 +88,7 @@
           </template>
           <template #contextMenu>
             <context-menu-quick-action
+              v-if="!isResourceDisabled(resource)"
               :ref="(el) => (tileRefs.dropBtns[resource.id] = el as ContextMenuQuickActionRef)"
               :item="resource"
               class="resource-tiles-btn-action-dropdown"
@@ -313,6 +315,10 @@ export default defineComponent({
     }
 
     const isResourceClickable = (resource: Resource) => {
+      if (isResourceDisabled(resource)) {
+        return false
+      }
+
       if (resource.isFolder) {
         return true
       }

diff --git a/packages/web-pkg/src/composables/actions/files/useFileActionsRename.ts b/packages/web-pkg/src/composables/actions/files/useFileActionsRename.ts
@@ -238,7 +238,7 @@ export const useFileActionsRename = () => {
         }
 
         const renameDisabled = resources.some((resource) => {
-          return !resource.canRename()
+          return !resource.canRename() || resource.processing
         })
         return !renameDisabled
       },

diff --git a/packages/web-pkg/tests/unit/components/FilesList/ResourceTable.spec.ts b/packages/web-pkg/tests/unit/components/FilesList/ResourceTable.spec.ts
@@ -16,9 +16,11 @@ import { displayPositionedDropdown } from '../../../../src/helpers/contextMenuDr
 import { eventBus } from '../../../../src/services/eventBus'
 import { SideBarEventTopics } from '../../../../src/composables/sideBar'
 import { mock } from 'vitest-mock-extended'
-import { computed } from 'vue'
+import { computed, ref } from 'vue'
 import { Identity } from '@ownclouders/web-client/graph/generated'
 import { describe } from 'vitest'
+import { useFileActionsRename } from '../../../../src/composables/actions/files'
+import { FileAction } from '../../../../src/composables/actions/types'
 
 const mockUseEmbedMode = vi.fn().mockReturnValue({
   isLocationPicker: computed(() => false),
@@ -36,6 +38,11 @@ vi.mock('../../../../src/composables/resources', async (importOriginal) => ({
   useCanBeOpenedWithSecureView: vi.fn()
 }))
 
+vi.mock('../../../../src/composables/actions/files', async (importOriginal) => ({
+  ...(await importOriginal<any>()),
+  useFileActionsRename: vi.fn()
+}))
+
 const router = {
   push: vi.fn(),
   afterEach: vi.fn(),
@@ -491,16 +498,15 @@ describe('ResourceTable', () => {
       expect(spyDisplayPositionedDropdown).toHaveBeenCalledTimes(1)
     })
 
-    it('does not emit select event on clicking the three-dot icon in table row of a disabled resource', async () => {
-      const spyDisplayPositionedDropdown = vi.mocked(displayPositionedDropdown)
+    it('does not show the three-dot icon in table row of a disabled resource', () => {
       const { wrapper } = getMountedWrapper({ addProcessingResources: true })
-      await wrapper
-        .find(
-          '.oc-tbody-tr-rainforest .oc-table-data-cell-actions .resource-table-btn-action-dropdown'
-        )
-        .trigger('click')
-      expect(wrapper.emitted('update:selectedIds')).toBeUndefined()
-      expect(spyDisplayPositionedDropdown).toHaveBeenCalledTimes(0)
+      expect(
+        wrapper
+          .find(
+            '.oc-tbody-tr-rainforest .oc-table-data-cell-actions .resource-table-btn-action-dropdown'
+          )
+          .exists()
+      ).toBeFalsy()
     })
 
     it('removes invalid chars from item ids for usage in html template', async () => {
@@ -601,19 +607,31 @@ describe('ResourceTable', () => {
       expect(wrapper.findAll('.resource-table-shared-with .oc-avatar').length).toBe(1)
     })
   })
+  describe('rename action', () => {
+    it('shows if available', () => {
+      const { wrapper } = getMountedWrapper()
+      expect(wrapper.find('.resource-table-edit-name').exists()).toBeTruthy()
+    })
+    it('does not show if not available', () => {
+      const { wrapper } = getMountedWrapper({ hasRenameAction: false })
+      expect(wrapper.find('.resource-table-edit-name').exists()).toBeFalsy()
+    })
+  })
 })
 
 function getMountedWrapper({
   props = {},
   userContextReady = true,
   addProcessingResources = false,
   canBeOpenedWithSecureView = true,
+  hasRenameAction = true,
   resources = resourcesWithAllFields
 }: {
   props?: PartialComponentProps<typeof ResourceTable>
   userContextReady?: boolean
   addProcessingResources?: boolean
   canBeOpenedWithSecureView?: boolean
+  hasRenameAction?: boolean
   resources?: Resource[]
 } = {}) {
   const capabilities = {
@@ -624,6 +642,12 @@ function getMountedWrapper({
     canBeOpenedWithSecureView: () => canBeOpenedWithSecureView
   })
 
+  vi.mocked(useFileActionsRename).mockReturnValue(
+    mock<ReturnType<typeof useFileActionsRename>>({
+      actions: ref([mock<FileAction>({ isVisible: () => hasRenameAction })])
+    })
+  )
+
   return {
     wrapper: mount(ResourceTable, {
       props: {

diff --git a/packages/web-pkg/tests/unit/components/FilesList/ResourceTiles.spec.ts b/packages/web-pkg/tests/unit/components/FilesList/ResourceTiles.spec.ts
@@ -7,7 +7,9 @@ import { ComponentPublicInstance, computed } from 'vue'
 import { extractDomSelector } from '@ownclouders/web-client'
 import OcDrop from 'design-system/src/components/OcDrop/OcDrop.vue'
 import { useCanBeOpenedWithSecureView } from '../../../../src/composables/resources'
+import { displayPositionedDropdown } from '../../../../src/helpers/contextMenuDropdown'
 
+vi.mock('../../../../src/helpers/contextMenuDropdown')
 vi.mock('../../../../src/composables/viewMode', async (importOriginal) => ({
   ...(await importOriginal<any>()),
   useTileSize: vi.fn().mockReturnValue({
@@ -65,7 +67,7 @@ const resources = [
     canRename: vi.fn(),
     getDomSelector: () => extractDomSelector('forest'),
     canDownload: () => true
-  }
+  } as Resource
 ]
 
 describe('ResourceTiles component', () => {
@@ -198,6 +200,36 @@ describe('ResourceTiles component', () => {
         expect(wrapper.emitted('fileDropped')).toBeDefined()
       })
     })
+    describe('context menu', () => {
+      it('triggers the positioned dropdown on click', async () => {
+        const spyDisplayPositionedDropdown = vi.mocked(displayPositionedDropdown)
+        const { wrapper } = getWrapper({ props: { resources } })
+        const btn = wrapper.find('.resource-tiles-btn-action-dropdown')
+        await btn.trigger('click')
+        expect(spyDisplayPositionedDropdown).toHaveBeenCalled()
+      })
+      it('does not show for disabled resources', () => {
+        const { wrapper } = getWrapper({
+          props: { resources: [{ ...resources[0], processing: true }] }
+        })
+        expect(wrapper.find('.resource-tiles-btn-action-dropdown').exists()).toBeFalsy()
+      })
+    })
+    describe('checkboxes', () => {
+      it('update the selected ids on click', async () => {
+        const { wrapper } = getWrapper({ props: { resources } })
+        const checkbox = wrapper.find('input[type="checkbox"]')
+        await checkbox.trigger('click')
+        expect(wrapper.emitted('update:selectedIds')).toBeTruthy()
+      })
+      it('are disabled for disabled resources', () => {
+        const { wrapper } = getWrapper({
+          props: { resources: [{ ...resources[0], processing: true }] }
+        })
+        const checkbox = wrapper.find('input[type="checkbox"]')
+        expect(Object.keys(checkbox.attributes())).toContain('disabled')
+      })
+    })
   })
 
   it.each([