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: undefined points in touch events #1517

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

slak44
Copy link

@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

codecov bot commented Dec 6, 2022

Codecov Report

Merging #1517 (e11001a) into master (925a3dd) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            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     
Impacted Files Coverage Δ
src/eventListeners/touchEventListeners.js 4.27% <0.00%> (-0.05%) ⬇️
src/tools/MagnifyTool.js 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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