Skip to content

fix: undefined points in touch events#1517

Open
slak44 wants to merge 2 commits intocornerstonejs:masterfrom
Medicai-io:fix/undefined-touch-events
Open

fix: undefined points in touch events#1517
slak44 wants to merge 2 commits intocornerstonejs:masterfrom
Medicai-io:fix/undefined-touch-events

Conversation

@slak44
Copy link
Copy Markdown

@slak44 slak44 commented Dec 6, 2022

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bug fix.

  • What is the current behavior? (You can also link to an open issue here)

Currently, there are several situations where undefined point objects are used in touch event handlers:

  1. Without touching the viewport beforehand, use the zoom tool. The panend event handler is called with startPoints undefined, which later throws.
  2. Using the magnify tool too quickly after scrolling the stack tries to draw the tool but evt.detail.currentPoints is undefined.
  3. There's also a way to reach the panmove handler with the lastDelta object undefined. Though I can't reproduce this as easily, the fix is similar.
  • What is the new behavior (if this is a feature change)?

Check that all these objects are defined, so the tools don't end up accessing properties on undefined. There was already one such check in panend, but the same problem shows up in a few places.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No.

  • Other information:

Found the issues by using the tools in OHIF.

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 6, 2022

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 21.69%. Comparing base (925a3dd) to head (e11001a).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
src/eventListeners/touchEventListeners.js 0.00% 1 Missing and 2 partials ⚠️
src/tools/MagnifyTool.js 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1517      +/-   ##
==========================================
- Coverage   21.70%   21.69%   -0.01%     
==========================================
  Files         287      287              
  Lines       10137    10139       +2     
  Branches     2081     2082       +1     
==========================================
  Hits         2200     2200              
- Misses       6750     6751       +1     
- Partials     1187     1188       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant