From f35d9a7079c94ca8b203e2b10b5facdea9c3196a Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sun, 20 Jun 2021 13:35:14 +0200 Subject: [PATCH] fix(cdk/drag-drop): resolve various event tracking issues inside the shadow dom The `drag-drop` module depends on a single document-level `scroll` listener in order to adjust itself when the page or an element is scrolled. The problem is that the events won't be picked up if they come from inside a shadow root. These changes add some logic to bind an extra event at the shadow root level. Furthermore, they fix several places where we were reading the `Event.target` while not accounting for shadow DOM. Fixes #22939. --- src/cdk/drag-drop/directives/drag.spec.ts | 159 +++++++++++++++---- src/cdk/drag-drop/drag-drop-registry.spec.ts | 4 +- src/cdk/drag-drop/drag-drop-registry.ts | 43 ++++- src/cdk/drag-drop/drag-ref.ts | 18 ++- src/cdk/drag-drop/drop-list-ref.ts | 54 ++++--- src/cdk/drag-drop/parent-position-tracker.ts | 7 +- tools/public_api_guard/cdk/drag-drop.d.ts | 1 + 7 files changed, 215 insertions(+), 71 deletions(-) diff --git a/src/cdk/drag-drop/directives/drag.spec.ts b/src/cdk/drag-drop/directives/drag.spec.ts index 16c114aff2f1..c9962f8a14f7 100644 --- a/src/cdk/drag-drop/directives/drag.spec.ts +++ b/src/cdk/drag-drop/directives/drag.spec.ts @@ -44,28 +44,33 @@ const ITEM_WIDTH = 75; describe('CdkDrag', () => { function createComponent( - componentType: Type, providers: Provider[] = [], dragDistance = 0, - extraDeclarations: Type[] = []): ComponentFixture { - TestBed - .configureTestingModule({ - imports: [DragDropModule, CdkScrollableModule], - declarations: [componentType, PassthroughComponent, ...extraDeclarations], - providers: [ - { - provide: CDK_DRAG_CONFIG, - useValue: { - // We default the `dragDistance` to zero, because the majority of the tests - // don't care about it and drags are a lot easier to simulate when we don't - // have to deal with thresholds. - dragStartThreshold: dragDistance, - pointerDirectionChangeThreshold: 5 - } as DragDropConfig - }, - ...providers - ], - }) - .compileComponents(); + componentType: Type, providers: Provider[] = [], dragDistance = 0, + extraDeclarations: Type[] = [], encapsulation?: ViewEncapsulation): ComponentFixture { + TestBed.configureTestingModule({ + imports: [DragDropModule, CdkScrollableModule], + declarations: [componentType, PassthroughComponent, ...extraDeclarations], + providers: [ + { + provide: CDK_DRAG_CONFIG, + useValue: { + // We default the `dragDistance` to zero, because the majority of the tests + // don't care about it and drags are a lot easier to simulate when we don't + // have to deal with thresholds. + dragStartThreshold: dragDistance, + pointerDirectionChangeThreshold: 5 + } as DragDropConfig + }, + ...providers + ], + }); + + if (encapsulation != null) { + TestBed.overrideComponent(componentType, { + set: {encapsulation} + }); + } + TestBed.compileComponents(); return TestBed.createComponent(componentType); } @@ -2010,6 +2015,54 @@ describe('CdkDrag', () => { }); })); + it('should calculate the index if the list is scrolled while dragging inside the shadow DOM', + fakeAsync(() => { + // This test is only relevant for Shadow DOM-supporting browsers. + if (!_supportsShadowDom()) { + return; + } + + const fixture = createComponent(DraggableInScrollableVerticalDropZone, [], undefined, [], + ViewEncapsulation.ShadowDom); + fixture.detectChanges(); + const dragItems = fixture.componentInstance.dragItems; + const firstItem = dragItems.first; + const thirdItemRect = dragItems.toArray()[2].element.nativeElement.getBoundingClientRect(); + const list = fixture.componentInstance.dropInstance.element.nativeElement; + + startDraggingViaMouse(fixture, firstItem.element.nativeElement); + fixture.detectChanges(); + + dispatchMouseEvent(document, 'mousemove', thirdItemRect.left + 1, thirdItemRect.top + 1); + fixture.detectChanges(); + + list.scrollTop = ITEM_HEIGHT * 10; + dispatchFakeEvent(list, 'scroll'); + fixture.detectChanges(); + + dispatchMouseEvent(document, 'mouseup'); + fixture.detectChanges(); + flush(); + fixture.detectChanges(); + + expect(fixture.componentInstance.droppedSpy).toHaveBeenCalledTimes(1); + + const event = fixture.componentInstance.droppedSpy.calls.mostRecent().args[0]; + + // Assert the event like this, rather than `toHaveBeenCalledWith`, because Jasmine will + // go into an infinite loop trying to stringify the event, if the test fails. + expect(event).toEqual({ + previousIndex: 0, + currentIndex: 12, + item: firstItem, + container: fixture.componentInstance.dropInstance, + previousContainer: fixture.componentInstance.dropInstance, + isPointerOverContainer: jasmine.any(Boolean), + distance: {x: jasmine.any(Number), y: jasmine.any(Number)}, + dropPoint: {x: jasmine.any(Number), y: jasmine.any(Number)} + }); + })); + it('should calculate the index if the viewport is scrolled while dragging', fakeAsync(() => { const fixture = createComponent(DraggableInDropZone); @@ -2260,6 +2313,54 @@ describe('CdkDrag', () => { cleanup(); })); + it('should update the boundary if a parent is scrolled while dragging inside the shadow DOM', + fakeAsync(() => { + // This test is only relevant for Shadow DOM-supporting browsers. + if (!_supportsShadowDom()) { + return; + } + + const fixture = createComponent(DraggableInScrollableParentContainer, [], undefined, [], + ViewEncapsulation.ShadowDom); + fixture.componentInstance.boundarySelector = '.cdk-drop-list'; + fixture.detectChanges(); + + const container: HTMLElement = fixture.nativeElement.shadowRoot + .querySelector('.scroll-container'); + const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement; + const list = fixture.componentInstance.dropInstance.element.nativeElement; + const cleanup = makeScrollable('vertical', container); + container.scrollTop = 10; + + // Note that we need to measure after scrolling. + let listRect = list.getBoundingClientRect(); + + startDraggingViaMouse(fixture, item); + startDraggingViaMouse(fixture, item, listRect.right, listRect.bottom); + flush(); + dispatchMouseEvent(document, 'mousemove', listRect.right, listRect.bottom); + fixture.detectChanges(); + + const preview = fixture.nativeElement.shadowRoot + .querySelector('.cdk-drag-preview')! as HTMLElement; + let previewRect = preview.getBoundingClientRect(); + + // Different browsers round the scroll position differently so + // assert that the offsets are within a pixel of each other. + expect(Math.abs(previewRect.bottom - listRect.bottom)).toBeLessThan(2); + + container.scrollTop = 0; + dispatchFakeEvent(container, 'scroll'); + fixture.detectChanges(); + listRect = list.getBoundingClientRect(); // We need to update these since we've scrolled. + dispatchMouseEvent(document, 'mousemove', listRect.right, listRect.bottom); + fixture.detectChanges(); + previewRect = preview.getBoundingClientRect(); + + expect(Math.abs(previewRect.bottom - listRect.bottom)).toBeLessThan(2); + cleanup(); + })); + it('should clear the id from the preview', fakeAsync(() => { const fixture = createComponent(DraggableInDropZone); fixture.detectChanges(); @@ -5520,7 +5621,8 @@ describe('CdkDrag', () => { return; } - const fixture = createComponent(ConnectedDropZonesInsideShadowRoot); + const fixture = createComponent(ConnectedDropZones, [], undefined, [], + ViewEncapsulation.ShadowDom); fixture.detectChanges(); const groups = fixture.componentInstance.groupedDragItems; @@ -5651,7 +5753,8 @@ describe('CdkDrag', () => { return; } - const fixture = createComponent(ConnectedDropZonesInsideShadowRoot); + const fixture = createComponent(ConnectedDropZones, [], undefined, [], + ViewEncapsulation.ShadowDom); fixture.detectChanges(); const item = fixture.componentInstance.groupedDragItems[0][1]; @@ -5961,7 +6064,7 @@ class DraggableInDropZone implements AfterViewInit { // Firefox preserves the `scrollTop` value from previous similar containers. This // could throw off test assertions and result in flaky results. // See: https://bugzilla.mozilla.org/show_bug.cgi?id=959812. - this._elementRef.nativeElement.querySelector('.scroll-container').scrollTop = 0; + this.dropInstance.element.nativeElement.scrollTop = 0; } } @@ -6363,14 +6466,6 @@ class ConnectedDropZones implements AfterViewInit { } } -@Component({ - encapsulation: ViewEncapsulation.ShadowDom, - styles: CONNECTED_DROP_ZONES_STYLES, - template: CONNECTED_DROP_ZONES_TEMPLATE -}) -class ConnectedDropZonesInsideShadowRoot extends ConnectedDropZones { -} - @Component({ encapsulation: ViewEncapsulation.ShadowDom, styles: CONNECTED_DROP_ZONES_STYLES, diff --git a/src/cdk/drag-drop/drag-drop-registry.spec.ts b/src/cdk/drag-drop/drag-drop-registry.spec.ts index 9eca66fc68e9..623fd557a4af 100644 --- a/src/cdk/drag-drop/drag-drop-registry.spec.ts +++ b/src/cdk/drag-drop/drag-drop-registry.spec.ts @@ -224,7 +224,7 @@ describe('DragDropRegistry', () => { it('should dispatch `scroll` events if the viewport is scrolled while dragging', () => { const spy = jasmine.createSpy('scroll spy'); - const subscription = registry.scroll.subscribe(spy); + const subscription = registry.scrolled().subscribe(spy); const item = new DragItem(); registry.startDragging(item, createMouseEvent('mousedown')); @@ -236,7 +236,7 @@ describe('DragDropRegistry', () => { it('should not dispatch `scroll` events when not dragging', () => { const spy = jasmine.createSpy('scroll spy'); - const subscription = registry.scroll.subscribe(spy); + const subscription = registry.scrolled().subscribe(spy); dispatchFakeEvent(document, 'scroll'); diff --git a/src/cdk/drag-drop/drag-drop-registry.ts b/src/cdk/drag-drop/drag-drop-registry.ts index 2c7135374d48..535499b5c31f 100644 --- a/src/cdk/drag-drop/drag-drop-registry.ts +++ b/src/cdk/drag-drop/drag-drop-registry.ts @@ -9,7 +9,7 @@ import {Injectable, NgZone, OnDestroy, Inject} from '@angular/core'; import {DOCUMENT} from '@angular/common'; import {normalizePassiveListenerOptions} from '@angular/cdk/platform'; -import {Subject} from 'rxjs'; +import {merge, Observable, Observer, Subject} from 'rxjs'; /** Event options that can be used to bind an active, capturing event. */ const activeCapturingEventOptions = normalizePassiveListenerOptions({ @@ -62,7 +62,11 @@ export class DragDropRegistry implements O */ readonly pointerUp: Subject = new Subject(); - /** Emits when the viewport has been scrolled while the user is dragging an item. */ + /** + * Emits when the viewport has been scrolled while the user is dragging an item. + * @deprecated To be turned into a private member. Use the `scrolled` method instead. + * @breaking-change 13.0.0 + */ readonly scroll: Subject = new Subject(); constructor( @@ -185,6 +189,41 @@ export class DragDropRegistry implements O return this._activeDragInstances.indexOf(drag) > -1; } + /** + * Gets a stream that will emit when any element on the page is scrolled while an item is being + * dragged. + * @param shadowRoot Optional shadow root that the current dragging sequence started from. + * Top-level listeners won't pick up events coming from the shadow DOM so this parameter can + * be used to include an additional top-level listener at the shadow root level. + */ + scrolled(shadowRoot?: DocumentOrShadowRoot | null): Observable { + const streams: Observable[] = [this.scroll]; + + if (shadowRoot && shadowRoot !== this._document) { + // Note that this is basically the same as `fromEvent` from rjxs, but we do it ourselves, + // because we want to guarantee that the event is bound outside of the `NgZone`. With + // `fromEvent` it'll only happen if the subscription is outside the `NgZone`. + streams.push(new Observable((observer: Observer) => { + return this._ngZone.runOutsideAngular(() => { + const eventOptions = true; + const callback = (event: Event) => { + if (this._activeDragInstances.length) { + observer.next(event); + } + }; + + (shadowRoot as ShadowRoot).addEventListener('scroll', callback, eventOptions); + + return () => { + (shadowRoot as ShadowRoot).removeEventListener('scroll', callback, eventOptions); + }; + }); + })); + } + + return merge(...streams); + } + ngOnDestroy() { this._dragInstances.forEach(instance => this.removeDragItem(instance)); this._dropInstances.forEach(instance => this.removeDropContainer(instance)); diff --git a/src/cdk/drag-drop/drag-ref.ts b/src/cdk/drag-drop/drag-ref.ts index 6c66c0df21e6..db00e8fea55d 100644 --- a/src/cdk/drag-drop/drag-ref.ts +++ b/src/cdk/drag-drop/drag-ref.ts @@ -22,7 +22,7 @@ import { } from './drag-styling'; import {getTransformTransitionDurationInMs} from './transition-duration'; import {getMutableClientRect, adjustClientRect} from './client-rect'; -import {ParentPositionTracker} from './parent-position-tracker'; +import {getEventTarget, ParentPositionTracker} from './parent-position-tracker'; import {deepCloneNode} from './clone-node'; /** Object that can be used to configure the behavior of DragRef. */ @@ -623,7 +623,7 @@ export class DragRef { // Delegate the event based on whether it started from a handle or the element itself. if (this._handles.length) { const targetHandle = this._handles.find(handle => { - const target = event.target; + const target = getEventTarget(event); return !!target && (target === handle || handle.contains(target as HTMLElement)); }); @@ -851,6 +851,7 @@ export class DragRef { const isTouchSequence = isTouchEvent(event); const isAuxiliaryMouseButton = !isTouchSequence && (event as MouseEvent).button !== 0; const rootElement = this._rootElement; + const target = getEventTarget(event); const isSyntheticEvent = !isTouchSequence && this._lastTouchEventTime && this._lastTouchEventTime + MOUSE_EVENT_IGNORE_TIME > Date.now(); @@ -860,7 +861,7 @@ export class DragRef { // it's flaky and it fails if the user drags it away quickly. Also note that we only want // to do this for `mousedown` since doing the same for `touchstart` will stop any `click` // events from firing on touch devices. - if (event.target && (event.target as HTMLElement).draggable && event.type === 'mousedown') { + if (target && (target as HTMLElement).draggable && event.type === 'mousedown') { event.preventDefault(); } @@ -884,9 +885,9 @@ export class DragRef { this._removeSubscriptions(); this._pointerMoveSubscription = this._dragDropRegistry.pointerMove.subscribe(this._pointerMove); this._pointerUpSubscription = this._dragDropRegistry.pointerUp.subscribe(this._pointerUp); - this._scrollSubscription = this._dragDropRegistry.scroll.subscribe(scrollEvent => { - this._updateOnScroll(scrollEvent); - }); + this._scrollSubscription = this._dragDropRegistry + .scrolled(this._getShadowRoot()) + .subscribe(scrollEvent => this._updateOnScroll(scrollEvent)); if (this._boundaryElement) { this._boundaryRect = getMutableClientRect(this._boundaryElement); @@ -1084,7 +1085,8 @@ export class DragRef { return this._ngZone.runOutsideAngular(() => { return new Promise(resolve => { const handler = ((event: TransitionEvent) => { - if (!event || (event.target === this._preview && event.propertyName === 'transform')) { + if (!event || (getEventTarget(event) === this._preview && + event.propertyName === 'transform')) { this._preview.removeEventListener('transitionend', handler); resolve(); clearTimeout(timeout); @@ -1379,7 +1381,7 @@ export class DragRef { const scrollDifference = this._parentPositions.handleScroll(event); if (scrollDifference) { - const target = event.target as Node; + const target = getEventTarget(event); // ClientRect dimensions are based on the scroll position of the page and its parent node so // we have to update the cached boundary ClientRect if the user has scrolled. Check for diff --git a/src/cdk/drag-drop/drop-list-ref.ts b/src/cdk/drag-drop/drop-list-ref.ts index c21d2d94bf9a..60db591fef39 100644 --- a/src/cdk/drag-drop/drop-list-ref.ts +++ b/src/cdk/drag-drop/drop-list-ref.ts @@ -899,33 +899,35 @@ export class DropListRef { * Used for updating the internal state of the list. */ private _listenToScrollEvents() { - this._viewportScrollSubscription = this._dragDropRegistry.scroll.subscribe(event => { - if (this.isDragging()) { - const scrollDifference = this._parentPositions.handleScroll(event); - - if (scrollDifference) { - // Since we know the amount that the user has scrolled we can shift all of the - // client rectangles ourselves. This is cheaper than re-measuring everything and - // we can avoid inconsistent behavior where we might be measuring the element before - // its position has changed. - this._itemPositions.forEach(({clientRect}) => { - adjustClientRect(clientRect, scrollDifference.top, scrollDifference.left); - }); - - // We need two loops for this, because we want all of the cached - // positions to be up-to-date before we re-sort the item. - this._itemPositions.forEach(({drag}) => { - if (this._dragDropRegistry.isDragging(drag)) { - // We need to re-sort the item manually, because the pointer move - // events won't be dispatched while the user is scrolling. - drag._sortFromLastPointerPosition(); - } - }); + this._viewportScrollSubscription = this._dragDropRegistry + .scrolled(this._getShadowRoot()) + .subscribe(event => { + if (this.isDragging()) { + const scrollDifference = this._parentPositions.handleScroll(event); + + if (scrollDifference) { + // Since we know the amount that the user has scrolled we can shift all of the + // client rectangles ourselves. This is cheaper than re-measuring everything and + // we can avoid inconsistent behavior where we might be measuring the element before + // its position has changed. + this._itemPositions.forEach(({clientRect}) => { + adjustClientRect(clientRect, scrollDifference.top, scrollDifference.left); + }); + + // We need two loops for this, because we want all of the cached + // positions to be up-to-date before we re-sort the item. + this._itemPositions.forEach(({drag}) => { + if (this._dragDropRegistry.isDragging(drag)) { + // We need to re-sort the item manually, because the pointer move + // events won't be dispatched while the user is scrolling. + drag._sortFromLastPointerPosition(); + } + }); + } + } else if (this.isReceiving()) { + this._cacheParentPositions(); } - } else if (this.isReceiving()) { - this._cacheParentPositions(); - } - }); + }); } /** diff --git a/src/cdk/drag-drop/parent-position-tracker.ts b/src/cdk/drag-drop/parent-position-tracker.ts index 52e5305b658d..b66fccf4943b 100644 --- a/src/cdk/drag-drop/parent-position-tracker.ts +++ b/src/cdk/drag-drop/parent-position-tracker.ts @@ -47,7 +47,7 @@ export class ParentPositionTracker { /** Handles scrolling while a drag is taking place. */ handleScroll(event: Event): ScrollPosition | null { - const target = event.target as HTMLElement | Document; + const target = getEventTarget(event); const cachedPosition = this.positions.get(target); if (!cachedPosition) { @@ -88,3 +88,8 @@ export class ParentPositionTracker { return {top: topDifference, left: leftDifference}; } } + +/** Gets the target of an event while accounting for shadow dom. */ +export function getEventTarget(event: Event): HTMLElement | Document { + return (event.composedPath ? event.composedPath()[0] : event.target) as HTMLElement | Document; +} diff --git a/tools/public_api_guard/cdk/drag-drop.d.ts b/tools/public_api_guard/cdk/drag-drop.d.ts index 7ce9051036a7..7428f40d4de3 100644 --- a/tools/public_api_guard/cdk/drag-drop.d.ts +++ b/tools/public_api_guard/cdk/drag-drop.d.ts @@ -254,6 +254,7 @@ export declare class DragDropRegistry; startDragging(drag: I, event: TouchEvent | MouseEvent): void; stopDragging(drag: I): void; static ɵfac: i0.ɵɵFactoryDeclaration, never>;