fix(files): VirtualList rendering for scrolling calculations

Signed-off-by: skjnldsv <[email protected]>

Author: skjnldsv

apps/files/src/components/FilesListVirtual.vue

@@ -70,7 +70,6 @@
 import type { UserConfig } from '../types'
 import type { Node as NcNode } from '@nextcloud/files'
 import type { ComponentPublicInstance, PropType } from 'vue'
-import type { Location } from 'vue-router'
 
 import { Folder, Permission, View, getFileActions, FileType } from '@nextcloud/files'
 import { showError } from '@nextcloud/dialogs'
@@ -86,6 +85,7 @@ import { useFileListWidth } from '../composables/useFileListWidth.ts'
 import { useRouteParameters } from '../composables/useRouteParameters.ts'
 import { useSelectionStore } from '../store/selection.js'
 import { useUserConfigStore } from '../store/userconfig.ts'
+import logger from '../logger.ts'
 
 import FileEntry from './FileEntry.vue'
 import FileEntryGrid from './FileEntryGrid.vue'
@@ -95,7 +95,6 @@ import FilesListTableFooter from './FilesListTableFooter.vue'
 import FilesListTableHeader from './FilesListTableHeader.vue'
 import FilesListTableHeaderActions from './FilesListTableHeaderActions.vue'
 import VirtualList from './VirtualList.vue'
