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

fix(touch): prevent infinite loop on multi-touch drag #8470

Merged
merged 4 commits into from
Aug 8, 2024

Conversation

AbhinavKRN
Copy link
Contributor

@AbhinavKRN AbhinavKRN commented Aug 7, 2024

The basics

The details

Resolves

Fixes #8396

Proposed Changes

This pull request addresses a RangeError: Maximum call stack size exceeded error that occurs when users attempt to drag a block with multiple fingers on a workspace where zoom controls/wheel are disabled. The change modifies the handling of touch events to prevent an infinite loop.

Reason for Changes

The error occurs due to the interaction between the handleMove and handleTouchMove functions. If a multi-touch event does not meet the conditions for a pinch zoom, the functions call each other recursively, resulting in a maximum call stack size exceeded error. This issue has caused disruptions, triggering alarms in our monitoring system and impacting our on-call engineers.

Test Coverage

I have tested the changes manually by:

  1. Using a touch device.
  2. Disabling zoom wheel and controls.
  3. Adding a block to the workspace.
  4. Using two fingers to attempt to drag the block.
    The RangeError no longer occurs, and the drag operation works as expected. Additional unit tests have been created to simulate multi-touch events and verify that no infinite loop occurs.

Documentation

No documentation updates are required as this change only affects internal event handling.

Additional Information

The issue was detected using NewRelic, which reported an increase in JS errors since migrating additional labs to Google Blockly. The proposed solution involves modifying the handleTouchMove function to handle the move operation directly when pinch zoom conditions are not met, preventing the recursive call loop.

Detailed Analysis

The problem occurs in the interaction between handleMove and handleTouchMove:

handleMove Function:

handleMove(e: PointerEvent) {
  if ((this.isDragging() && Touch.shouldHandleEvent(e)) || !this.isMultiTouch()) {
    // Handle single touch or drag...
  } else if (this.isMultiTouch()) {
    this.handleTouchMove(e);
    Touch.longStop();
  }
}

@AbhinavKRN AbhinavKRN requested a review from a team as a code owner August 7, 2024 09:40
Copy link

google-cla bot commented Aug 7, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome! It looks like this is your first pull request in Blockly, so here are a couple of tips:

  • You can find tips about contributing to Blockly and how to validate your changes on our developer site.
  • All contributors must sign the Google Contributor License Agreement (CLA). If the google-cla bot leaves a comment on this PR, make sure you follow the instructions.
  • We use conventional commits to make versioning the package easier. Make sure your commit message is in the proper format or learn how to fix it.
  • If any of the other checks on this PR fail, you can click on them to learn why. It might be that your change caused a test failure, or that you need to double-check the style guide.
    Thank you for opening this PR! A member of the Blockly team will review it soon.

@BeksOmega BeksOmega requested review from BeksOmega and removed request for gonfunko August 7, 2024 16:31
@BeksOmega BeksOmega assigned BeksOmega and unassigned gonfunko Aug 7, 2024
@BeksOmega
Copy link
Collaborator

Heya @AbhinavKRN This change looks good =) Can you go ahead and sign the CLA for me so I can get it merged?

@AbhinavKRN
Copy link
Contributor Author

@BeksOmega Can you tell me how to sign CLA, as i'm new to this term.

@BeksOmega
Copy link
Collaborator

Check out the message from above @AbhinavKRN =)

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@AbhinavKRN AbhinavKRN changed the title Resolved Infinite loop when handling 'pointermove' events. fix(touch): prevent infinite loop on multi-touch drag Aug 8, 2024
@github-actions github-actions bot added the PR: fix Fixes a bug label Aug 8, 2024
@AbhinavKRN
Copy link
Contributor Author

@BeksOmega Done with CLA!

@AbhinavKRN
Copy link
Contributor Author

@BeksOmega there was some error with prettier, i have resolved it again. Can you check it.

@BeksOmega BeksOmega merged commit 6b3f9a6 into google:develop Aug 8, 2024
7 checks passed
@BeksOmega
Copy link
Collaborator

Thank you again for the work on this @AbhinavKRN !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite loop when handling 'pointermove' events.
3 participants