Skip to content

fix(drag-drop): handle list and viewport auto scroll regions overlapping #16675

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
43 changes: 41 additions & 2 deletions src/cdk/drag-drop/directives/drag.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3081,9 +3081,47 @@ describe('CdkDrag', () => {
cleanup();
}));

it('should auto-scroll the viewport, not the list, when the pointer is over the edge of ' +
it('should auto-scroll the list, not the viewport, when the pointer is over the edge of ' +
'both the list and the viewport', fakeAsync(() => {
const fixture = createComponent(DraggableInDropZone);
const fixture = createComponent(DraggableInScrollableVerticalDropZone);
fixture.detectChanges();

const list = fixture.componentInstance.dropInstance.element.nativeElement;
const viewportRuler: ViewportRuler = TestBed.get(ViewportRuler);
const item = fixture.componentInstance.dragItems.first.element.nativeElement;

// Position the list so that its top aligns with the viewport top. That way the pointer
// will both over its top edge and the viewport's. We use top instead of bottom, because
// bottom behaves weirdly when we run tests on mobile devices.
list.style.position = 'fixed';
list.style.left = '50%';
list.style.top = '0';
list.style.margin = '0';

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

scrollTo(0, viewportRuler.getViewportSize().height * 5);
list.scrollTop = 50;

const initialScrollDistance = viewportRuler.getViewportScrollPosition().top;
expect(initialScrollDistance).toBeGreaterThan(0);
expect(list.scrollTop).toBe(50);

startDraggingViaMouse(fixture, item);
dispatchMouseEvent(document, 'mousemove', listRect.left + listRect.width / 2, 0);
fixture.detectChanges();
tickAnimationFrames(20);

expect(viewportRuler.getViewportScrollPosition().top).toBe(initialScrollDistance);
expect(list.scrollTop).toBeLessThan(50);

cleanup();
}));

it('should auto-scroll the viewport, when the pointer is over the edge of both the list ' +
'and the viewport, if the list cannot be scrolled in that direction', fakeAsync(() => {
const fixture = createComponent(DraggableInScrollableVerticalDropZone);
fixture.detectChanges();

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

scrollTo(0, viewportRuler.getViewportSize().height * 5);
list.scrollTop = 0;

const initialScrollDistance = viewportRuler.getViewportScrollPosition().top;
expect(initialScrollDistance).toBeGreaterThan(0);
Expand Down
77 changes: 60 additions & 17 deletions src/cdk/drag-drop/drop-list-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -517,31 +517,28 @@ export class DropListRef<T = any> {
let verticalScrollDirection = AutoScrollVerticalDirection.NONE;
let horizontalScrollDirection = AutoScrollHorizontalDirection.NONE;

// Check whether we should start scrolling the container.
if (this._isPointerNearDropContainer(pointerX, pointerY)) {
const element = coerceElement(this.element);

[verticalScrollDirection, horizontalScrollDirection] =
getElementScrollDirections(element, this._clientRect, pointerX, pointerY);

if (verticalScrollDirection || horizontalScrollDirection) {
scrollNode = element;
}
}

// @breaking-change 9.0.0 Remove null check for _viewportRuler once it's a required parameter.
// Check whether we're in range to scroll the viewport.
if (this._viewportRuler) {
// Otherwise check if we can start scrolling the viewport.
if (this._viewportRuler && !verticalScrollDirection && !horizontalScrollDirection) {
const {width, height} = this._viewportRuler.getViewportSize();
const clientRect = {width, height, top: 0, right: width, bottom: height, left: 0};
verticalScrollDirection = getVerticalScrollDirection(clientRect, pointerY);
horizontalScrollDirection = getHorizontalScrollDirection(clientRect, pointerX);
scrollNode = window;
}

// If we couldn't find a scroll direction based on the
// window, try with the container, if the pointer is close by.
if (!verticalScrollDirection && !horizontalScrollDirection &&
this._isPointerNearDropContainer(pointerX, pointerY)) {
verticalScrollDirection = getVerticalScrollDirection(this._clientRect, pointerY);
horizontalScrollDirection = getHorizontalScrollDirection(this._clientRect, pointerX);
scrollNode = coerceElement(this.element);
}

// TODO(crisbeto): we also need to account for whether the view or element are scrollable in
// the first place. With the current approach we'll still try to scroll them, but it just
// won't do anything. The only case where this is relevant is that if we have a scrollable
// list close to the viewport edge where the viewport isn't scrollable. In this case the
// we'll be trying to scroll the viewport rather than the list.

if (scrollNode && (verticalScrollDirection !== this._verticalScrollDirection ||
horizontalScrollDirection !== this._horizontalScrollDirection ||
scrollNode !== this._scrollNode)) {
Expand Down Expand Up @@ -994,3 +991,49 @@ function getHorizontalScrollDirection(clientRect: ClientRect, pointerX: number)

return AutoScrollHorizontalDirection.NONE;
}

/**
* Gets the directions in which an element node should be scrolled,
* assuming that the user's pointer is already within it scrollable region.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"within it scrollable region" -> "within its scrollable region" ?

* @param element Element for which we should calculate the scroll direction.
* @param clientRect Bounding client rectangle of the element.
* @param pointerX Position of the user's pointer along the x axis.
* @param pointerY Position of the user's pointer along the y axis.
*/
function getElementScrollDirections(element: HTMLElement, clientRect: ClientRect, pointerX: number,
pointerY: number): [AutoScrollVerticalDirection, AutoScrollHorizontalDirection] {
const computedVertical = getVerticalScrollDirection(clientRect, pointerY);
const computedHorizontal = getHorizontalScrollDirection(clientRect, pointerX);
let verticalScrollDirection = AutoScrollVerticalDirection.NONE;
let horizontalScrollDirection = AutoScrollHorizontalDirection.NONE;

// Note that we here we do some extra checks for whether the element is actually scrollable in
// a certain direction and we only assign the scroll direction if it is. We do this so that we
// can allow other elements to be scrolled, if the current element can't be scrolled anymore.
// This allows us to handle cases where the scroll regions of two scrollable elements overlap.
if (computedVertical) {
const scrollTop = element.scrollTop;

if (computedVertical === AutoScrollVerticalDirection.UP) {
if (scrollTop > 0) {
verticalScrollDirection = AutoScrollVerticalDirection.UP;
}
} else if (element.scrollHeight - scrollTop > element.clientHeight) {
verticalScrollDirection = AutoScrollVerticalDirection.DOWN;
}
}

if (computedHorizontal) {
const scrollLeft = element.scrollLeft;

if (computedHorizontal === AutoScrollHorizontalDirection.LEFT) {
if (scrollLeft > 0) {
horizontalScrollDirection = AutoScrollHorizontalDirection.LEFT;
}
} else if (element.scrollWidth - scrollLeft > element.clientWidth) {
horizontalScrollDirection = AutoScrollHorizontalDirection.RIGHT;
}
}

return [verticalScrollDirection, horizontalScrollDirection];
}