Skip to content

fix(ios): send the current camera when onCameraChanged is registered - #4430

Open
aderiushev wants to merge 1 commit into
rnmapbox:mainfrom
aderiushev:fix/ios-camera-changed-on-register
Open

aderiushev wants to merge 1 commit into
rnmapbox:mainfrom
aderiushev:fix/ios-camera-changed-on-register

Conversation

@aderiushev

Copy link
Copy Markdown

Description

On iOS, MapView sends camera changes to JS only after JS has asked for them (setHandledMapChangedEventsWithEvents, which MapView calls from componentDidMount and again when a callback prop changes). A camera change before that is never sent, and nothing sends the camera afterwards. So when JS asks late, onCameraChanged gets no camera until the map moves:

  • when the screen keeps JS busy while the map mounts, so the start camera comes before componentDidMount (timing-dependent; we hit it every time in our app's map screen, which mounts many layers);
  • when onCameraChanged is set after the map has settled (always).

onMapIdle still reports the right camera, so an app that keeps the camera from onCameraChanged disagrees with the map until the user touches it.

This PR: when .cameraChanged is newly in the handled set, send the current camera once, as applyOnMapChange would (not when .regionIsChanging is registered, which takes precedence there). It goes through withMapView, so it waits for the map view. Registering the same set again (MapView does that on every render with an inline callback) sends nothing.

Android is not changed. There the handled set starts as null, so camera changes before the first registration are sent; a listener added later gets no camera until the map moves there too (from reading RNMBXMapView.kt, not run). I can add the same one-shot on Android if you want both to behave the same. #4426 fixes the Android start-camera loss from the 10 ms drop.

Checklist

  • I've read CONTRIBUTING.md
  • I updated the doc/other generated code with running yarn generate in the root folder (no changes)
  • I have tested the new feature on /example app.
    • In V11 mode/ios
    • In New Architecture mode/ios
    • In V11 mode/android
    • In New Architecture mode/android
  • I added/updated a sample - if a new feature was implemented (/example)

Screenshot OR Video

The reproducer below in the example app (main at 47a736a, React Native 0.84.1, New Architecture, Mapbox 11.33.0), iOS 26.5 simulator (iPhone 16). The map opens at zoom 12 and is never touched; each run is a fresh app launch.

main this PR
'busy' (JS busy 3 s while the map mounts), 10 runs: no onCameraChanged at all 3 of 10 0 of 10
'late' (onCameraChanged set at the first idle), 3 runs: onCameraChanged calls 0, 0, 0 1, 1, 1 (zoom 12.00, 3 ms after the idle)

In every run onMapIdle reported zoom 12.00. Without the busy child the start camera usually comes after componentDidMount, which is why the 'busy' case is timing-dependent; in one run with this PR the one-shot carried the map's first camera (zoom 0.47) and the start camera followed as a normal change. The text area has a fixed height: text that grows would resize the map, and a resize is a camera change of its own.

Component to reproduce the issue you're fixing

BugReportExample.js — 'busy' and 'late' modes, counts onCameraChanged for a map that is never moved
import React, { useLayoutEffect, useRef, useState } from 'react';
import { Text, View } from 'react-native';
import { Camera, MapView } from '@rnmapbox/maps';

// The map opens here and is never moved: the only camera change is the start camera.
const START = { centerCoordinate: [-74.00597, 40.71427], zoomLevel: 12 };

// Two ways JS asks for camera events after the start camera:
// 'busy': the screen keeps JS busy for 3 s while the map mounts (as a screen with many layers and
//         stores does), so MapView's componentDidMount, which asks native for camera events, runs
//         late. Timing-dependent.
// 'late': onCameraChanged is set only once the map is idle. Deterministic.
const MODE = 'busy';

const BusyOnMount = () => {
  useLayoutEffect(() => {
    const end = Date.now() + 3000;
    while (Date.now() < end) {
      // busy
    }
  }, []);

  return null;
};

const BugReportExample = () => {
  const mountedAt = useRef(Date.now());
  const [listening, setListening] = useState(MODE === 'busy');
  const [camera, setCamera] = useState({ calls: 0, first: '-' });
  const [idle, setIdle] = useState('-');

  const onCameraChanged = (state) => {
    const { zoom } = state.properties;

    setCamera((c) => ({
      calls: c.calls + 1,
      first: c.calls === 0 ? `zoom ${zoom.toFixed(2)} at +${Date.now() - mountedAt.current} ms` : c.first,
    }));
  };

  // onMapIdle carries the camera the map really has
  const onMapIdle = (state) => {
    setIdle(`zoom ${state.properties.zoom.toFixed(2)} at +${Date.now() - mountedAt.current} ms`);
    setListening(true);
  };

  return (
    <View style={{ flex: 1 }}>
      <MapView
        style={{ flex: 1 }}
        onCameraChanged={listening ? onCameraChanged : undefined}
        onMapIdle={onMapIdle}
      >
        <Camera defaultSettings={START} />
        {MODE === 'busy' && <BusyOnMount />}
      </MapView>
      {/* A fixed height, so text that grows does not resize the map (a resize is a camera change) */}
      <View style={{ height: 96 }}>
        <Text testID="camera">{`${MODE} | onCameraChanged calls: ${camera.calls} | first: ${camera.first}`}</Text>
        <Text testID="idle">{`onMapIdle: ${idle}`}</Text>
      </View>
    </View>
  );
};

export default BugReportExample;

🤖 Generated with Claude Code

@mfazekas mfazekas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this reproduces cleanly. On main (iOS sim, RN 0.84.1, new arch), attaching onCameraChanged after the map settled gave no event until the camera moved, and with 3 s of JS busy work during mount 10 of 10 runs got no camera at all. With this PR the late attach gets exactly one event with the current camera (zoom 12.00, isGestureActive: false, same payload keys as a normal event). The busy mount gets exactly one event in 10 of 10 runs. Five re-renders with an inline callback send nothing extra, normal camera changes are unaffected, and with onRegionIsChanging registered no one-shot is sent.

A few requests before merging:

  1. Please move the event construction into a small helper (e.g. sendCameraChanged()) and use it from both applyOnMapChange and here, so the one-shot can't drift from the regular cameraChanged payload.
  2. withMapView doesn't wait here: RNMBXMapViewModule.setHandledMapChangedEvents already rejects when the view has no map, and it runs on the main thread with _mapView set, so the callback always runs synchronously. Either call it directly or keep withMapView as a guard, but the "waits for the map view" part of the description isn't accurate.
  3. The comment can be one sentence, e.g. // Camera changes before JS registers for them are never sent, so send the current camera when it first does. The "as applyOnMapChange would" part goes away with the helper.
  4. Android parity: the late-attach case has the same gap there (setHandledMapChangedEvents only stores the set). I'd prefer both platforms in this PR, since the change is small. Treat the initial null set as already containing CAMERA_CHANGED, so the first registration doesn't resend the startup camera, which Android already delivers. Then emit handleMapChangedEvent(CAMERA_CHANGED) when it is newly added and REGION_IS_CHANGING isn't registered.
Reproducer used (Late / Busy / Late+Region, Attach, Detach, Re-render x5, setCamera)
import React, { useLayoutEffect, useRef, useState } from 'react';
import { Button, Text, View } from 'react-native';
import { Camera, MapView } from '@rnmapbox/maps';

// Repro for rnmapbox/maps#4430: onCameraChanged gets no camera when it is registered after the
// start camera. The map is never touched unless "setCamera" is pressed.
const START = { centerCoordinate: [-74.00597, 40.71427], zoomLevel: 12 };

const BusyOnMount = () => {
  // Delays MapView.componentDidMount (which registers the handled events) by 3 s of JS busy work.
  useLayoutEffect(() => {
    const end = Date.now() + 3000;
    while (Date.now() < end) {}
  }, []);
  return null;
};

const fmt = (s) =>
  s
    ? `z ${s.properties.zoom.toFixed(2)} c ${s.properties.center
        .map((v) => v.toFixed(3))
        .join(',')} gest ${s.gestures?.isGestureActive}`
    : '-';

const Repro = ({ mode, onRegion }) => {
  const t0 = useRef(Date.now());
  const camera = useRef(null);
  const [listening, setListening] = useState(mode === 'busy');
  const [, setTick] = useState(0);
  const [cam, setCam] = useState({ calls: 0, first: null, last: null, firstAt: 0 });
  const [region, setRegion] = useState(0);
  const [idle, setIdle] = useState(null);

  const log = (kind, payload) =>
    console.log(`[R4430] ${mode} +${Date.now() - t0.current}ms ${kind} ${JSON.stringify(payload)}`);

  return (
    <View style={{ flex: 1 }}>
      <MapView
        style={{ flex: 1 }}
        // inline on purpose: a new function every render, so MapView re-registers events each render
        onCameraChanged={
          listening
            ? (s) => {
                log('onCameraChanged', s);
                setCam((c) => ({
                  calls: c.calls + 1,
                  first: c.first ?? s,
                  firstAt: c.first ? c.firstAt : Date.now() - t0.current,
                  last: s,
                }));
              }
            : undefined
        }
        onRegionIsChanging={
          onRegion
            ? (f) => {
                log('onRegionIsChanging', f.properties);
                setRegion((n) => n + 1);
              }
            : undefined
        }
        onMapIdle={(s) => {
          log('onMapIdle', s);
          setIdle(s);
        }}
      >
        <Camera ref={camera} defaultSettings={START} />
        {mode === 'busy' && <BusyOnMount />}
      </MapView>
      {/* fixed height: a map resize would itself be a camera change */}
      <View style={{ height: 170 }}>
        <Text testID="r-cam">{`${mode} listening=${listening} calls=${cam.calls} first@+${cam.firstAt}ms`}</Text>
        <Text testID="r-first">{`first: ${fmt(cam.first)}`}</Text>
        <Text testID="r-last">{`last: ${fmt(cam.last)}`}</Text>
        <Text testID="r-idle">{`idle: ${fmt(idle)} region=${region}`}</Text>
        <View style={{ flexDirection: 'row', flexWrap: 'wrap' }}>
          <Button testID="r-attach" title="Attach" onPress={() => setListening(true)} />
          <Button testID="r-detach" title="Detach" onPress={() => setListening(false)} />
          <Button testID="r-rerender" title="Re-render x5" onPress={() => [1, 2, 3, 4, 5].forEach((i) => setTimeout(() => setTick((n) => n + 1), i * 100))} />
          <Button testID="r-move" title="setCamera z14" onPress={() => camera.current?.setCamera({ zoomLevel: 14, animationDuration: 0 })} />
        </View>
      </View>
    </View>
  );
};

const CameraOnRegisterRepro = () => {
  const [run, setRun] = useState({ key: 0, mode: 'late', onRegion: false });
  const start = (mode, onRegion = false) => setRun((r) => ({ key: r.key + 1, mode, onRegion }));
  return (
    <View style={{ flex: 1 }}>
      <View style={{ flexDirection: 'row', flexWrap: 'wrap', paddingTop: 4 }}>
        <Button testID="r-late" title="Late" onPress={() => start('late')} />
        <Button testID="r-busy" title="Busy" onPress={() => start('busy')} />
        <Button testID="r-late-region" title="Late+Region" onPress={() => start('late', true)} />
      </View>
      <Repro key={run.key} mode={run.mode} onRegion={run.onRegion} />
    </View>
  );
};

export default CameraOnRegisterRepro;

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants