Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Improper setup of max zoom levels now (since v5) allows map to become unresponsive #1566

Closed
androidseb opened this issue Jun 25, 2023 · 11 comments · Fixed by #1578
Closed
Labels
bug This issue reports broken functionality or another error P: 2 (soon™?)

Comments

@androidseb
Copy link
Contributor

What is the bug?

When you zoom in too much with gestures (e.g. pinch to zoom or mouse wheel), the map UI will break and become blank. Further new gestures will be ignored. Unless the map view controller is called to zoom back out, the map will remain stuck in this state.

It seems like this bug appeared in version 5.0.0, although I haven't tested in 4.0.0. As far as I can tell from my extensive usage, that bug is not present in version 3.1.0.

How can we reproduce it?

Summary

At the time of writing (June 25th 2023), the official Flutter Map demo page available here has this issue:
https://demo.fleaflet.dev/

I'm having this issue on mobile devices with the pinch to zoom gesture as well, but I'll describe the reproductions steps on the demo page since it's the most straightforward way to reproduce the issue.

Repro steps

  • Open the Flutter Map demo page here: https://demo.fleaflet.dev/ (tested this with Firefox and Chrome)
  • Use the mouse wheel to zoom in, until the UI breaks and the map view becomes blank
  • At this point: the map view is blank and unresponsive, the user is stuck and can no longer pan or zoom out

Notes

If you have access to the map view controller (e.g. zoom buttons) to zoom out, the user can get out of this situation: https://demo.fleaflet.dev/plugin_zoombuttons

Do you have a potential solution?

No response

Platforms

Android,iOS,Web

Severity

Obtrusive: Prevents normal functioning but causes no errors in the console

@androidseb androidseb added the bug This issue reports broken functionality or another error label Jun 25, 2023
@ibrierley
Copy link
Collaborator

Have you tried adding a maxZoom ?

@androidseb
Copy link
Contributor Author

Yes, in my specific use case, it seems like changing the maxZoom value of the map view from 23 to 19 prevents the issue from happening.

@androidseb
Copy link
Contributor Author

OK, so I found the source of the problem in my app's code, I was using flutter_map wrong:

  • For the FlutterMap widget's options I was using maxZoom: 23
  • For the TileLayer object, I was using maxZoom: 21

Whenever I was zooming past level 21, the bug was happening.

Version 3.1.0 of flutter_map was more forgiving around this incorrect usage of the API, I'm assuming it was correcting the maxZoom value of the TileLayer based on the value of the FlutterMap widget's options.

I'll look at the demo app's code and see if there is a quick fix I can suggest as a PR...

@androidseb
Copy link
Contributor Author

Looking at https://github.com/fleaflet/flutter_map/blob/master/example/lib/pages/home.dart, I think I see what is happening...

This is the code in question:

