Skip to content

Commit 1122bb1

Browse files
committed
fix(drag-drop): handle list and viewport auto scroll regions overlapping
The way auto-scrolling is currently set up is that we have regions that are X% of the width/height of a node and whenever the user's pointer gets into the region, we start scrolling either the element or the page. The problem with our current approach is that as soon we find that the user's pointer is in one node's scroll region, we disregard any of the other scroll regions and we start scrolling, even if the element can't actually scroll. This is problematic in cases where the scroll list might be close to the viewport's edge and thus it could have its region overlapping with the viewport's. These changes address the issue in two ways: 1. We have the list's scroll regions take precedence over the ones of the page. 2. If the regions overlap, we check whether the element can continue scrolling that direction and if it can't, we start scrolling the page. Fixes #16647.
1 parent 8e321ae commit 1122bb1

File tree

2 files changed

+101
-19
lines changed

2 files changed

+101
-19
lines changed

src/cdk/drag-drop/directives/drag.spec.ts

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3081,9 +3081,47 @@ describe('CdkDrag', () => {
30813081
cleanup();
30823082
}));
30833083

3084-
it('should auto-scroll the viewport, not the list, when the pointer is over the edge of ' +
3084+
it('should auto-scroll the list, not the viewport, when the pointer is over the edge of ' +
30853085
'both the list and the viewport', fakeAsync(() => {
3086-
const fixture = createComponent(DraggableInDropZone);
3086+
const fixture = createComponent(DraggableInScrollableVerticalDropZone);
3087+
fixture.detectChanges();
3088+
3089+
const list = fixture.componentInstance.dropInstance.element.nativeElement;
3090+
const viewportRuler: ViewportRuler = TestBed.get(ViewportRuler);
3091+
const item = fixture.componentInstance.dragItems.first.element.nativeElement;
3092+
3093+
// Position the list so that its top aligns with the viewport top. That way the pointer
3094+
// will both over its top edge and the viewport's. We use top instead of bottom, because
3095+
// bottom behaves weirdly when we run tests on mobile devices.
3096+
list.style.position = 'fixed';
3097+
list.style.left = '50%';
3098+
list.style.top = '0';
3099+
list.style.margin = '0';
3100+
3101+
const listRect = list.getBoundingClientRect();
3102+
const cleanup = makePageScrollable();
3103+
3104+
scrollTo(0, viewportRuler.getViewportSize().height * 5);
3105+
list.scrollTop = 50;
3106+
3107+
const initialScrollDistance = viewportRuler.getViewportScrollPosition().top;
3108+
expect(initialScrollDistance).toBeGreaterThan(0);
3109+
expect(list.scrollTop).toBe(50);
3110+
3111+
startDraggingViaMouse(fixture, item);
3112+
dispatchMouseEvent(document, 'mousemove', listRect.left + listRect.width / 2, 0);
3113+
fixture.detectChanges();
3114+
tickAnimationFrames(20);
3115+
3116+
expect(viewportRuler.getViewportScrollPosition().top).toBe(initialScrollDistance);
3117+
expect(list.scrollTop).toBeLessThan(50);
3118+
3119+
cleanup();
3120+
}));
3121+
3122+
it('should auto-scroll the viewport, when the pointer is over the edge of both the list ' +
3123+
'and the viewport, if the list cannot be scrolled in that direction', fakeAsync(() => {
3124+
const fixture = createComponent(DraggableInScrollableVerticalDropZone);
30873125
fixture.detectChanges();
30883126

30893127
const list = fixture.componentInstance.dropInstance.element.nativeElement;
@@ -3102,6 +3140,7 @@ describe('CdkDrag', () => {
31023140
const cleanup = makePageScrollable();
31033141

31043142
scrollTo(0, viewportRuler.getViewportSize().height * 5);
3143+
list.scrollTop = 0;
31053144

31063145
const initialScrollDistance = viewportRuler.getViewportScrollPosition().top;
31073146
expect(initialScrollDistance).toBeGreaterThan(0);

src/cdk/drag-drop/drop-list-ref.ts

Lines changed: 60 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -517,31 +517,28 @@ export class DropListRef<T = any> {
517517
let verticalScrollDirection = AutoScrollVerticalDirection.NONE;
518518
let horizontalScrollDirection = AutoScrollHorizontalDirection.NONE;
519519

520+
// Check whether we should start scrolling the container.
521+
if (this._isPointerNearDropContainer(pointerX, pointerY)) {
522+
const element = coerceElement(this.element);
523+
524+
[verticalScrollDirection, horizontalScrollDirection] =
525+
getElementScrollDirections(element, this._clientRect, pointerX, pointerY);
526+
527+
if (verticalScrollDirection || horizontalScrollDirection) {
528+
scrollNode = element;
529+
}
530+
}
531+
520532
// @breaking-change 9.0.0 Remove null check for _viewportRuler once it's a required parameter.
521-
// Check whether we're in range to scroll the viewport.
522-
if (this._viewportRuler) {
533+
// Otherwise check if we can start scrolling the viewport.
534+
if (this._viewportRuler && !verticalScrollDirection && !horizontalScrollDirection) {
523535
const {width, height} = this._viewportRuler.getViewportSize();
524536
const clientRect = {width, height, top: 0, right: width, bottom: height, left: 0};
525537
verticalScrollDirection = getVerticalScrollDirection(clientRect, pointerY);
526538
horizontalScrollDirection = getHorizontalScrollDirection(clientRect, pointerX);
527539
scrollNode = window;
528540
}
529541

530-
// If we couldn't find a scroll direction based on the
531-
// window, try with the container, if the pointer is close by.
532-
if (!verticalScrollDirection && !horizontalScrollDirection &&
533-
this._isPointerNearDropContainer(pointerX, pointerY)) {
534-
verticalScrollDirection = getVerticalScrollDirection(this._clientRect, pointerY);
535-
horizontalScrollDirection = getHorizontalScrollDirection(this._clientRect, pointerX);
536-
scrollNode = coerceElement(this.element);
537-
}
538-
539-
// TODO(crisbeto): we also need to account for whether the view or element are scrollable in
540-
// the first place. With the current approach we'll still try to scroll them, but it just
541-
// won't do anything. The only case where this is relevant is that if we have a scrollable
542-
// list close to the viewport edge where the viewport isn't scrollable. In this case the
543-
// we'll be trying to scroll the viewport rather than the list.
544-
545542
if (scrollNode && (verticalScrollDirection !== this._verticalScrollDirection ||
546543
horizontalScrollDirection !== this._horizontalScrollDirection ||
547544
scrollNode !== this._scrollNode)) {
@@ -994,3 +991,49 @@ function getHorizontalScrollDirection(clientRect: ClientRect, pointerX: number)
994991

995992
return AutoScrollHorizontalDirection.NONE;
996993
}
994+
995+
/**
996+
* Gets the directions in which an element node should be scrolled,
997+
* assuming that the user's pointer is already within it scrollable region.
998+
* @param element Element for which we should calculate the scroll direction.
999+
* @param clientRect Bounding client rectangle of the element.
1000+
* @param pointerX Position of the user's pointer along the x axis.
1001+
* @param pointerY Position of the user's pointer along the y axis.
1002+
*/
1003+
function getElementScrollDirections(element: HTMLElement, clientRect: ClientRect, pointerX: number,
1004+
pointerY: number): [AutoScrollVerticalDirection, AutoScrollHorizontalDirection] {
1005+
const computedVertical = getVerticalScrollDirection(clientRect, pointerY);
1006+
const computedHorizontal = getHorizontalScrollDirection(clientRect, pointerX);
1007+
let verticalScrollDirection = AutoScrollVerticalDirection.NONE;
1008+
let horizontalScrollDirection = AutoScrollHorizontalDirection.NONE;
1009+
1010+
// Note that we here we do some extra checks for whether the element is actually scrollable in
1011+
// a certain direction and we only assign the scroll direction if it is. We do this so that we
1012+
// can allow other elements to be scrolled, if the current element can't be scrolled anymore.
1013+
// This allows us to handle cases where the scroll regions of two scrollable elements overlap.
1014+
if (computedVertical) {
1015+
const scrollTop = element.scrollTop;
1016+
1017+
if (computedVertical === AutoScrollVerticalDirection.UP) {
1018+
if (scrollTop > 0) {
1019+
verticalScrollDirection = AutoScrollVerticalDirection.UP;
1020+
}
1021+
} else if (element.scrollHeight - scrollTop > element.clientHeight) {
1022+
verticalScrollDirection = AutoScrollVerticalDirection.DOWN;
1023+
}
1024+
}
1025+
1026+
if (computedHorizontal) {
1027+
const scrollLeft = element.scrollLeft;
1028+
1029+
if (computedHorizontal === AutoScrollHorizontalDirection.LEFT) {
1030+
if (scrollLeft > 0) {
1031+
horizontalScrollDirection = AutoScrollHorizontalDirection.LEFT;
1032+
}
1033+
} else if (element.scrollWidth - scrollLeft > element.clientWidth) {
1034+
horizontalScrollDirection = AutoScrollHorizontalDirection.RIGHT;
1035+
}
1036+
}
1037+
1038+
return [verticalScrollDirection, horizontalScrollDirection];
1039+
}

0 commit comments

Comments
 (0)