mapbox / mapbox-gl-js

Interactive, thoroughly customizable maps in the browser, powered by vector tiles and WebGL
https://docs.mapbox.com/mapbox-gl-js/
Other
11.25k stars 2.23k forks source link

Map `idle` event not firing since 3.4 release #13236

Closed ianformanek closed 4 months ago

ianformanek commented 4 months ago

mapbox-gl-js version: 3.4.0, 3.5.X

browser: All

Steps to Trigger Behavior

  1. Create mapbox-gl map instance using new mapboxgl.Map(...)
  2. Add idle event handler via map.on('idle', () => { ...}); or map.once('idle', () => { ...});
  3. Perform some map operation to make sure the map was not idle already before adding the listener
  4. Observe the idle callback is never called

Link to Demonstration

Working correctly in 3.3: https://codepen.io/cluvio/pen/dyBXjjw Not working in 3.4: https://codepen.io/cluvio/pen/vYqKaaM Not working in 3.5.2: https://codepen.io/cluvio/pen/eYwzjXy

(the only difference between the 3 codepens is the HTML script + stylesheet URLs)

Expected Behavior

idle event handler is called. (or, if map is already idle, have some kind of way of detecting that, as suggested in #12964)

Actual Behavior

In 3.3 and before, the idle event handler is called correctly, in 3.4 and later, the idle event handler is never called. This does not seem to be caused by map-is-already-idle when adding the event listener.

mourner commented 4 months ago

Technically, the map is never idle when you pass repaint: true to options. Can you add more context on why you need to set that option to true? We exposed this option in v3.4, that's why it "works" in older versions (because this option has no effect).

ianformanek commented 4 months ago

Technically, the map is never idle when you pass repaint: true to options. Can you add more context on why you need to set that option to true? We exposed this option in v3.4, that's why it "works" in older versions (because this option has no effect).

This is a good point, indeed the repaint: true is causing the issue starting 3.4.

The repaint: true in our case comes from a long time ago, when our mapbox-gl version was 0.23.0. It seems the repaint property back then was used for something different (this PR is one of the few things I found from that time) and we probably used it to work around some issues with updating data on existing map instance.

With the new semantics, it might be safer to name the property with a distinct/unique name (debugContinuousRepaint), but our ancient leftover is probably not a strong enough trigger for this, so just a minor suggestion.

In any case, we will remove the repaint: true which is all that is needed for our case, thanks for the quick help!