-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix: fix pinch to zoom #6544
Conversation
There was a problem hiding this 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() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
* fix: fix pinch to zoom * chore: format
The basics
npm run format
andnpm run lint
The details
Resolves
fixes #6508
Proposed Changes
TouchEvent
s 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 becausePointerEvent
is actually a subclass ofMouseEvent
so we always enter the first if statement.Behavior After Change
Touch.getTouchIdentifierFromEvent
returnsevent.pointerId
forPointerEvent
s 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 aMouseEvent
and checkingchangedTouches
if it's aTouchEvent
. Note that even if we have aPointerEvent
where thepointerType
ismouse
, we still use thepointerId
and not the string'mouse'
as that matches the old behavior from before #6099.Reason for Changes
fixes broken pinch to zoom
Test Coverage
PointerEvent
s as Blockly isn't actually supported in any environment that doesn't supportPointerEvent
s.Documentation
Additional Information
I'm 99% certain that we could change
getTouchIdentifierFromEvent
to simplyreturn e.pointerId
, since PointerEvents are now robustly supported. But I'll leave that to #6543.