FIX: Prevent `LockOn` conflicts (PR #10422)

If there’s already a LockOn instance, don’t create a new one.

Fixes a shaky viewport effect after certain transitions:

(as in #10421, the change doesn’t have a test. Functions like scrollToPost or scrollToElement don’t have any effect in the test environment)

GitHub

One side effect of this is that if finished is not called for some reason then we won’t be able to lock on ever again in the same browser session.

It might be better if we found an existing lockon instance to call a destroy() or similarly named method to make it stop working, then replace it with a new one.

That’s a good point, but I think we should handle this potential error state in the LockOn class. I’ve added the note about this in the refactor PR (3.)

I tried the replace-if-present first, but in the situation illustrated in the gif (i.e. transitioning to a post via a link with an anchor) the order of installed locks is: “scroll to the anchor” first, then “scroll to the post”. Hence discarding if a lock-on is present.

Ideally, there would be just a single lock-on to begin with, but both make sense where they are - one being our default scroll-to-element lock installed in handleURL and the other being installed by the topic-from-params route (via DiscourseURL.jumpToPost call)


(some time later)

Okay, here’s an attempt to make lockOn replace-if-present: https://github.com/discourse/discourse/commit/d300eab4b4f45038046712ae6bebfad8bee7614c

It uses the _discourse_anchor (from the other PR) and then passes it around through two transitions and a controller setup.

(It’s just a reference, i.e. doesn’t work on it’s own :laughing: It requires the commits from the refactor, and it also uncovered an unrelated issue that has yet to be solved. damn you, fontawesome sprites!)

Sure, I wouldn’t mind trying out this approach!

Updated this PR :slightly_smiling_face:

Let’s try it out!