Alternate fix for PopUp keepInView recursion and speed up associated test

[rjackson]

Related to #8483

I was reviewing the PopUp keepInView test with the eye of speeding it up, however I found that the pan animation was forced on. This was introduced with #7792 to guard against infinite recursion from (on moveend) -> _adjustPan -> panBy -> _resetView -> (fire moveend).

Reviewing the Popup code, we always expect _adjustPan to be triggered by its own panBy call but ordinarily the view will have updated and the dx and dy offsets will be 0, avoiding the recursive call. We only end up with the recursive call when _resetView is called, because the map doesn't have a chance to re-render itself (is that right?).

The fact that _adjustPan will always invoke itself allows an alternate solution: We can set a local variable when we start autopanning, and check it in the later invocation. If its present, we know the moveend currently being handled was invoked by the same function, and can thus return early – also avoiding the recursive invocation.

With this change, the associated test can be simplified and sped up from 812ms to 10ms (81x faster).

I'm raising this as a draft for now as I'm keen for thoughts on this as an alternate fix. I also want to do some manual testing varying pan distances and animations to further validate the change.

[rjackson]

I'm happy opening this up as ready for review :)

I've added a debug page and had a play around with the keepInView popups. From my testing, I can confirm that the _autopanning local variable appears to be a reliable fix for the freeze issue that #7792 addressed.

As described in my comment earlier, I also noticed that there was no way to disable pan animations invoked by the keepInView logic. That meant my original attempt to speed up these tests was actually fragile and could break depending on future changes in Map.panBy even if the autopanning behaviour does not.

I have adjusted the test to work more reliably in that case, and added a new "short hop" to make sure we have coverage of Map.panBy's animated pan and reset view behaviours.

After these changes, we're now seeing a speed up from 812ms to 271ms (3x faster).

The resulting tests now also have no dependence on the _autopanning alternate fix. If we think that change is too risky, we can drop it but still keep the faster tests :)

[mourner]

Thank you for such a thorough and thoughtful approach to PRs! This looks great.

To be honest, that adjustPan / panBy etc. flow was written such a long time ago that I don't really remember how it's supposed to work. But intuitively, the _autopanning flag should be the better approach because it's explicit and it's much clearer what's going on. I'll let one more person to review though to make sure I'm not missing anything, cc @Falke-Design

[Falke-Design]

Changing latlng with keepInView: false, doesn't move the map anymore:

var p = L.popup({keepInView: false})
        .setContent('Popup')
        .setLatLng(map.getBounds().getNorthEast());
map.openPopup(p);

setTimeout(()=>{
	p.setLatLng(map.getBounds().getNorthEast());
	console.log('map should move')
}, 1000)

[rjackson]

Changing latlng with keepInView: false, doesn't move the map anymore

Oop, good catch! Thanks!

I've attached a quick fix for that by only setting the _autopanning flag when keepInView is set. :)

[jonkoops]

@Falke-Design could you give this PR another look?

[Falke-Design]

Very nice, thank you @rjackson