Skip to content

Commit 232593c

Browse files
evil159claude
authored andcommitted
Fix black flash when removing/re-adding annotations (#12326)
## Summary - Fixes [MAPSIOS-2180](https://mapbox.atlassian.net/browse/MAPSIOS-2180): Verizon reported a black flash when removing/re-adding polygon annotations (regression in v11.17.0). - Root cause: `removeGeoJSONSourceFeatures` is async (worker thread) while `setLayerProperties` is sync (immediate). When annotations were cleared, `syncLayer()` reset data-driven properties to literal style defaults (e.g. `fill-color: #000000`) before features were actually removed from the source — so for 1+ frames the still-visible features rendered with the black default. - Fix: in the "unused properties" reset path in `syncLayer()`, swap the literal style default for a coalesce expression that reads each feature's own stored value first (with the style default as fallback). Stale features waiting on the async remove now keep their correct colors; no black flash. ### Why minimal A broader architectural refactor (coalesce expressions set once, manager values merged into features, static data-driven/non-data-driven key sets via stylegen) was prototyped and reviewed but carries meaningful scope and risk for a hotfix affecting all annotation users and every downstream (Flutter SDK, Nav SDK). Tracked as a follow-up in [MAPSIOS-2185](https://mapbox.atlassian.net/browse/MAPSIOS-2185). ## Test plan - [x] Unit tests: `AnnotationManagerImplTests` + all four `*AnnotationManagerTests` suites pass locally on iPhone 16 Pro / iOS 18.6. - [x] Run the `PolygonAnnotationExample` reproducer (rapid remove/re-add cycle) — confirm no black flash. - [ ] Verify polygon renders correctly after re-add (fill-color, fill-opacity, fill-outline-color). - [ ] Test with manager-level properties (e.g. `polygonAnnotationManager.fillOpacity = 0.5`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) [MAPSIOS-2180]: https://mapbox.atlassian.net/browse/MAPSIOS-2180?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [MAPSIOS-2185]: https://mapbox.atlassian.net/browse/MAPSIOS-2185?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ cc @mapbox/maps-ios --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> GitOrigin-RevId: 0ffd039c5f6f563ededf18443e1b11c9695ad805
1 parent 8fdd1fb commit 232593c

10 files changed

Lines changed: 621 additions & 213 deletions

Sources/MapboxMaps/Annotations/AnnotationManagerImpl.swift

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,13 @@ final class AnnotationManagerImpl<AnnotationType: Annotation & AnnotationInterna
8080
/// the subsequent sync.
8181
private var previouslySetLayerPropertyKeys: Set<String> = []
8282

83+
/// Keys that have been data-driven in any previous sync (monotonic — never shrinks
84+
/// for the lifetime of the manager, bounded by the layer type's property count).
85+
/// Keeping these keys routed through the data-driven coalesce path ensures stale
86+
/// features (still visible while an async source removal is in flight) keep reading
87+
/// their own stored values instead of snapping to literal defaults — MAPSIOS-2180.
88+
private var previouslyDataDrivenLayerPropertyKeys: Set<String> = []
89+
8390
private var draggedAnnotationIndex: Array<PointAnnotation>.Index?
8491
private var destroyOnce = Once()
8592
private var syncSourceOnce = Once(happened: true)
@@ -289,31 +296,31 @@ final class AnnotationManagerImpl<AnnotationType: Annotation & AnnotationInterna
289296

290297
delegate?.syncImages()
291298

292-
// Construct the properties dictionary from the annotations
293-
let dataDrivenLayerPropertyKeys = Set(annotations.flatMap(\.layerProperties.keys))
299+
// Route both current-annotation keys and any previously-data-driven keys through the
300+
// coalesce path. Keeping previously-seen keys ensures stale features (still visible
301+
// while an async source removal is in flight) keep reading their own stored values
302+
// instead of snapping to a literal default — MAPSIOS-2180 black flash.
303+
let currentDataDrivenLayerPropertyKeys = Set(annotations.flatMap(\.layerProperties.keys))
304+
let dataDrivenLayerPropertyKeys = currentDataDrivenLayerPropertyKeys
305+
.union(previouslyDataDrivenLayerPropertyKeys)
294306

295-
/// The logic of the expression is the following
296-
/// Firstly, it tries to get the the value for the given key from `layerProperties` in feature. Properties for the feature are set in <Type>Annotation.feature
297-
/// Secondly, it tries to read the value from `layerProperties` dictionary from annotation manager.
298-
/// In the end if the property is not set either on annotation or on annotation manager we just use default value from the style.gst
299307
let dataDrivenProperties = Dictionary(
300-
uniqueKeysWithValues: dataDrivenLayerPropertyKeys.map { (key) -> (String, Any) in (key, [
301-
"coalesce",
302-
["get", key, ["get", "layerProperties"]],
303-
layerProperties[key] ?? StyleManager.layerPropertyDefaultValue(for: self.layerType, property: key).value
304-
] as [Any])})
308+
uniqueKeysWithValues: dataDrivenLayerPropertyKeys.map { (key) -> (String, Any) in
309+
(key, coalesceExpression(forKey: key))
310+
})
305311

306-
// Merge the common layer properties
312+
// Merge manager-level literals — data-driven coalesce wins on key collisions.
307313
let newLayerProperties = dataDrivenProperties.merging(layerProperties, uniquingKeysWith: { dataDriven, _ in dataDriven })
308314

309-
// Construct the properties dictionary to reset any properties that are no longer used
315+
// Remaining unused keys are manager-only literals the user removed. Reset to literal
316+
// style defaults — core maps doesn't always honor coalesce over non-data-driven paint properties.
310317
let unusedPropertyKeys = previouslySetLayerPropertyKeys.subtracting(newLayerProperties.keys)
311318
let unusedProperties = Dictionary(uniqueKeysWithValues: unusedPropertyKeys.map { (key) -> (String, Any) in
312319
(key, StyleManager.layerPropertyDefaultValue(for: self.layerType, property: key).value)
313320
})
314321

315-
// Store the new set of property keys
316322
previouslySetLayerPropertyKeys = Set(newLayerProperties.keys)
323+
previouslyDataDrivenLayerPropertyKeys = dataDrivenLayerPropertyKeys
317324

318325
// Merge the new and unused properties
319326
let allLayerProperties = newLayerProperties.merging(unusedProperties, uniquingKeysWith: { $1 })
@@ -331,6 +338,16 @@ final class AnnotationManagerImpl<AnnotationType: Annotation & AnnotationInterna
331338
}
332339
}
333340

341+
/// Builds the coalesce expression used for data-driven layer properties:
342+
/// feature's own `layerProperties` first, then the manager-level value, then the style default.
343+
private func coalesceExpression(forKey key: String) -> [Any] {
344+
return [
345+
"coalesce",
346+
["get", key, ["get", "layerProperties"]],
347+
layerProperties[key] ?? StyleManager.layerPropertyDefaultValue(for: self.layerType, property: key).value
348+
]
349+
}
350+
334351
// MARK: - User interaction handling
335352

336353
typealias TokenPair = (AnyCancelable, AnyCancelable)

Tests/MapboxMapsTests/Annotations/AnnotationManagerImplTests.swift

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,14 +209,79 @@ final class AnnotationManagerImplTests: XCTestCase {
209209
XCTAssertEqual(props2["x"] as? String, "X")
210210
XCTAssertEqual(props2["bar"] as? String, "qux")
211211

212-
// resets to default
212+
// `icon-allow-overlap` was never set by any annotation in this test, so it resets
213+
// to the literal style default — not a coalesce expression.
213214
let def = StyleManager.layerPropertyDefaultValue(for: .symbol, property: "icon-allow-overlap").value as? Bool
214215
XCTAssertEqual(props2["icon-allow-overlap"] as? Bool, def)
215216

216217
try checkExpression(key: "text-field", props: props2)
217218
XCTAssertEqual(props2.count, 6)
218219
}
219220

221+
// Regression test for MAPSIOS-2180: when a key that was data-driven (some annotation
222+
// carried it in its own layerProperties) becomes unused, it must reset as a coalesce
223+
// expression so that stale features awaiting the async source removal keep rendering
224+
// their own stored values instead of a literal default (which would cause a black flash
225+
// for fill-color).
226+
func testDataDrivenKeyResetsAsCoalesceExpression() throws {
227+
// Annotation with a per-annotation data-driven property
228+
me.annotations = [
229+
PointAnnotation(id: "foo", coordinate: .init(latitude: 0, longitude: 0))
230+
.textSize(10)
231+
]
232+
harness.triggerDisplayLink()
233+
style.setLayerPropertiesStub.reset()
234+
235+
// Remove all annotations — the key is now unused but was data-driven last sync
236+
me.annotations = []
237+
harness.triggerDisplayLink()
238+
239+
let props = try XCTUnwrap(style.setLayerPropertiesStub.invocations.last?.parameters.properties)
240+
let resetValue = try XCTUnwrap(props["text-size"] as? [Any])
241+
XCTAssertEqual(resetValue[0] as? String, "coalesce", "data-driven reset should be a coalesce expression")
242+
let lookup = try XCTUnwrap(resetValue[1] as? [Any])
243+
XCTAssertEqual(lookup[0] as? String, "get")
244+
XCTAssertEqual(lookup[1] as? String, "text-size")
245+
}
246+
247+
// Pins the contract that `previouslyDataDrivenLayerPropertyKeys` accumulates across syncs:
248+
// once a key has been data-driven, its coalesce expression keeps being pushed to the layer
249+
// on subsequent syncs — even for syncs whose current annotations no longer carry it, and
250+
// even across intervening annotations with entirely different properties.
251+
func testDataDrivenKeysAccumulateAcrossSyncs() throws {
252+
// Sync 1: annotation carries text-size
253+
me.annotations = [
254+
PointAnnotation(id: "a", coordinate: .init(latitude: 0, longitude: 0))
255+
.textSize(10)
256+
]
257+
harness.triggerDisplayLink()
258+
259+
// Sync 2: annotations removed
260+
me.annotations = []
261+
harness.triggerDisplayLink()
262+
263+
// Sync 3: different annotation with a different data-driven property (text-field)
264+
me.annotations = [
265+
PointAnnotation(id: "b", coordinate: .init(latitude: 0, longitude: 0))
266+
.textField("x")
267+
]
268+
harness.triggerDisplayLink()
269+
270+
// Sync 4: annotations removed again
271+
style.setLayerPropertiesStub.reset()
272+
me.annotations = []
273+
harness.triggerDisplayLink()
274+
275+
// At sync 4, BOTH text-size (from sync 1) and text-field (from sync 3) must still be
276+
// pushed as coalesce expressions — the accumulation is what keeps stale features
277+
// rendering their own colors during async removes for any key that has ever been data-driven.
278+
let props = try XCTUnwrap(style.setLayerPropertiesStub.invocations.last?.parameters.properties)
279+
let textSize = try XCTUnwrap(props["text-size"] as? [Any])
280+
XCTAssertEqual(textSize[0] as? String, "coalesce", "text-size coalesce should persist after later syncs")
281+
let textField = try XCTUnwrap(props["text-field"] as? [Any])
282+
XCTAssertEqual(textField[0] as? String, "coalesce", "text-field coalesce should persist after later syncs")
283+
}
284+
220285
@available(*, deprecated)
221286
func testHandleTap() throws {
222287
let delegate = Delegate()

Tests/MapboxMapsTests/Annotations/Generated/CircleAnnotationIntegrationTests.swift

Lines changed: 48 additions & 24 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)