Skip to content

Commit

Permalink
fix(drag-drop): boundary not accounting for parent scrolling (angular…
Browse files Browse the repository at this point in the history 
…#19108)

In angular#18597 some logic was added to update the boundary dimensions when the page is scrolled, however that logic didn't account for the case where a parent element is being scrolled. We were already handling parent elements for the drop list so these changes move the logic into a separate class so it can be reused and fix the issue for the drag item.

Fixes angular#19086.
  • Loading branch information
@crisbeto
crisbeto authored Apr 23, 2020
1 parent 4eef958 commit 0bf1a07
Show file tree
Hide file tree
Showing 5 changed files with 216 additions and 132 deletions.
73 changes: 56 additions & 17 deletions src/cdk/drag-drop/directives/drag.spec.ts
View file
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 @@ -3485,7 +3522,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 @@ -3506,7 +3543,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 @@ -3529,7 +3566,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 @@ -3550,7 +3587,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 @@ -3587,7 +3624,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 @@ -3625,7 +3662,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 @@ -4744,7 +4781,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 @@ -5984,11 +6021,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
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 @@ -1014,8 +1022,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 @@ -1027,10 +1036,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 @@ -1148,6 +1158,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 @@ -1223,19 +1234,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

0 comments on commit 0bf1a07

Please sign in to comment.