[google_maps_flutter_web] Add Advanced markers support (web) #9773

aednlaxer · 2025-08-08T12:51:25Z

This PR adds Advanced markers support to the web implementation of google_maps_flutter.
Approved combined PR: #7882
Approved and merged platform interface PR: #9737
Issue: #155526

Pre-Review Checklist

gemini-code-assist

Code Review

This PR adds support for Advanced Markers to the web implementation of google_maps_flutter. The changes are extensive and well-structured, refactoring the marker handling logic to support both legacy and advanced markers through generics and abstract classes. New tests have been added for the new functionality, and existing ones have been updated. The implementation looks solid, but I've found a couple of critical issues that would prevent the code from compiling or running correctly.

gemini-code-assist · 2025-08-08T12:54:24Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_controller.dart

+          clusterManagersController: clusterManagersController!
+              as ClusterManagersController<gmaps.AdvancedMarkerElement>,


There's a typo here. clusterManagersController is used, but it should be _clusterManagersController. This will cause a compilation error as clusterManagersController is not defined in this scope.

clusterManagersController: _clusterManagersController! as ClusterManagersController<gmaps.AdvancedMarkerElement>,

Obviously this does compile, but it does make it order-sensitive to when the private variable is set, which we don't want, so this should use _clusterManagersController.

packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart

stuartmorgan-g · 2025-08-19T18:42:29Z

packages/google_maps_flutter/google_maps_flutter_web/lib/google_maps_flutter_web.dart

 import 'package:google_maps_flutter_platform_interface/google_maps_flutter_platform_interface.dart';
 import 'package:sanitize_html/sanitize_html.dart';
 import 'package:stream_transform/stream_transform.dart';
+import 'package:web/web.dart' as web;


Why is this being imported twice, once with a namespace?

It's imported with namespace because some types are found from multiple packages. I think we can remove the second import and use to these types with "web" namespace everywhere.

stuartmorgan-g · 2025-08-19T18:45:51Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_controller.dart

+          clusterManagersController: clusterManagersController!
+              as ClusterManagersController<gmaps.AdvancedMarkerElement>,


Obviously this does compile, but it does make it order-sensitive to when the private variable is set, which we don't want, so this should use _clusterManagersController.

stuartmorgan-g · 2025-08-19T18:51:06Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/marker.dart

