Skip to content
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

bug(drag-drop): Auto-scroll step is double the expected value due to RxJS bug #19242

Open
Achilles1515 opened this issue May 3, 2020 · 0 comments
Labels
area: cdk/drag-drop P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@Achilles1515
Copy link

The auto-scroll pixel step size per frame is currently set as 2.

/**
 * Number of pixels to scroll for each frame when auto-scrolling an element.
 * The value comes from trying it out manually until it feels right.
 */
const AUTO_SCROLL_STEP = 2;

But this is false. The actual scroll step is double this value (4).

ng-run demo

The above demo has a console.log statement added here:

/**
   * Starts listening to scroll events on the viewport.
   * 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);

        // <--- HERE --->
        console.log(scrollDifference);

        if (scrollDifference) {
          // ...
        }
      } else if (this.isReceiving()) {
        // ...
      }
    });
  }

Notice the typical value being reported is +4 or -4 while auto-scrolling.

GIF of auto-scroll and resulting console log:

GIF2

Problem

The interval(0, animationFrameScheduler) call below triggers an RxJS bug that causes the work in the subscription to be called twice as often.

  /** Starts the interval that'll auto-scroll the element. */
  private _startScrollInterval = () => {
    this._stopScrolling();

    interval(0, animationFrameScheduler)
      .pipe(takeUntil(this._stopScrollTimers))
      .subscribe(() => {
        const node = this._scrollNode;

        if (this._verticalScrollDirection === AutoScrollVerticalDirection.UP) {
          incrementVerticalScroll(node, -AUTO_SCROLL_STEP);
        } else if (this._verticalScrollDirection === AutoScrollVerticalDirection.DOWN) {
          incrementVerticalScroll(node, AUTO_SCROLL_STEP);
        }

        if (this._horizontalScrollDirection === AutoScrollHorizontalDirection.LEFT) {
          incrementHorizontalScroll(node, -AUTO_SCROLL_STEP);
        } else if (this._horizontalScrollDirection === AutoScrollHorizontalDirection.RIGHT) {
          incrementHorizontalScroll(node, AUTO_SCROLL_STEP);
        }
      });
  }

More information on this bug can be found in this issue report, and a fix is supposedly in for the next RxJS patch. Even so, it is not recommended to use animationFrameScheduler to run loops. The new animationFrames observable is for this purpose, but that's an RxJS 7.0 feature.

I think a regular requestAnimationFrame loop should be used instead.

Environment

  • Angular:
  • CDK/Material: 9.2.2
  • Browser(s):
  • Operating System (e.g. Windows, macOS, Ubuntu):
@Achilles1515 Achilles1515 added the needs triage This issue needs to be triaged by the team label May 3, 2020
@crisbeto crisbeto added area: cdk/drag-drop P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs triage This issue needs to be triaged by the team labels May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cdk/drag-drop P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

No branches or pull requests

2 participants