-import logger from '../logger.ts'
 
 export default defineComponent({
 	name: 'FilesListVirtual',
@@ -230,28 +229,17 @@ export default defineComponent({
 	watch: {
 		// If nodes gets populated and we have a fileId,
 		// an openFile or openDetails, we fire the appropriate actions.
-		isEmpty(isEmpty: boolean) {
-			if (isEmpty || !this.fileId) {
-				return
-			}
-
-			logger.debug('FilesListVirtual: nodes populated, checking for requested fileId, openFile or openDetails again', {
-				fileId: this.fileId,
-				openFile: this.openFile,
-				openDetails: this.openDetails,
-			})
-
-			if (this.openFile) {
-				this.handleOpenFile(this.fileId)
-			}
-
-			if (this.openDetails) {
-				this.openSidebarForFile(this.fileId)
-			}
-
-			if (this.fileId) {
-				this.scrollToFile(this.fileId, false)
-			}
+		isEmpty() {
+			this.handleOpenQueries()
+		},
+		fileId() {
+			this.handleOpenQueries()
+		},
+		openFile() {
+			this.handleOpenQueries()
+		},
+		openDetails() {
+			this.handleOpenQueries()
 		},
 	},
 
@@ -281,6 +269,33 @@ export default defineComponent({
 	},
 
 	methods: {
+		handleOpenQueries() {
+			// If the list is empty, or we don't have a fileId,
+			// there's nothing to be done.
+			if (this.isEmpty || !this.fileId) {
+				return
+			}
+
+			logger.debug('FilesListVirtual: checking for requested fileId, openFile or openDetails', {
+				nodes: this.nodes,
+				fileId: this.fileId,
+				openFile: this.openFile,
+				openDetails: this.openDetails,
+			})
+
+			if (this.openFile) {
+				this.handleOpenFile(this.fileId)
+			}
+
+			if (this.openDetails) {
+				this.openSidebarForFile(this.fileId)
+			}
+
+			if (this.fileId) {
+				this.scrollToFile(this.fileId, false)
+			}
+		},
+
 		openSidebarForFile(fileId) {
 			// Open the sidebar for the given URL fileid
 			// iif we just loaded the app.
@@ -306,6 +321,7 @@ export default defineComponent({
 				}
 
 				this.scrollToIndex = Math.max(0, index)
+				logger.debug('Scrolling to file ' + fileId, { fileId, index })
 			}
 		},
 
@@ -370,15 +386,13 @@ export default defineComponent({
 			}
 			// The file is either a folder or has no default action other than downloading
 			// in this case we need to open the details instead and remove the route from the history
-			const query = this.$route.query
-			delete query.openfile
-			query.opendetails = ''
-
 			logger.debug('Ignore `openfile` query and replacing with `opendetails` for ' + node.path, { node })
-			await this.$router.replace({
-				...(this.$route as Location),
-				query,
-			})
+			window.OCP.Files.Router.goToRoute(
+				null,
+				this.$route.params,
+				{ ...this.$route.query, openfile: undefined, opendetails: '' },
+				true, // silent update of the URL
+			)
 		},
 
 		onDragOver(event: DragEvent) {
@@ -525,6 +539,13 @@ export default defineComponent({
 				// Hide the table header below the overlay
 				margin-block-start: calc(-1 * var(--row-height));
 			}
+
+			// Visually hide the table when there are no files
+			&--hidden {
+				visibility: hidden;
+				z-index: -1;
+				opacity: 0;
+			}
 		}
 
 		.files-list__filters {

apps/files/src/components/VirtualList.vue

@@ -25,11 +25,13 @@
 			<slot name="empty" />
 		</div>
 
-		<table v-show="dataSources.length > 0"
-			:aria-hidden="dataSources.length === 0"
+		<table :aria-hidden="dataSources.length === 0"
 			:inert="dataSources.length === 0"
 			class="files-list__table"
-			:class="{ 'files-list__table--with-thead-overlay': !!$scopedSlots['header-overlay'] }">
+			:class="{
+				'files-list__table--with-thead-overlay': !!$scopedSlots['header-overlay'],
+				'files-list__table--hidden': dataSources.length === 0,
+			}">
 			<!-- Accessibility table caption for screen readers -->
 			<caption v-if="caption" class="hidden-visually">
 				{{ caption }}
@@ -318,7 +320,7 @@ export default defineComponent({
 
 	methods: {
 		scrollTo(index: number) {
-			if (!this.$el) {
+			if (!this.$el || this.index === index) {
 				return
 			}
 

cypress/e2e/files/FilesUtils.ts

@@ -293,6 +293,12 @@ export function enableGridMode() {
 export function calculateViewportHeight(rows: number): Cypress.Chainable<number> {
 	cy.visit('/apps/files')
 
+	cy.get('[data-cy-files-list]')
+		.should('be.visible')
+
+	cy.get('[data-cy-files-list-tbody] tr', { timeout: 5000 })
+		.and('be.visible')
+
 	return cy.get('[data-cy-files-list]')
 		.should('be.visible')
 		.then((filesList) => {

cypress/e2e/files/scrolling.cy.ts

@@ -3,36 +3,23 @@
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
 
-import type { User } from '@nextcloud/cypress'
 import { calculateViewportHeight, enableGridMode, getRowForFile } from './FilesUtils.ts'
 import { beFullyInViewport, notBeFullyInViewport } from '../core-utils.ts'
 
-describe('files: Scrolling to selected file in file list', { testIsolation: true }, () => {
+describe('files: Scrolling to selected file in file list', () => {
 	const fileIds = new Map<number, string>()
-	let user: User
 	let viewportHeight: number
 
 	before(() => {
-		cy.createRandomUser().then(($user) => {
-			user = $user
-
-			cy.rm(user, '/welcome.txt')
-			for (let i = 1; i <= 10; i++) {
-				cy.uploadContent(user, new Blob([]), 'text/plain', `/${i}.txt`)
-					.then((response) => fileIds.set(i, Number.parseInt(response.headers['oc-fileid']).toString()))
-			}
-
-			cy.login(user)
-			cy.viewport(1200, 800)
-			// Calculate height to ensure that those 10 elements can not be rendered in one list (only 6 will fit the screen)
-			calculateViewportHeight(6)
-				.then((height) => { viewportHeight = height })
-		})
+		initFilesAndViewport(fileIds)
+			.then((_viewportHeight) => {
+				cy.log(`Saving viewport height to ${_viewportHeight}px`)
+				viewportHeight = _viewportHeight
+			})
 	})
 
 	beforeEach(() => {
 		cy.viewport(1200, viewportHeight)
-		cy.login(user)
 	})
 
 	it('Can see first file in list', () => {
@@ -123,41 +110,17 @@ describe('files: Scrolling to selected file in file list', { testIsolation: true
 	}
 })
 
-describe('files: Scrolling to selected file in file list (GRID MODE)', { testIsolation: true }, () => {
+describe('files: Scrolling to selected file in file list (GRID MODE)', () => {
 	const fileIds = new Map<number, string>()
-	let user: User
 	let viewportHeight: number
 
 	before(() => {
-		cy.wrap(Cypress.automation('remote:debugger:protocol', {
-			command: 'Network.clearBrowserCache',
-		  }))
-
-		cy.createRandomUser().then(($user) => {
-			user = $user
-
-			cy.rm(user, '/welcome.txt')
-			for (let i = 1; i <= 12; i++) {
-				cy.uploadContent(user, new Blob([]), 'text/plain', `/${i}.txt`)
-					.then((response) => fileIds.set(i, Number.parseInt(response.headers['oc-fileid']).toString()))
-			}
-
-			// Set grid mode
-			cy.login(user)
-			cy.visit('/apps/files')
-			enableGridMode()
-
-			// 768px width will limit the columns to 3
-			cy.viewport(768, 800)
-			// Calculate height to ensure that those 12 elements can not be rendered in one list (only 3 will fit the screen)
-			calculateViewportHeight(3)
-				.then((height) => { viewportHeight = height })
-		})
+		initFilesAndViewport(fileIds, true)
+			.then((_viewportHeight) => { viewportHeight = _viewportHeight })
 	})
 
 	beforeEach(() => {
 		cy.viewport(768, viewportHeight)
-		cy.login(user)
 	})
 
 	// First row
@@ -288,3 +251,34 @@ function beOverlappedByTableHeader($el: JQuery<HTMLElement>, expected = true) {
 function notBeOverlappedByTableHeader($el: JQuery<HTMLElement>) {
 	return beOverlappedByTableHeader($el, false)
 }
+
+function initFilesAndViewport(fileIds: Map<number, string>, gridMode = false): Cypress.Chainable<number> {
+	return cy.createRandomUser().then((user) => {
+		cy.rm(user, '/welcome.txt')
+
+		// Create files with names 1.txt, 2.txt, ..., 10.txt
+		const count = gridMode ? 12 : 10
+		for (let i = 1; i <= count; i++) {
+			cy.uploadContent(user, new Blob([]), 'text/plain', `/${i}.txt`)
+				.then((response) => fileIds.set(i, Number.parseInt(response.headers['oc-fileid']).toString()))
+		}
+
+		cy.login(user)
+		cy.viewport(1200, 800)
+
+		cy.visit('/apps/files')
+
+		// If grid mode is requested, enable it
+		if (gridMode) {
+			enableGridMode()
+		}
+
+		// Calculate height to ensure that those 10 elements can not be rendered in one list (only 6 will fit the screen, 3 in grid mode)
+		return calculateViewportHeight(gridMode ? 3 : 6)
+			.then((height) => {
+				// Set viewport height to the calculated height
+				cy.log(`Setting viewport height to ${height}px`)
+				cy.wrap(height)
+			})
+	})
+}