+/// The `MarkerController` class wraps a [gmaps.AdvancedMarkerElement]
+/// or [gmaps.Marker], how it handles events, and its associated (optional)
+/// [gmaps.InfoWindow] widget.
+abstract class MarkerController<T, O> {


I missed this before; isn't this a breaking change for this package? It looks like this (via the part directive) is public API, and the public constructor can no longer be directly called.

That's not a significant issue if we don't expect people to generally be creating one directly, but it does mean we need to version this PR as a major version change.

Since the web package is still in the pre-release version I raised the version number to 0.6.0.

stuartmorgan-g · 2025-08-19T18:53:44Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/markers.dart

+///
+/// [T] must extend [JSObject]. It's not specified in code because our mocking
+/// framework does not support mocking JSObjects.
+abstract class MarkersController<T, O> extends GeometryController {


Same here; this looks breaking.

stuartmorgan-g · 2025-08-19T18:54:11Z

packages/google_maps_flutter/google_maps_flutter_web/pubspec.yaml

  sanitize_html: ^2.0.0
  stream_transform: ^2.0.0
-  web: ">=0.5.1 <2.0.0"
+  web: ">=1.0.0 <2.0.0"


This can just be ^1.0.0

stuartmorgan-g · 2025-09-23T18:35:03Z

From triage: @aednlaxer Are you still planning on updating this PR?

stuartmorgan-g · 2025-09-24T14:26:22Z

FYI: After merging in main, you'll need to update the license header in all the new files with a new copy from an existing file, as the format has changed slightly.

aednlaxer · 2025-09-24T14:34:36Z

From triage: @aednlaxer Are you still planning on updating this PR?

Yes, please don't close it

mdebbar

Overall, it looks good to me! I just have a few minor code improvements.

mdebbar · 2025-10-01T14:19:31Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart

+        options.glyphColor = _getCssColor(circleGlyph.color);
+      case final TextGlyph textGlyph:
+        final web.Element element = web.document.createElement('p');
+        element.innerHTML = textGlyph.text.toJS;


It's safer to use element.text:

Suggested change

element.innerHTML = textGlyph.text.toJS;

element.text = textGlyph.text;

mdebbar · 2025-10-01T14:22:30Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart

+          element.setAttribute(
+            'style',
+            'color: ${_getCssColor(textGlyph.textColor!)}',
+          );


Suggested change

element.setAttribute(

'style',

'color: ${_getCssColor(textGlyph.textColor!)}',

);

element.style.color = _getCssColor(textGlyph.textColor!);

mdebbar · 2025-10-01T14:34:50Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart

+  icon.setAttribute(
+    'style',
+    <String>[
+      if (size != null) ...<String>[
+        'width: ${size.width.toStringAsFixed(1)}px;',
+        'height: ${size.height.toStringAsFixed(1)}px;',
+      ],
+      if (opacity != null) 'opacity: $opacity;',
+      if (isVisible != null) 'visibility: ${isVisible ? 'visible' : 'hidden'};',
+    ].join(' '),


Suggested change

icon.setAttribute(

'style',

<String>[

if (size != null) ...<String>[

'width: ${size.width.toStringAsFixed(1)}px;',

'height: ${size.height.toStringAsFixed(1)}px;',

],

if (opacity != null) 'opacity: $opacity;',

if (isVisible != null) 'visibility: ${isVisible ? 'visible' : 'hidden'};',

].join(' '),

final iconStyle = icon.style;

if (size != null) {

iconStyle

..width = '${size.width.toStringAsFixed(1)}px'

..height = '${size.height.toStringAsFixed(1)}px';

}

if (opacity != null) {

iconStyle.opacity = opacity;

}

if (isVisible != null) {

iconStyle.visibility = isVisible ? 'visible' : 'hidden';

}

mdebbar · 2025-10-01T14:46:21Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/marker.dart

+  });
+
+  @override
+  void initializeMarkerListener({


We typically use the "add listener" terminology in the flutter code base: https://api.flutter.dev/flutter/search.html?q=addlistener

What do you think about renaming this to addMarkerListeners?

mdebbar · 2025-10-01T14:46:53Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/marker.dart

-    if (_infoWindow != null && newInfoWindowContent != null) {
-      _infoWindow.content = newInfoWindowContent;
+    if (onTap != null) {
+      marker.onClick.listen((gmaps.MapMouseEvent event) {


Do we need to unsubscribe from these listeners when the marker is removed?

If yes, it can easily be done:

_subscriptions = [ if (onTap != null) marker.onClick.listen((_) => onTap.call()), if (onDragStart != null) marker.onDragstart.listen((event) { marker.position = event.latLng; onDragStart.call(event.latLng ?? _nullGmapsLatLng); }), ... ];

and later in void remove():

_subscriptions?.forEach((sub) => sub.cancel); _subscriptions = null;

mdebbar · 2025-10-01T14:47:19Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/marker.dart

+    required VoidCallback? onTap,
+  }) {
+    if (onTap != null) {
+      marker.onClick.listen((gmaps.MapMouseEvent event) {


Same question about unsubscribing from listeners.

aednlaxer requested a review from ditman as a code owner August 8, 2025 12:51

github-actions bot added p: google_maps_flutter platform-web labels Aug 8, 2025

gemini-code-assist bot reviewed Aug 8, 2025

View reviewed changes

stuartmorgan-g requested changes Aug 19, 2025

View reviewed changes

stuartmorgan-g mentioned this pull request Sep 24, 2025

[google_maps_flutter] Add Advanced Markers support #7882

Open

11 tasks

aednlaxer added 2 commits September 25, 2025 16:46

Add Advanced markers support (web)

2c8ffc2

Set minimum version of web package to 1.0.0

65b8c81

illuminati1911 force-pushed the feature/advanced_markers_google_maps_flutter_web branch from 9da73a7 to 65b8c81 Compare September 25, 2025 08:48

illuminati1911 requested a review from mdebbar as a code owner September 25, 2025 08:48

illuminati1911 added 5 commits September 26, 2025 09:59

Advanced markers package pr fixes

de3c41a

Unify web package double import

02d7aca

Raise google maps flutter web package to version 0.6.0

3214761

Fix get marker id function in google maps flutter web package

0e1a413

Updated license headers in the google maps flutter web package

b6b799a

aednlaxer requested a review from stuartmorgan-g September 29, 2025 06:55

mdebbar reviewed Oct 1, 2025

View reviewed changes

		clusterManagersController: clusterManagersController!
		as ClusterManagersController<gmaps.AdvancedMarkerElement>,

	element.innerHTML = textGlyph.text.toJS;
	element.text = textGlyph.text;

[google_maps_flutter_web] Add Advanced markers support (web) #9773

Are you sure you want to change the base?

[google_maps_flutter_web] Add Advanced markers support (web) #9773

Uh oh!

Conversation

aednlaxer commented Aug 8, 2025

Pre-Review Checklist

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Sep 23, 2025

Uh oh!

stuartmorgan-g commented Sep 24, 2025

Uh oh!

aednlaxer commented Sep 24, 2025

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!