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: fix pinch to zoom #6544

Merged
merged 2 commits into from
Oct 14, 2022
Merged

fix: fix pinch to zoom #6544

merged 2 commits into from
Oct 14, 2022

Conversation

maribethb
Copy link
Contributor

@maribethb maribethb commented Oct 13, 2022

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

fixes #6508

Proposed Changes

  • Correctly sets the current gesture id the same way it did before fix: improve types in touch code #6099 (if the pointerId exists, use that, mostly)
  • Adds tests for Blockly.Touch. I didn't write any tests for TouchEvents because I literally couldn't test Blockly in a browser that doesn't support Pointer Events (even IE 10 mostly supports them, and we can't even test in IE11 because we no longer support it in other ways). The MouseEvent tests are also pretty useless for that reason, but it was easier to write a fake MouseEvent.

Behavior Before Change

Pinch to zoom is broken because Touch.getTouchIdentifierFromEvent always returns the string 'mouse' for all pointer events, which is all events in modern browsers. The reason it always returns that is because PointerEvent is actually a subclass of MouseEvent so we always enter the first if statement.

Behavior After Change

Touch.getTouchIdentifierFromEvent returns event.pointerId for PointerEvents first. If somehow we don't have a PointerEvent, which is extremely unlikely, then we fall back to the old behavior of 'mouse' if it's a MouseEvent and checking changedTouches if it's a TouchEvent. Note that even if we have a PointerEvent where the pointerType is mouse, we still use the pointerId and not the string 'mouse' as that matches the old behavior from before #6099.

Reason for Changes

fixes broken pinch to zoom

Test Coverage

  • Added unit tests
  • Manually tested using BrowserStack in iOS, Android, and Windows. Also tested in local playground using mouse. Note that all of these manual tests use PointerEvents as Blockly isn't actually supported in any environment that doesn't support PointerEvents.

Documentation

Additional Information

I'm 99% certain that we could change getTouchIdentifierFromEvent to simply return e.pointerId, since PointerEvents are now robustly supported. But I'll leave that to #6543.

@maribethb maribethb requested a review from a team as a code owner October 13, 2022 22:45
@maribethb maribethb requested a review from BeksOmega October 13, 2022 22:45
@github-actions github-actions bot added the PR: fix Fixes a bug label Oct 13, 2022
Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Thanks for writing tests to go with it!

chai.assert.equal(Blockly.Touch.getTouchIdentifierFromEvent(mousedown), 'mouse');
});

test('is pointerId for mouse PointerEvents', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious: why do we want this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this test basically makes sure the behavior is the same as it was in blockly-v8.0.5. In that code (and now this code), if e.pointerId was defined, we'd use that, no matter what. In PointerEvents, it should always be defined, so effectively, we always used that. That means for old old old MouseEvents (that are not PointerEvents) we'd use the string 'mouse' but we haven't actually used it for a long time.

I wrote this test because originally I assumed that if the PointerEvent had a pointerType that was mouse then we'd use the string 'mouse' but that is not correct. We just always use pointerId if it's available. So basically I wrote the test as a reminder of the current behavior.

@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Oct 14, 2022
@maribethb maribethb merged commit caf91c8 into google:develop Oct 14, 2022
cpcallen pushed a commit to cpcallen/blockly that referenced this pull request Oct 19, 2022
* fix: fix pinch to zoom

* chore: format
This was referenced Oct 19, 2022
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.

Pinch to zoom not working anymore
2 participants