Fix CSS scale handling by passing through mjolnir.js event data#558
Conversation
|
I've added a unit test for the abstract class which this PR updates. It's so heavily integrated with deck.gl and mjolnir.js that there's a lot of mocking needed to exercise the code.. maybe there's a better way to test this? |
|
To locally test this you can add this div around the DeckGL element in advanced editor example: <div
style={{
transform: 'scaleX(1.2) scaleY(0.8)',
transformOrigin: 'center',
width: '100%',
height: '100%'
}}
>
<DeckGL>.... </DeckGL>
</div>Behavior before the fix: before.mp4Behavior after the fix: after.mp4 |
|
Nice fix — using Re: your question about the testing approach — the mocking level is appropriate here. You can't avoid mocking That said, the test could be leaner. Since all the event handlers now delegate to // Test the actual fix — one focused test
test('toBasePointerEvent uses offsetCenter, not clientX/Y', () => {
const event = makeMockGestureEvent('click');
const result = layer.toBasePointerEvent(event);
expect(result.screenCoords).toEqual([event.offsetCenter.x, event.offsetCenter.y]);
expect(result.mapCoords).toEqual(MOCK_EVENT_MAP_COORDS);
expect(result.picks).toHaveLength(1);
expect(result.sourceEvent).toBe(event.srcEvent);
});
// Then just verify wiring — each handler delegates correctly
test('event handlers delegate to toBasePointerEvent', () => {
const spy = vi.spyOn(layer, 'toBasePointerEvent');
layer.onLayerClick = vi.fn();
layer._onclick(makeMockGestureEvent('click'));
expect(spy).toHaveBeenCalledOnce();
});That would cut the test file significantly while covering the same surface. The handler-specific tests ( |
b924048 to
635bb23
Compare
|
Thanks for the helpful feedback @charlieforward9 The test does repeat itself a bit 👍. A targeted test for the fix is a really solid idea, what about a pattern like this: const assertBasePointerEvent = (evt: BasePointerEvent, mockSrcEvent: any) => {
expect(evt.screenCoords).toEqual(MOCK_EVENT_SCREEN_COORDS);
expect(evt.mapCoords).toEqual(MOCK_EVENT_MAP_COORDS);
expect(evt.sourceEvent).toBe(mockSrcEvent.srcEvent);
expect(Array.isArray(evt.picks)).toBe(true);
expect(evt.picks[0]).toMatchObject({ index: 0, object: { id: 1 } });
return evt;
};
const expectBasePointerEvent = <E extends BasePointerEvent>(callback: any, mockSrcEvent: any): E => {
expect(callback).toHaveBeenCalledOnce();
return assertBasePointerEvent(callback.mock.calls[0][0], mockSrcEvent) as E;
};
// The targeted test for the new behaviour
test('toBasePointerEvent uses offsetCenter, not clientX/Y', () => {
const event = makeMockGestureEvent('click');
const result = layer.toBasePointerEvent(event);
assertBasePointerEvent(result, event);
});
// Re-use the helpers to ensure that the events that get passed to the event handler functions look correct
test('_onpanstart calls onStartDragging with correct event', () => {
const onStartDraggingSpy = vi.fn();
layer.onStartDragging = onStartDraggingSpy;
const mockEvent = makeMockGestureEvent('panstart');
layer._onpanstart(mockEvent);
// verify BasePointerEvent properties are correct
const event: StartDraggingEvent = expectBasePointerEvent(onStartDraggingSpy, mockEvent);
// verify DraggingEvent properties are correct (though, this is also common and could be in another shared helper)
expect(event.pointerDownScreenCoords).toEqual(MOCK_EVENT_SCREEN_COORDS);
expect(event.pointerDownMapCoords).toEqual(MOCK_EVENT_MAP_COORDS);
expect(Array.isArray(event.pointerDownPicks)).toBe(true);
expect(event.pointerDownPicks[0]).toMatchObject({ index: 0, object: { id: 1 } });
expect(typeof event.cancelPan).toBe('function');
}); |
|
Beautiful ✨ |
This PR fixes incorrect screen coordinate calculation in EditableLayer by using the offsetCenter provided by mjolnir.js gesture events, instead of recomputing coordinates from the DOM event (clientX/Y + canvas bounds).
I've also tightened up the types in
editable-layer.ts, there are a lot of events typed asanywhich this tries to address too.Some local manual testing done, no tests written yet - raising to get initial thoughts on this approach.