fix: map widget marker onClick duplication (#39517)
## Description
When clicking on a marker in the Map Widget, the onMarkerClick event was
being triggered twice for a single click, causing duplicate actions and
potentially unexpected behavior in applications.
```js
// In MapWidget/widget/index.tsx
onMarkerClick = (lat?: number, long?: number, title?: string) => {
this.disableDrag(true);
const selectedMarker = {
lat: lat,
long: long,
title: title,
};
console.log("🚀 ~ MapWidget ~ title:", title); // This was logging twice
this.props.updateWidgetMetaProperty("selectedMarker", selectedMarker, {
triggerPropertyName: "onMarkerClick",
dynamicString: this.props.onMarkerClick,
event: {
type: EventType.ON_MARKER_CLICK,
},
});
};
```
## Root Cause
This happens because:
1. The Marker component was adding duplicate event listeners to the
Google Maps marker.
2. Two separate useEffect hooks were both adding "click" event
listeners:
- One during marker initialisation
- Another in a separate useEffect that tracks the onClick prop
```js
// First listener in initialization useEffect
googleMapMarker.addListener("click", () => {
if (onClick) onClick();
});
// Second listener in a separate useEffect
marker.addListener("click", () => {
if (onClick) onClick();
});
```
3. When a marker was clicked, both event listeners fired, causing the
onClick callback to execute twice.
4. This resulted in onMarkerClick being called twice for a single user
interaction.
## Solution
Modify the `Marker.tsx` component to avoid adding duplicate event
listeners:
1. Remove the click event listener from the initialisation useEffect
2. Add proper cleanup for event listeners using clearListeners and
removeListener
3. Store the listener reference to properly remove it in the cleanup
function
4. Apply the same pattern to the dragend event listener
```js
// track on onclick - with proper cleanup
useEffect(() => {
if (!marker) return;
// Clear existing click listeners before adding a new one
google.maps.event.clearListeners(marker, "click");
// Add the new click listener
const clickListener = marker.addListener("click", () => {
if (onClick) onClick();
});
// Return cleanup function
return () => {
google.maps.event.removeListener(clickListener);
};
}, [marker, onClick]);
```
## Why this works
1. Properly cleans up existing listeners before adding new ones
2. Ensures only one click handler is active at any time
3. Prevents event handler duplication during component re-renders
## Testing
Added a Cypress test that verifies the marker click event triggers
exactly once per click:
1. Creates a map widget with a marker
2. Uses a JS Object to track click count
3. Verifies the click count increments by exactly 1 for each click
4. Ensures no duplicate events are triggered
Fixes #39514
## Automation
/ok-to-test tags="@tag.Widget, @tag.Sanity, @tag.Maps, @tag.Binding"
### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/13673511245>
> Commit: 3c025ebd9dd85a3c409bc1582e13e324d8dbb2cd
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13673511245&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Widget, @tag.Sanity, @tag.Maps, @tag.Binding`
> Spec:
> <hr>Wed, 05 Mar 2025 11:49:24 UTC
<!-- end of auto-generated comment: Cypress test results -->
## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Enhanced the behavior of map markers so they respond more reliably to
user interactions like clicking and dragging, addressing issues that
could affect overall stability.
- **Tests**
- Added comprehensive tests to ensure marker functionality remains
consistent and dependable during use.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
parent
832a7062e1
commit
c0e6c3c0ba
121
app/client/src/widgets/MapWidget/component/MapComponent.test.tsx
Normal file
121
app/client/src/widgets/MapWidget/component/MapComponent.test.tsx
Normal file
|
|
@ -0,0 +1,121 @@
|
|||
import React from "react";
|
||||
import { render } from "@testing-library/react";
|
||||
import Marker from "../component/Marker";
|
||||
|
||||
// Mock the google maps API
|
||||
const mockAddListener = jest.fn().mockImplementation((event, callback) => {
|
||||
// Store the callback to simulate click events
|
||||
if (event === "click") {
|
||||
(
|
||||
mockAddListener as unknown as {
|
||||
clickCallback: (...args: unknown[]) => void;
|
||||
}
|
||||
).clickCallback = callback;
|
||||
}
|
||||
|
||||
return "listener-id"; // Return a mock listener ID
|
||||
});
|
||||
|
||||
const mockRemoveListener = jest.fn();
|
||||
const mockSetMap = jest.fn();
|
||||
const mockSetIcon = jest.fn();
|
||||
const mockSetPosition = jest.fn();
|
||||
const mockSetTitle = jest.fn();
|
||||
|
||||
// Add type declaration for the global google object
|
||||
declare global {
|
||||
interface Window {
|
||||
google: unknown;
|
||||
}
|
||||
}
|
||||
|
||||
// Mock the google object
|
||||
(global as unknown as { google: unknown }).google = {
|
||||
maps: {
|
||||
Marker: jest.fn().mockImplementation(() => ({
|
||||
setMap: mockSetMap,
|
||||
setIcon: mockSetIcon,
|
||||
setPosition: mockSetPosition,
|
||||
setTitle: mockSetTitle,
|
||||
addListener: mockAddListener,
|
||||
})),
|
||||
Point: jest.fn().mockImplementation((x, y) => ({ x, y })),
|
||||
event: {
|
||||
clearListeners: jest.fn(),
|
||||
removeListener: mockRemoveListener,
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
describe("Map Widget - Marker Component", () => {
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks();
|
||||
});
|
||||
|
||||
it("should trigger onClick callback only once per click", () => {
|
||||
const onClickMock = jest.fn();
|
||||
|
||||
// Render the marker component with onClick handler
|
||||
render(
|
||||
<Marker
|
||||
onClick={onClickMock}
|
||||
position={{ lat: 37.7749, lng: -122.4194 }}
|
||||
title="Test Marker"
|
||||
/>,
|
||||
);
|
||||
|
||||
// Simulate a marker click by directly calling the stored callback
|
||||
(
|
||||
mockAddListener as unknown as {
|
||||
clickCallback: (...args: unknown[]) => void;
|
||||
}
|
||||
).clickCallback();
|
||||
|
||||
// Verify onClick was called exactly once
|
||||
expect(onClickMock).toHaveBeenCalledTimes(1);
|
||||
|
||||
// Simulate another click
|
||||
(
|
||||
mockAddListener as unknown as {
|
||||
clickCallback: (...args: unknown[]) => void;
|
||||
}
|
||||
).clickCallback();
|
||||
|
||||
// Verify onClick was called exactly twice (once per click)
|
||||
expect(onClickMock).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it("should clear previous click listeners when onClick prop changes", () => {
|
||||
const { rerender } = render(
|
||||
<Marker
|
||||
onClick={jest.fn()}
|
||||
position={{ lat: 37.7749, lng: -122.4194 }}
|
||||
title="Test Marker"
|
||||
/>,
|
||||
);
|
||||
|
||||
// Verify clearListeners was called during initial render
|
||||
expect(google.maps.event.clearListeners).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
"click",
|
||||
);
|
||||
|
||||
// Reset the mock to check if it's called again
|
||||
(google.maps.event.clearListeners as jest.Mock).mockClear();
|
||||
|
||||
// Rerender with a different onClick handler
|
||||
rerender(
|
||||
<Marker
|
||||
onClick={jest.fn()}
|
||||
position={{ lat: 37.7749, lng: -122.4194 }}
|
||||
title="Test Marker"
|
||||
/>,
|
||||
);
|
||||
|
||||
// Verify clearListeners was called again when onClick changed
|
||||
expect(google.maps.event.clearListeners).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
"click",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
@ -31,10 +31,6 @@ const Marker: React.FC<MarkerProps> = (options) => {
|
|||
title,
|
||||
});
|
||||
|
||||
googleMapMarker.addListener("click", () => {
|
||||
if (onClick) onClick();
|
||||
});
|
||||
|
||||
setMarker(googleMapMarker);
|
||||
}
|
||||
|
||||
|
|
@ -79,19 +75,32 @@ const Marker: React.FC<MarkerProps> = (options) => {
|
|||
useEffect(() => {
|
||||
if (!marker) return;
|
||||
|
||||
marker.addListener("click", () => {
|
||||
google.maps.event.clearListeners(marker, "click");
|
||||
const clickListener = marker.addListener("click", () => {
|
||||
if (onClick) onClick();
|
||||
});
|
||||
|
||||
return () => {
|
||||
google.maps.event.removeListener(clickListener);
|
||||
};
|
||||
}, [marker, onClick]);
|
||||
|
||||
// add dragend event on marker
|
||||
useEffect(() => {
|
||||
if (!marker) return;
|
||||
if (!marker || !onDragEnd) return;
|
||||
|
||||
marker.addListener("dragend", (e: google.maps.MapMouseEvent) => {
|
||||
if (onDragEnd) onDragEnd(e);
|
||||
});
|
||||
}, [marker, options.onDragEnd]);
|
||||
google.maps.event.clearListeners(marker, "dragend");
|
||||
const dragEndListener = marker.addListener(
|
||||
"dragend",
|
||||
(e: google.maps.MapMouseEvent) => {
|
||||
if (onDragEnd) onDragEnd(e);
|
||||
},
|
||||
);
|
||||
|
||||
return () => {
|
||||
google.maps.event.removeListener(dragEndListener);
|
||||
};
|
||||
}, [marker, onDragEnd]);
|
||||
|
||||
return null;
|
||||
};
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user