Fix cell DND issues

roblourens · roblourens · commit 12b445140d50 · 2020-05-15T16:39:33.000-05:00
- Infinite loop in syncing focus/selection where the view cell index is not the same as the model cell index
- Error thrown when dragging just above the top of the list
diff --git a/src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts b/src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts
@@ -364,7 +364,7 @@ export interface INotebookEditor extends ICompositeCodeEditor {
 
 export interface INotebookCellList {
 	readonly contextKeyService: IContextKeyService;
-	elementAt(position: number): ICellViewModel;
+	elementAt(position: number): ICellViewModel | undefined;
 	elementHeight(element: ICellViewModel): number;
 	onWillScroll: Event<ScrollEvent>;
 	onDidChangeFocus: Event<IListEvent<ICellViewModel>>;
diff --git a/src/vs/workbench/contrib/notebook/browser/view/notebookCellList.ts b/src/vs/workbench/contrib/notebook/browser/view/notebookCellList.ts
@@ -67,15 +67,17 @@ export class NotebookCellList extends WorkbenchList<CellViewModel> implements ID
 			this._previousFocusedElements = e.elements;
 
 			// if focus is in the list, but is not inside the focused element, then reset focus
-			if (DOM.isAncestor(document.activeElement, this.rowsContainer)) {
-				const focusedElement = this.getFocusedElements()[0];
-				if (focusedElement) {
-					const focusedDomElement = this.domElementOfElement(focusedElement);
-					if (focusedDomElement && !DOM.isAncestor(document.activeElement, focusedDomElement)) {
-						focusedDomElement.focus();
+			setTimeout(() => {
+				if (DOM.isAncestor(document.activeElement, this.rowsContainer)) {
+					const focusedElement = this.getFocusedElements()[0];
+					if (focusedElement) {
+						const focusedDomElement = this.domElementOfElement(focusedElement);
+						if (focusedDomElement && !DOM.isAncestor(document.activeElement, focusedDomElement)) {
+							focusedDomElement.focus();
+						}
 					}
 				}
-			}
+			}, 0);
 		}));
 
 		const notebookEditorCursorAtBoundaryContext = NOTEBOOK_EDITOR_CURSOR_BOUNDARY.bindTo(contextKeyService);
@@ -132,8 +134,13 @@ export class NotebookCellList extends WorkbenchList<CellViewModel> implements ID
 
 	}
 
-	elementAt(position: number): ICellViewModel {
-		return this.element(this.view.indexAt(position));
+	elementAt(position: number): ICellViewModel | undefined {
+		const idx = this.view.indexAt(position);
+		if (idx >= 0) {
+			return this.element(idx);
+		}
+
+		return undefined;
 	}
 
 	elementHeight(element: ICellViewModel): number {
diff --git a/src/vs/workbench/contrib/notebook/browser/view/renderers/cellRenderer.ts b/src/vs/workbench/contrib/notebook/browser/view/renderers/cellRenderer.ts
@@ -537,11 +537,15 @@ export class CellDragAndDropController extends Disposable {
 		this._register(domEvent(document.body, DOM.EventType.DRAG_END, true)(this.onGlobalDragEnd.bind(this)));
 
 		const addCellDragListener = (eventType: string, handler: (e: CellDragEvent) => void) => {
-			this._register(
-				Event.map(
-					domEvent(notebookEditor.getDomNode(), eventType),
-					this.toCellDragEvent.bind(this))
-					(handler));
+			this._register(DOM.addDisposableListener(
+				notebookEditor.getDomNode(),
+				eventType,
+				e => {
+					const cellDragEvent = this.toCellDragEvent(e);
+					if (cellDragEvent) {
+						handler(cellDragEvent);
+					}
+				}));
 		};
 
 		addCellDragListener(DOM.EventType.DRAG_OVER, event => {
@@ -576,10 +580,14 @@ export class CellDragAndDropController extends Disposable {
 		this.listInsertionIndicator.style.opacity = visible ? '1' : '0';
 	}
 
-	private toCellDragEvent(event: DragEvent): CellDragEvent {
+	private toCellDragEvent(event: DragEvent): CellDragEvent | undefined {
 		const targetTop = this.notebookEditor.getDomNode().getBoundingClientRect().top;
 		const dragOffset = this.list.scrollTop + event.clientY - targetTop;
 		const draggedOverCell = this.list.elementAt(dragOffset);
+		if (!draggedOverCell) {
+			return undefined;
+		}
+
 		const cellTop = this.list.getAbsoluteTopOfElement(draggedOverCell);
 		const cellHeight = this.list.elementHeight(draggedOverCell);
 
