From 0ed9258f51ffedac6a593c199355037db0e32ca2 Mon Sep 17 00:00:00 2001 From: pengx17 Date: Wed, 15 Jan 2025 15:04:01 +0000 Subject: [PATCH] fix(core): some dnd perf issues (#9661) 1. page list item are bound two draggables. adding `draggable` prop to WorkbenchLink to mitigate the issue. 2. DndService may not resolve datatransfer when dragging. --- .../component/src/ui/dnd/draggable.ts | 22 +++-- .../component/src/ui/dnd/drop-target.ts | 87 +++++++++++-------- .../page-list/docs/page-list-item.tsx | 2 +- .../core/src/modules/dnd/services/index.ts | 42 ++++----- .../modules/workbench/view/workbench-link.tsx | 9 +- 5 files changed, 89 insertions(+), 73 deletions(-) diff --git a/packages/frontend/component/src/ui/dnd/draggable.ts b/packages/frontend/component/src/ui/dnd/draggable.ts index 964548e7b5..1bb1022c5c 100644 --- a/packages/frontend/component/src/ui/dnd/draggable.ts +++ b/packages/frontend/component/src/ui/dnd/draggable.ts @@ -87,10 +87,16 @@ export const useDraggable = ( }, [...deps, context.toExternalData]); useEffect(() => { - if (!dragRef.current) { + if ( + !dragRef.current || + (typeof options.canDrag === 'boolean' && !options.canDrag) + ) { return; } + const element = dragRef.current; + const dragHandle = dragHandleRef.current; + const windowEvent = { dragleave: () => { setDraggingPosition(state => @@ -104,11 +110,11 @@ export const useDraggable = ( }, }; - dragRef.current.dataset.affineDraggable = 'true'; + element.dataset.affineDraggable = 'true'; const cleanupDraggable = draggable({ - element: dragRef.current, - dragHandle: dragHandleRef.current ?? undefined, + element, + dragHandle: dragHandle ?? undefined, canDrag: draggableGet(options.canDrag), getInitialData: draggableGet(options.data), getInitialDataForExternal: draggableGet(options.toExternalData), @@ -130,8 +136,8 @@ export const useDraggable = ( if (enableDropTarget.current) { setDropTarget([]); } - if (dragRef.current) { - dragRef.current.dataset['dragging'] = 'true'; + if (element) { + element.dataset['dragging'] = 'true'; } options.onDragStart?.(args); }, @@ -153,8 +159,8 @@ export const useDraggable = ( if (enableDropTarget.current) { setDropTarget([]); } - if (dragRef.current) { - delete dragRef.current.dataset['dragging']; + if (element) { + delete element.dataset['dragging']; } options.onDrop?.(args); }, diff --git a/packages/frontend/component/src/ui/dnd/drop-target.ts b/packages/frontend/component/src/ui/dnd/drop-target.ts index ae3bd083ae..31d0bfd2a7 100644 --- a/packages/frontend/component/src/ui/dnd/drop-target.ts +++ b/packages/frontend/component/src/ui/dnd/drop-target.ts @@ -1,3 +1,4 @@ +import { combine } from '@atlaskit/pragmatic-drag-and-drop/combine'; import { dropTargetForElements } from '@atlaskit/pragmatic-drag-and-drop/element/adapter'; import { dropTargetForExternal } from '@atlaskit/pragmatic-drag-and-drop/external/adapter'; import type { @@ -17,7 +18,14 @@ import { type Instruction, type ItemMode, } from '@atlaskit/pragmatic-drag-and-drop-hitbox/tree-item'; -import { useContext, useEffect, useMemo, useRef, useState } from 'react'; +import { + useCallback, + useContext, + useEffect, + useMemo, + useRef, + useState, +} from 'react'; import { shallowUpdater } from '../../utils'; import { getAdaptedEventArgs, isExternalDrag } from './common'; @@ -201,9 +209,16 @@ export const useDropTarget = ( // eslint-disable-next-line react-hooks/exhaustive-deps }, [...deps, dropTargetContext.fromExternalData]); - const dropTargetOptions = useMemo(() => { + const getDropTargetOptions = useCallback(() => { const wrappedCanDrop = dropTargetGet(options.canDrop, options); - let _element: HTMLElement | null = null; + let element: HTMLElement | null = dropTargetRef.current; + + if ( + !element || + (typeof options.canDrop === 'boolean' && !options.canDrop) + ) { + return null; + } const updateDragOver = ( args: DropTargetDragEvent, @@ -254,12 +269,7 @@ export const useDropTarget = ( }; return { - get element() { - if (!_element) { - _element = dropTargetRef.current; - } - return _element; - }, + element, canDrop: wrappedCanDrop ? (args: DropTargetGetFeedback) => { // check if args has data. if not, it's an external drag @@ -290,8 +300,8 @@ export const useDropTarget = ( } if (options.treeInstruction) { setTreeInstruction(null); - if (dropTargetRef.current) { - delete dropTargetRef.current.dataset['treeInstruction']; + if (element) { + delete element.dataset['treeInstruction']; } } if (options.closestEdge) { @@ -300,8 +310,8 @@ export const useDropTarget = ( if (enableDropEffect.current) { setDropEffect(null); } - if (dropTargetRef.current) { - delete dropTargetRef.current.dataset['draggedOver']; + if (element) { + delete element.dataset['draggedOver']; } // external data is only available in drop event thus @@ -318,10 +328,7 @@ export const useDropTarget = ( return; } - if ( - args.location.current.dropTargets[0]?.element === - dropTargetRef.current - ) { + if (args.location.current.dropTargets[0]?.element === element) { options.onDrop?.({ ...args, treeInstruction: extractInstruction(args.self.data), @@ -376,19 +383,15 @@ export const useDropTarget = ( }, onDropTargetChange: (args: DropTargetDropEvent) => { args = getAdaptedEventArgs(args, options.fromExternalData); - if ( - args.location.current.dropTargets[0]?.element === - dropTargetRef.current - ) { + if (args.location.current.dropTargets[0]?.element === element) { if (enableDraggedOver.current) { setDraggedOver(true); } if (options.treeInstruction) { const instruction = extractInstruction(args.self.data); setTreeInstruction(instruction); - if (dropTargetRef.current) { - dropTargetRef.current.dataset['treeInstruction'] = - instruction?.type; + if (element) { + element.dataset['treeInstruction'] = instruction?.type; } } if (options.closestEdge) { @@ -410,8 +413,8 @@ export const useDropTarget = ( clientY: args.location.current.input.clientY, }); } - if (dropTargetRef.current) { - dropTargetRef.current.dataset['draggedOver'] = 'true'; + if (element) { + element.dataset['draggedOver'] = 'true'; } } else { if (enableDraggedOver.current) { @@ -422,8 +425,8 @@ export const useDropTarget = ( } if (options.treeInstruction) { setTreeInstruction(null); - if (dropTargetRef.current) { - delete dropTargetRef.current.dataset['treeInstruction']; + if (element) { + delete element.dataset['treeInstruction']; } } if (enableDropEffect.current) { @@ -440,8 +443,8 @@ export const useDropTarget = ( if (options.closestEdge) { setClosestEdge(null); } - if (dropTargetRef.current) { - delete dropTargetRef.current.dataset['draggedOver']; + if (element) { + delete element.dataset['draggedOver']; } } }, @@ -449,18 +452,26 @@ export const useDropTarget = ( }, [options]); useEffect(() => { - if (!dropTargetRef.current) { + const dropTargetOptions = getDropTargetOptions(); + if (!dropTargetOptions) { return; } - return dropTargetForElements(dropTargetOptions as any); - }, [dropTargetOptions]); - useEffect(() => { - if (!dropTargetRef.current || !options.fromExternalData) { - return; + // @ts-expect-error: fix type error + const cleanup = [dropTargetForElements(dropTargetOptions)]; + + if (options.allowExternal && options.fromExternalData) { + // @ts-expect-error: fix type error + cleanup.push(dropTargetForExternal(dropTargetOptions)); } - return dropTargetForExternal(dropTargetOptions as any); - }, [dropTargetOptions, options.fromExternalData]); + + return combine(...cleanup); + }, [ + getDropTargetOptions, + options.canDrop, + options.allowExternal, + options.fromExternalData, + ]); return { dropTargetRef, diff --git a/packages/frontend/core/src/components/page-list/docs/page-list-item.tsx b/packages/frontend/core/src/components/page-list/docs/page-list-item.tsx index 6016ed737d..cbe24a12c7 100644 --- a/packages/frontend/core/src/components/page-list/docs/page-list-item.tsx +++ b/packages/frontend/core/src/components/page-list/docs/page-list-item.tsx @@ -382,7 +382,7 @@ const PageListItemWrapper = forwardRef( if (to) { return ( - + {children} ); diff --git a/packages/frontend/core/src/modules/dnd/services/index.ts b/packages/frontend/core/src/modules/dnd/services/index.ts index 18d48b4b5b..36bda604fa 100644 --- a/packages/frontend/core/src/modules/dnd/services/index.ts +++ b/packages/frontend/core/src/modules/dnd/services/index.ts @@ -81,7 +81,7 @@ export class DndService extends Service { isDropEvent?: boolean ) => { if (!isDropEvent) { - return this.resolveBlocksuiteExternalData(args.source) || {}; + return {}; } let resolved: AffineDNDData['draggable'] | null = null; @@ -168,30 +168,24 @@ export class DndService extends Service { if (!dndAPI) { return null; } - - if (source.types.includes(dndAPI.mimeType)) { - const from = { - at: 'blocksuite-editor', - } as const; - - let entity: Entity | null = null; - - const encoded = source.getStringData(dndAPI.mimeType); - const snapshot = encoded ? dndAPI.decodeSnapshot(encoded) : null; - entity = snapshot ? this.resolveBlockSnapshot(snapshot) : null; - - if (!entity) { - return { - from, - }; - } else { - return { - entity, - from, - }; - } + const encoded = source.getStringData(dndAPI.mimeType); + if (!encoded) { + return null; } - return null; + const snapshot = dndAPI.decodeSnapshot(encoded); + if (!snapshot) { + return null; + } + const entity = this.resolveBlockSnapshot(snapshot); + if (!entity) { + return null; + } + return { + entity, + from: { + at: 'blocksuite-editor', + }, + }; }; private readonly resolveHTML: EntityResolver = html => { diff --git a/packages/frontend/core/src/modules/workbench/view/workbench-link.tsx b/packages/frontend/core/src/modules/workbench/view/workbench-link.tsx index 65ba3e639f..f1a42ab8cc 100644 --- a/packages/frontend/core/src/modules/workbench/view/workbench-link.tsx +++ b/packages/frontend/core/src/modules/workbench/view/workbench-link.tsx @@ -47,7 +47,10 @@ function resolveToEntity( } export const WorkbenchLink = forwardRef( - function WorkbenchLink({ to, onClick, replaceHistory, ...other }, ref) { + function WorkbenchLink( + { to, onClick, draggable = true, replaceHistory, ...other }, + ref + ) { const { workbenchService } = useServices({ WorkbenchService, }); @@ -79,8 +82,10 @@ export const WorkbenchLink = forwardRef( to: stringTo, }, }, + canDrag: + typeof draggable === 'boolean' ? draggable : draggable === 'true', }; - }, [to, basename, stringTo]); + }, [to, basename, stringTo, draggable]); return (