Skip to content

fix(drag-drop): boundary not accounting for parent scrolling #19108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 23, 2020
Merged

fix(drag-drop): boundary not accounting for parent scrolling #19108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 56 additions & 17 deletions src/cdk/drag-drop/directives/drag.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ describe('CdkDrag', () => {
const fixture = createComponent(StandaloneDraggable);
fixture.detectChanges();

const cleanup = makePageScrollable();
const cleanup = makeScrollable();
const dragElement = fixture.componentInstance.dragElement.nativeElement;

scrollTo(0, 500);
Expand Down Expand Up @@ -126,7 +126,7 @@ describe('CdkDrag', () => {
fixture.detectChanges();

const dragElement = fixture.componentInstance.dragElement.nativeElement;
const cleanup = makePageScrollable();
const cleanup = makeScrollable();

scrollTo(0, 500);
expect(dragElement.style.transform).toBeFalsy();
Expand Down Expand Up @@ -256,7 +256,7 @@ describe('CdkDrag', () => {
fixture.detectChanges();

const dragElement = fixture.componentInstance.dragElement.nativeElement;
const cleanup = makePageScrollable();
const cleanup = makeScrollable();

scrollTo(0, 500);
expect(dragElement.style.transform).toBeFalsy();
Expand Down Expand Up @@ -285,7 +285,7 @@ describe('CdkDrag', () => {
fixture.detectChanges();

const dragElement = fixture.componentInstance.dragElement.nativeElement;
const cleanup = makePageScrollable();
const cleanup = makeScrollable();

scrollTo(0, 500);
expect(dragElement.style.transform).toBeFalsy();
Expand Down Expand Up @@ -2034,7 +2034,7 @@ describe('CdkDrag', () => {

const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;
const list = fixture.componentInstance.dropInstance.element.nativeElement;
const cleanup = makePageScrollable();
const cleanup = makeScrollable();
scrollTo(0, 10);
let listRect = list.getBoundingClientRect(); // Note that we need to measure after scrolling.

Expand All @@ -2060,6 +2060,43 @@ describe('CdkDrag', () => {
cleanup();
}));

it('should update the boundary if a parent is scrolled while dragging', fakeAsync(() => {
const fixture = createComponent(DraggableInScrollableParentContainer);
fixture.componentInstance.boundarySelector = '.cdk-drop-list';
fixture.detectChanges();

const container: HTMLElement = fixture.nativeElement.querySelector('.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;
let listRect = list.getBoundingClientRect(); // Note that we need to measure after scrolling.

startDraggingViaMouse(fixture, item);
startDraggingViaMouse(fixture, item, listRect.right, listRect.bottom);
flush();
dispatchMouseEvent(document, 'mousemove', listRect.right, listRect.bottom);
fixture.detectChanges();

const preview = document.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();
Expand Down Expand Up @@ -2375,7 +2412,7 @@ describe('CdkDrag', () => {
fakeAsync(() => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();
const cleanup = makePageScrollable();
const cleanup = makeScrollable();

scrollTo(0, 500);
assertDownwardSorting(fixture, fixture.componentInstance.dragItems.map(item => {
Expand All @@ -2396,7 +2433,7 @@ describe('CdkDrag', () => {
fakeAsync(() => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();
const cleanup = makePageScrollable();
const cleanup = makeScrollable();

scrollTo(0, 500);
assertUpwardSorting(fixture, fixture.componentInstance.dragItems.map(item => {
Expand Down Expand Up @@ -2893,7 +2930,7 @@ describe('CdkDrag', () => {
it('should keep the preview next to the trigger if the page was scrolled', fakeAsync(() => {
const fixture = createComponent(DraggableInDropZoneWithCustomPreview);
fixture.detectChanges();
const cleanup = makePageScrollable();
const cleanup = makeScrollable();
const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;

startDraggingViaMouse(fixture, item, 50, 50);
Expand Down Expand Up @@ -3468,7 +3505,7 @@ describe('CdkDrag', () => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();

const cleanup = makePageScrollable();
const cleanup = makeScrollable();
const item = fixture.componentInstance.dragItems.first.element.nativeElement;
const viewportRuler = TestBed.inject(ViewportRuler);
const viewportSize = viewportRuler.getViewportSize();
Expand All @@ -3489,7 +3526,7 @@ describe('CdkDrag', () => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();

const cleanup = makePageScrollable();
const cleanup = makeScrollable();
const item = fixture.componentInstance.dragItems.first.element.nativeElement;
const viewportRuler = TestBed.inject(ViewportRuler);
const viewportSize = viewportRuler.getViewportSize();
Expand All @@ -3512,7 +3549,7 @@ describe('CdkDrag', () => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();

const cleanup = makePageScrollable('horizontal');
const cleanup = makeScrollable('horizontal');
const item = fixture.componentInstance.dragItems.first.element.nativeElement;
const viewportRuler = TestBed.inject(ViewportRuler);
const viewportSize = viewportRuler.getViewportSize();
Expand All @@ -3533,7 +3570,7 @@ describe('CdkDrag', () => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();

const cleanup = makePageScrollable('horizontal');
const cleanup = makeScrollable('horizontal');
const item = fixture.componentInstance.dragItems.first.element.nativeElement;
const viewportRuler = TestBed.inject(ViewportRuler);
const viewportSize = viewportRuler.getViewportSize();
Expand Down Expand Up @@ -3570,7 +3607,7 @@ describe('CdkDrag', () => {
list.style.margin = '0';

const listRect = list.getBoundingClientRect();
const cleanup = makePageScrollable();
const cleanup = makeScrollable();

scrollTo(0, viewportRuler.getViewportSize().height * 5);
list.scrollTop = 50;
Expand Down Expand Up @@ -3608,7 +3645,7 @@ describe('CdkDrag', () => {
list.style.margin = '0';

const listRect = list.getBoundingClientRect();
const cleanup = makePageScrollable();
const cleanup = makeScrollable();

scrollTo(0, viewportRuler.getViewportSize().height * 5);
list.scrollTop = 0;
Expand Down Expand Up @@ -4684,7 +4721,7 @@ describe('CdkDrag', () => {
fixture.detectChanges();

// Make the page scrollable and scroll the items out of view.
const cleanup = makePageScrollable();
const cleanup = makeScrollable();
scrollTo(0, 4000);
dispatchFakeEvent(document, 'scroll');
fixture.detectChanges();
Expand Down Expand Up @@ -5890,11 +5927,13 @@ function getElementSibligsByPosition(element: Element, direction: 'top' | 'left'
* Adds a large element to the page in order to make it scrollable.
* @returns Function that should be used to clean up after the test is done.
*/
function makePageScrollable(direction: 'vertical' | 'horizontal' = 'vertical') {
function makeScrollable(
direction: 'vertical' | 'horizontal' = 'vertical',
element = document.body) {
const veryTallElement = document.createElement('div');
veryTallElement.style.width = direction === 'vertical' ? '100%' : '4000px';
veryTallElement.style.height = direction === 'vertical' ? '2000px' : '5px';
document.body.appendChild(veryTallElement);
element.appendChild(veryTallElement);

return () => {
scrollTo(0, 0);
Expand Down
55 changes: 34 additions & 21 deletions src/cdk/drag-drop/drag-ref.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ import {Direction} from '@angular/cdk/bidi';
import {normalizePassiveListenerOptions} from '@angular/cdk/platform';
import {coerceBooleanProperty, coerceElement} from '@angular/cdk/coercion';
import {Subscription, Subject, Observable} from 'rxjs';
import {startWith} from 'rxjs/operators';
import {DropListRefInternal as DropListRef} from './drop-list-ref';
import {DragDropRegistry} from './drag-drop-registry';
import {extendStyles, toggleNativeDragInteractions} from './drag-styling';
import {getTransformTransitionDurationInMs} from './transition-duration';
import {getMutableClientRect, adjustClientRect} from './client-rect';
import {ParentPositionTracker} from './parent-position-tracker';

/** Object that can be used to configure the behavior of DragRef. */
export interface DragRefConfig {
Expand Down Expand Up @@ -136,8 +136,8 @@ export class DragRef<T = any> {
/** Index at which the item started in its initial container. */
private _initialIndex: number;

/** Cached scroll position on the page when the element was picked up. */
private _scrollPosition: {top: number, left: number};
/** Cached positions of scrollable parent elements. */
private _parentPositions: ParentPositionTracker;

/** Emits when the item is being moved. */
private _moveEvents = new Subject<{
Expand Down Expand Up @@ -305,6 +305,7 @@ export class DragRef<T = any> {
private _dragDropRegistry: DragDropRegistry<DragRef, DropListRef>) {

this.withRootElement(element);
this._parentPositions = new ParentPositionTracker(_document, _viewportRuler);
_dragDropRegistry.registerDragItem(this);
}

Expand Down Expand Up @@ -422,6 +423,7 @@ export class DragRef<T = any> {
this._disabledHandles.clear();
this._dropContainer = undefined;
this._resizeSubscription.unsubscribe();
this._parentPositions.clear();
this._boundaryElement = this._rootElement = this._placeholderTemplate =
this._previewTemplate = this._anchor = null!;
}
Expand Down Expand Up @@ -702,7 +704,9 @@ export class DragRef<T = any> {

this._toggleNativeDragInteractions();

if (this._dropContainer) {
const dropContainer = this._dropContainer;

if (dropContainer) {
const element = this._rootElement;
const parent = element.parentNode!;
const preview = this._preview = this._createPreviewElement();
Expand All @@ -718,12 +722,16 @@ export class DragRef<T = any> {
element.style.display = 'none';
this._document.body.appendChild(parent.replaceChild(placeholder, element));
getPreviewInsertionPoint(this._document).appendChild(preview);
this._dropContainer.start();
this._initialContainer = this._dropContainer;
this._initialIndex = this._dropContainer.getItemIndex(this);
dropContainer.start();
this._initialContainer = dropContainer;
this._initialIndex = dropContainer.getItemIndex(this);
} else {
this._initialContainer = this._initialIndex = undefined!;
}

// Important to run after we've called `start` on the parent container
// so that it has had time to resolve its scrollable parents.
this._parentPositions.cache(dropContainer ? dropContainer.getScrollableParents() : []);
}

/**
Expand Down Expand Up @@ -775,8 +783,8 @@ export class DragRef<T = any> {
this._removeSubscriptions();
this._pointerMoveSubscription = this._dragDropRegistry.pointerMove.subscribe(this._pointerMove);
this._pointerUpSubscription = this._dragDropRegistry.pointerUp.subscribe(this._pointerUp);
this._scrollSubscription = this._dragDropRegistry.scroll.pipe(startWith(null)).subscribe(() => {
this._updateOnScroll();
this._scrollSubscription = this._dragDropRegistry.scroll.subscribe(scrollEvent => {
this._updateOnScroll(scrollEvent);
});

if (this._boundaryElement) {
Expand Down Expand Up @@ -1012,8 +1020,9 @@ export class DragRef<T = any> {
const handleElement = referenceElement === this._rootElement ? null : referenceElement;
const referenceRect = handleElement ? handleElement.getBoundingClientRect() : elementRect;
const point = isTouchEvent(event) ? event.targetTouches[0] : event;
const x = point.pageX - referenceRect.left - this._scrollPosition.left;
const y = point.pageY - referenceRect.top - this._scrollPosition.top;
const scrollPosition = this._getViewportScrollPosition();
const x = point.pageX - referenceRect.left - scrollPosition.left;
const y = point.pageY - referenceRect.top - scrollPosition.top;

return {
x: referenceRect.left - elementRect.left + x,
Expand All @@ -1025,10 +1034,11 @@ export class DragRef<T = any> {
private _getPointerPositionOnPage(event: MouseEvent | TouchEvent): Point {
// `touches` will be empty for start/end events so we have to fall back to `changedTouches`.
const point = isTouchEvent(event) ? (event.touches[0] || event.changedTouches[0]) : event;
const scrollPosition = this._getViewportScrollPosition();

return {
x: point.pageX - this._scrollPosition.left,
y: point.pageY - this._scrollPosition.top
x: point.pageX - scrollPosition.left,
y: point.pageY - scrollPosition.top
};
}

Expand Down Expand Up @@ -1146,6 +1156,7 @@ export class DragRef<T = any> {
/** Cleans up any cached element dimensions that we don't need after dragging has stopped. */
private _cleanupCachedDimensions() {
this._boundaryRect = this._previewRect = undefined;
this._parentPositions.clear();
}

/**
Expand Down Expand Up @@ -1221,19 +1232,21 @@ export class DragRef<T = any> {
}

/** Updates the internal state of the draggable element when scrolling has occurred. */
private _updateOnScroll() {
const oldScrollPosition = this._scrollPosition;
const currentScrollPosition = this._viewportRuler.getViewportScrollPosition();
private _updateOnScroll(event: Event) {
const scrollDifference = this._parentPositions.handleScroll(event);

// ClientRect dimensions are based on the page's scroll position so
// we have to update the cached boundary ClientRect if the user has scrolled.
if (oldScrollPosition && this._boundaryRect) {
const topDifference = oldScrollPosition.top - currentScrollPosition.top;
const leftDifference = oldScrollPosition.left - currentScrollPosition.left;
adjustClientRect(this._boundaryRect, topDifference, leftDifference);
if (this._boundaryRect && scrollDifference) {
adjustClientRect(this._boundaryRect, scrollDifference.top, scrollDifference.left);
}
}

this._scrollPosition = currentScrollPosition;
/** Gets the scroll position of the viewport. */
private _getViewportScrollPosition() {
const cachedPosition = this._parentPositions.positions.get(this._document);
return cachedPosition ? cachedPosition.scrollPosition :
this._viewportRuler.getViewportScrollPosition();
}
}

Expand Down
Loading