//...
//STRIPPED SOME CODE HERE
//...
            child: FlutterMap(
              options: MapOptions(
                center: const LatLng(51.5, -0.09),
                zoom: 5,
                maxBounds: LatLngBounds(
                  const LatLng(-90, -180),
                  const LatLng(90, 180),
                ),
              ),
//...
//STRIPPED SOME CODE HERE
//...
              children: [
                TileLayer(
                  urlTemplate:
                      'https://tile.openstreetmap.org/{z}/{x}/{y}.png',
                  userAgentPackageName: 'dev.fleaflet.flutter_map.example',
                ),
//...
//STRIPPED SOME CODE HERE
//...

My interpretation of why this bug is happening:

  • The MapOptions constructor has no maxZoom value specified so it remains null, which if I read the flutter_map library's code properly means the zoom is not limited, the user can attempt to zoom on the map as much as they want? @ibrierley did I get that right?
  • The TileLayer constructor has no maxZoom value specified, so it defaults to 18
  • As soon as the user zooms further than 18, the issue occurs

The easy fix here would be to specify the max zoom levels explicitly for every single page of the demo's code, but I feel like omitting parameters shouldn't cause this undesired behavior, ideally the flutter_map library can handle the case of Tile Layer and map options not matching in maxZoom spec more gracefully.

I'll try to look at the library's code a little more to see if I can find an idea for this improved handling...

@JaffaKetchup JaffaKetchup changed the title [BUG] Zoom controls can break the map UI if zooming too much [BUG] Improper setup of max zoom levels now (since v5) allows map to become unresponsive Jun 25, 2023
@JaffaKetchup
Copy link
Member

Thanks for reporting!
Tbh, I'm not even sure maxZoom makes sense on a TileLayer. maxNativeZoom sure, but there's no point limiting a single layer and not the others? What do you guys think?

@androidseb
Copy link
Contributor Author

@JaffaKetchup if you want to set up your map with different providers based on the zoom, maxZoom on the TileLayer might be useful? For example you could have a TileLayer for zoom levels 1 to 15 and then another TileLayer for zoom levels 16 to 23...

I feel like the ideal implementation from an API consumer's perspective would be to:

  • have TileLayer.maxZoom nullable and optional, defaulting to null (instead of 18)
    • if TileLayer.maxZoom is null, then the maxZoom of the map view is used as a fallback option
    • if TileLayer.maxZoom for the TileLayer is lower than the current map zoom, then the tiles stop showing
  • have the map view keep working if no Tile is available / showing

@JaffaKetchup
Copy link
Member

I think that's probably a good idea. I can't understand why the map stops working though, so that'll need more investigation.

@rorystephenson
Copy link
Contributor

rorystephenson commented Jul 13, 2023

When we go above TileLayer's maxZoom it is no longer shown. If you have something else showing, e.g. a Marker, my experience is that if you place the mouse over that thing then you can still perform gestures. So if a Marker is still visible and you place the mouse over it and scroll it will work. I haven't dug deeper but what I think is happening is that there is no longer a visible child widget when the TIleLayer appears so the gesture detection is not firing.

If this only started occuring in 5.0.0 I'm not 100% sure why, are we sure it wasn't broken in 4.0.0? I can see how my changes to TileLayer in 4.0.0 may have caused this as prior to 4.0.0 TileLayer had a backgroundColor option which I removed. If you have a backgroundColor you will always have a hit-testable child and gestures will always work.

I think the best solution is probably making the gesture detection hitTestBehaviour opaque instead of deferToChild. That way we could have multiple TileLayers with different max/min zoom as suggested above and if we go above the maxZoom gestures will still work.

EDIT: I've just tried this on the web demo using Chrome and I don't seem to be having this issue. Can someone else confirm?

@rorystephenson
Copy link
Contributor

rorystephenson commented Jul 13, 2023

I think this is 'fixed' by #1578.

I would've thought that it would be better to just wrap FlutterMap in a ColoredBox if you want a background color or to add a child below the TileLayer rather than making it an option of FlutterMap. That would break the gestures again when zoomed above maxZoom but setting the HitDetectionBehaviour to opaque should solve that. This avoids an extra widget in the widget tree when no background color is necessary as well.

@JaffaKetchup
Copy link
Member

Marking as fixed by #1578.

Although it is true that it adds an extra widget, it is a single 'not very deeply nested' and relatively cheap widget. Fiddling around with the hit detection behaviour may also cause issues when the map is inside a scrollable - but I'm not sure? Background color is a simple property and I don't want to make users have to do more just to specify one simple thing.

@rorystephenson
Copy link
Contributor

rorystephenson commented Jul 13, 2023

Although it is true that it adds an extra widget, it is a single 'not very deeply nested' and relatively cheap widget. Fiddling around with the hit detection behaviour may also cause issues when the map is inside a scrollable - but I'm not sure? Background color is a simple property and I don't want to make users have to do more just to specify one simple thing.

Yeah actually you're right that it makes sense for ease of use and the single extra widget is not a big deal.

FWIW I did some quick tests locally setting the hit test behaviour to opaque and removing the backgroundColor ColoredBox and it works without affecting the behaviour of the map in scrollables but it's not necessary given that there will always be a ColoredBox child.

I've submitted #1584 to add a test to make sure we don't accidentally break this in the future (plus a small tweak to remove the TileLayer ColoredBox unless the deprecated TileLayer backgroundColor is defined).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue reports broken functionality or another error P: 2 (soon™?)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants