From d2e4cec145ef2c4f606cf7df196be54522f816b4 Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Thu, 4 Jul 2024 09:39:32 +0200 Subject: [PATCH 1/2] fix: duplicated elements on public link page Fixes a bug where clicking the ownCloud logo on a public link page would lead to certain UI elements being duplicated. This happens because some extensions and extension points are being registered more than once because the application layout gets unmounted on mounted again when clicking the logo. The solution is to properly unregister those extensions on unmount. --- .../bugfix-duplicated-ui-elements-public-link | 6 +++ .../actions/files/useFileActionsCopy.ts | 2 +- .../actions/files/useFileActionsPaste.ts | 2 +- .../extensionRegistry/extensionRegistry.ts | 12 +++++ .../extensionRegistry.spec.ts | 53 +++++++++++++++++++ .../web-runtime/src/layouts/Application.vue | 37 ++++++++----- 6 files changed, 98 insertions(+), 14 deletions(-) create mode 100644 changelog/unreleased/bugfix-duplicated-ui-elements-public-link diff --git a/changelog/unreleased/bugfix-duplicated-ui-elements-public-link b/changelog/unreleased/bugfix-duplicated-ui-elements-public-link new file mode 100644 index 00000000000..9ed990f9f70 --- /dev/null +++ b/changelog/unreleased/bugfix-duplicated-ui-elements-public-link @@ -0,0 +1,6 @@ +Bugfix: Duplicated elements on public link page + +We've fixed a bug where clicking the ownCloud logo on a public link page would lead to certain UI elements being duplicated. + +https://github.com/owncloud/web/pull/11137 +https://github.com/owncloud/web/issues/10371 diff --git a/packages/web-pkg/src/composables/actions/files/useFileActionsCopy.ts b/packages/web-pkg/src/composables/actions/files/useFileActionsCopy.ts index a1debe69348..a80a86479f5 100644 --- a/packages/web-pkg/src/composables/actions/files/useFileActionsCopy.ts +++ b/packages/web-pkg/src/composables/actions/files/useFileActionsCopy.ts @@ -66,7 +66,7 @@ export const useFileActionsCopy = () => { } if (isLocationPublicActive(router, 'files-public-link')) { - return unref(currentFolder).canCreate() + return unref(currentFolder)?.canCreate() } if ( diff --git a/packages/web-pkg/src/composables/actions/files/useFileActionsPaste.ts b/packages/web-pkg/src/composables/actions/files/useFileActionsPaste.ts index 0e759d251a7..a2ce39213e5 100644 --- a/packages/web-pkg/src/composables/actions/files/useFileActionsPaste.ts +++ b/packages/web-pkg/src/composables/actions/files/useFileActionsPaste.ts @@ -162,7 +162,7 @@ export const useFileActionsPaste = () => { } if (isLocationPublicActive(router, 'files-public-link') && unref(currentFolder)) { - return unref(currentFolder).canCreate() + return unref(currentFolder)?.canCreate() } // copy can't be restricted in authenticated context, because diff --git a/packages/web-pkg/src/composables/piniaStores/extensionRegistry/extensionRegistry.ts b/packages/web-pkg/src/composables/piniaStores/extensionRegistry/extensionRegistry.ts index d0a66abff5a..72c7939215c 100644 --- a/packages/web-pkg/src/composables/piniaStores/extensionRegistry/extensionRegistry.ts +++ b/packages/web-pkg/src/composables/piniaStores/extensionRegistry/extensionRegistry.ts @@ -11,6 +11,11 @@ export const useExtensionRegistry = defineStore('extensionRegistry', () => { const registerExtensions = (e: Ref) => { extensions.value.push(e) } + const unregisterExtensions = (ids: string[]) => { + extensions.value = unref(extensions) + .map((e) => ref(unref(e).filter(({ id }) => !ids.includes(id)))) + .filter((e) => unref(e).length) + } const requestExtensions = (extensionPoint: ExtensionPoint) => { if (!extensionPoint.id || !extensionPoint.extensionType) { throw new Error('ExtensionPoint must have an id and an extensionType') @@ -30,6 +35,11 @@ export const useExtensionRegistry = defineStore('extensionRegistry', () => { const registerExtensionPoints = (e: Ref[]>) => { extensionPoints.value.push(e) } + const unregisterExtensionPoints = (ids: string[]) => { + extensionPoints.value = unref(extensionPoints) + .map((e) => ref(unref(e).filter(({ id }) => !ids.includes(id)))) + .filter((e) => unref(e).length) + } const getExtensionPoints = >( options: { extensionType?: ExtensionType @@ -52,9 +62,11 @@ export const useExtensionRegistry = defineStore('extensionRegistry', () => { return { extensions, registerExtensions, + unregisterExtensions, requestExtensions, extensionPoints, registerExtensionPoints, + unregisterExtensionPoints, getExtensionPoints } }) diff --git a/packages/web-pkg/tests/unit/composables/piniaStores/extensionRegistry/extensionRegistry.spec.ts b/packages/web-pkg/tests/unit/composables/piniaStores/extensionRegistry/extensionRegistry.spec.ts index 33f292c92b3..4946c59d9ff 100644 --- a/packages/web-pkg/tests/unit/composables/piniaStores/extensionRegistry/extensionRegistry.spec.ts +++ b/packages/web-pkg/tests/unit/composables/piniaStores/extensionRegistry/extensionRegistry.spec.ts @@ -132,6 +132,36 @@ describe('useExtensionRegistry', () => { } }) }) + + it('unregisters extensions', () => { + const extensionPoint: ExtensionPoint = mock< + ExtensionPoint + >({ + id: 'extension-point-id', + extensionType: 'customComponent' + }) + + const extension = mock({ + id: 'foo-1', + type: 'customComponent', + extensionPointIds: [extensionPoint.id] + }) + const extensions = computed(() => [extension]) + + getWrapper({ + setup: (instance) => { + instance.registerExtensions(extensions) + + const result1 = instance.requestExtensions(extensionPoint) + expect(result1.length).toBe(1) + + instance.unregisterExtensions([extension.id]) + + const result2 = instance.requestExtensions(extensionPoint) + expect(result2.length).toBe(0) + } + }) + }) }) describe('register and get extensionPoints', () => { @@ -196,6 +226,29 @@ describe('useExtensionRegistry', () => { } }) }) + + it('unregisters extension points', () => { + const extensionPoint = mock>({ + id: 'foo-1', + extensionType: 'customComponent' + }) + + const extensionPoints = computed[]>(() => [extensionPoint]) + + getWrapper({ + setup: (instance) => { + instance.registerExtensionPoints(extensionPoints) + + const result1 = instance.getExtensionPoints({ extensionType: 'customComponent' }) + expect(result1.length).toBe(1) + + instance.unregisterExtensionPoints([extensionPoint.id]) + + const result2 = instance.getExtensionPoints({ extensionType: 'customComponent' }) + expect(result2.length).toBe(0) + } + }) + }) }) }) diff --git a/packages/web-runtime/src/layouts/Application.vue b/packages/web-runtime/src/layouts/Application.vue index 77487947af6..ef33521f89b 100644 --- a/packages/web-runtime/src/layouts/Application.vue +++ b/packages/web-runtime/src/layouts/Application.vue @@ -63,7 +63,18 @@ import MobileNav from '../components/MobileNav.vue' import { NavItem, getExtensionNavItems } from '../helpers/navItems' import { LoadingIndicator } from '@ownclouders/web-pkg' import { useActiveApp, useRoute, useRouteMeta, useSpacesLoading } from '@ownclouders/web-pkg' -import { computed, defineComponent, provide, ref, toRef, unref, watch } from 'vue' +import { + computed, + defineComponent, + nextTick, + onBeforeUnmount, + onMounted, + provide, + ref, + toRef, + unref, + watch +} from 'vue' import { useRouter } from 'vue-router' import { useGettext } from 'vue3-gettext' @@ -200,14 +211,25 @@ export default defineComponent({ const extensionPoints = computed[]>(() => [progressBarExtensionPoint]) extensionRegistry.registerExtensionPoints(extensionPoints) + onMounted(async () => { + await nextTick() + window.addEventListener('resize', onResize) + onResize() + }) + + onBeforeUnmount(() => { + window.removeEventListener('resize', onResize) + + extensionRegistry.unregisterExtensions([progressBarExtensionId]) + extensionRegistry.unregisterExtensionPoints(unref(extensionPoints).flatMap((e) => e.id)) + }) + return { apps, - defaultProgressBarExtension, progressBarExtensionPoint, isSidebarVisible, isLoading, navItems, - onResize, isMobileWidth, navBarClosed, setNavBarClosed @@ -239,15 +261,6 @@ export default defineComponent({ (b.applicationMenu?.priority || Number.MAX_SAFE_INTEGER) ) } - }, - mounted() { - this.$nextTick(() => { - window.addEventListener('resize', this.onResize) - this.onResize() - }) - }, - beforeUnmount() { - window.removeEventListener('resize', this.onResize) } }) From 59c0d53a095b97141dfaa145a76b86f4a12840e3 Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Fri, 5 Jul 2024 08:39:18 +0200 Subject: [PATCH 2/2] fix: prevent apps from re-initializing on logo click Prevents all apps from re-initializing when clicking the ownCloud logo on public link pages. The issue happens because clicking the logo navigates the user to the `resolvePublicLink` page, which results in the public context being reset. We can omit this reset in that case though because we can guarantee that the user will either land on a public page, or on some other route which then resets the context anyways. --- packages/web-runtime/src/services/auth/authService.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/web-runtime/src/services/auth/authService.ts b/packages/web-runtime/src/services/auth/authService.ts index 219d480b403..95be9ff401d 100644 --- a/packages/web-runtime/src/services/auth/authService.ts +++ b/packages/web-runtime/src/services/auth/authService.ts @@ -93,7 +93,8 @@ export class AuthService implements AuthServiceInterface { if (publicLinkToken) { await this.publicLinkManager.updateContext(publicLinkToken) } - } else { + } else if (to.name !== 'resolvePublicLink') { + // no need to clear public context if we're routing the to public link resolving page this.publicLinkManager.clearContext() }