Fix issue whereby tooltips loaded dynamically while moving the map were never shown.

[theGOTOguy]

Slava Ukraini, leaflet team!

This pull request resolves #8591, whereby Tooltips were not added to the DOM while the map was moving.

@BasieP's analysis of the offending code was correct, as is the workaround suggested; However, I am using react-leaflet, where this workaround is not an option.

An automatic test is included.

[jonkoops]

@theGOTOguy could you stop adding new commits to this PR? It creates a lot of unnecessary noise. Besides, we will rebase and quash the commits either way.

[jonkoops]

Also please rebase your PR on main so we can merge if needed (no merge commits).

[Falke-Design]

The check for map dragging was added in #7862 to prevent adding / removing of the tooltip when occasionally hovering over the marker with the mouse while dragging.

[Falke-Design]

@mourner @jonkoops @IvanSanchez do you see any problems with setting the flags to false before fireing the event?

[theGOTOguy]

It seems like the only way this would matter would be if someone was looking at _moving or _dragging during the handling of a dragend event. We're not doing it internally to Leaflet, but it's possible that someone who depends on us might. To do so would be inadvisable, because there would be a race between the event firing and _moving and _dragging being updated after the event is fired. Maybe mark this as a breaking API change in case someone depends on the state of _moving or _dragging in an event triggered by dragend?

[jonkoops]

I have no issue with that, this is internal state so folks should not be relying on it anyways.

[Falke-Design]

@mourner @jonkoops @IvanSanchez have someone a good Idea to make this cleaner? The once listener should not be added multiple times and it is important to pass e as argument from the original _openTooltip call.

[theGOTOguy]

Here's a couple of ideas:

Superficially, you could do something like this:

                // If the map is moving, we will show the tooltip after it's done.
                if (this._map.dragging && this._map.dragging.moving() && !this._openOnceFlag) {
                        this._openOnceFlag = true;
                        this._map.once('moveend', () => {
                            this._openOnceFlag = false;
                            this._openTooltip(e);
                        });
                        return;
                }

It's not satisfying because we still have to store _openOnceFlag.

However, maybe there's a better solution. There are only four places where _openTooltip is called at all, and they're all here in Tooltip.js.

We could replace all instances of _openTooltip with _openTooltipWhenNotMoving, with:

_openTooltipWhenNotMoving(e) {
    if (this._map.dragging && this._map.dragging.moving()) {
        this._map.once('moveend', () => {
            this._openTooltip(e);
        });
    } else {
        this._openTooltip(e);
    }
}

Then remove the moving checks from _openTooltip proper.

In this way, we avoid storing an extra variable, and call _openTooltip at most once at an appropriate time when it would be needed.

Edit: I'm concerned that the _openOnceFlag might actually be incorrect. If the Tooltip was closed for any reason (say, permanent is set to false by some external code), then we could find that if it is later set to true again then it would no longer appear if _openTooltip is called while dragging.

[theGOTOguy]

I've implemented the former solution, which at least resolves a little bit of the nesting.

I don't see a better solution than the _openOnceFlag that you implemented, and I've convinced myself that it is correct even in case where some external code toggles permanent.

[theGOTOguy]

This is still an issue for me in production, and I'm wondering if you could take a look at the changes I've made and let me know if the solution as it sits is appropriate or if more changes are desired.

[Falke-Design]

Fine for me but we need an approve from another maintainer @IvanSanchez @mourner