fix: overlay remains visible on click when already moused over

If the overlay is visible due to a mouseover event and the user clicks a token to pin it, previously the overlay would briefly disappear as its data was reloaded. This could be worked around in the consumer by having replayable observables, but most consumers did not implement this correctly. Now, when a click event pins the overlay (and does not change its position from what the prior mouseover event yielded), it does not unsubscribe the previous data observables; it remains visible and subscribed.

Author: sqs

src/hoverifier.test.ts

@@ -98,6 +98,102 @@ describe('Hoverifier', () => {
         }
     })
 
+    it('pins the overlay without it disappearing temporarily on mouseover then click', () => {
+        for (const codeView of testcases) {
+            const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b))
+
+            const hover = {}
+            const defURL = 'def url'
+            const delayTime = 10
+
+            scheduler.run(({ cold, expectObservable }) => {
+                const hoverifier = createHoverifier({
+                    closeButtonClicks: NEVER,
+                    goToDefinitionClicks: new Observable<MouseEvent>(),
+                    hoverOverlayElements: of(null),
+                    hoverOverlayRerenders: EMPTY,
+                    fetchHover: createStubHoverFetcher(hover, delayTime),
+                    fetchJumpURL: createStubJumpURLFetcher(defURL, delayTime),
+                    pushHistory: noop,
+                    getReferencesURL: () => null,
+                })
+
+                const positionJumps = new Subject<{
+                    position: Position
+                    codeView: HTMLElement
+                    scrollElement: HTMLElement
+                }>()
+
+                const positionEvents = of(codeView.codeView).pipe(findPositionsFromEvents(codeView))
+
+                const subscriptions = new Subscription()
+
+                subscriptions.add(hoverifier)
+                subscriptions.add(
+                    hoverifier.hoverify({
+                        dom: codeView,
+                        positionEvents,
+                        positionJumps,
+                        resolveContext: () => codeView.revSpec,
+                    })
+                )
+
+                const hoverAndDefinitionUpdates = hoverifier.hoverStateUpdates.pipe(
+                    map(hoverState => !!hoverState.hoverOverlayProps),
+                    distinctUntilChanged(isEqual)
+                )
+
+                // If you need to debug this test, the following might help. Append this to the `outputDiagram`
+                // string below:
+                //
+                //   ` ${delayAfterMouseover - 1}ms c ${delayTime - 1}ms d`
+                //
+                // Also, add these properties to `outputValues`:
+                //
+                //   c: true, // the most important instant, right after the click to pin (must be true, meaning it doesn't disappear)
+                //   d: true,
+                //
+                // There should be no emissions at "c" or "d", so this will cause the test to fail. But those are
+                // the most likely instants where there would be an emission if pinning is causing a temporary
+                // disappearance of the overlay.
+                const delayAfterMouseover = 100
+                const outputDiagram = `${MOUSEOVER_DELAY}ms a ${delayTime - 1}ms b`
+                const outputValues: {
+                    [key: string]: boolean
+                } = {
+                    a: false,
+                    b: true,
+                }
+
+                // Mouseover https://sourcegraph.sgdev.org/github.com/gorilla/mux@cb4698366aa625048f3b815af6a0dea8aef9280a/-/blob/mux.go#L24:6
+                cold('a').subscribe(() =>
+                    dispatchMouseEventAtPositionImpure('mouseover', codeView, {
+                        line: 24,
+                        character: 6,
+                    })
+                )
+
+                // Click (to pin) https://sourcegraph.sgdev.org/github.com/gorilla/mux@cb4698366aa625048f3b815af6a0dea8aef9280a/-/blob/mux.go#L24:6
+                cold(`${MOUSEOVER_DELAY + delayTime + delayAfterMouseover}ms c`).subscribe(() =>
+                    dispatchMouseEventAtPositionImpure('click', codeView, {
+                        line: 24,
+                        character: 6,
+                    })
+                )
+
+                // Mouseover something else and ensure it remains pinned.
+                cold(`${MOUSEOVER_DELAY + delayTime + delayAfterMouseover + 100}ms d`).subscribe(() =>
+                    dispatchMouseEventAtPositionImpure('mouseover', codeView, {
+                        line: 25,
+                        character: 3,
+                    })
+                )
+
+                expectObservable(hoverAndDefinitionUpdates).toBe(outputDiagram, outputValues)
+            })
+        }
+    })
+
     it('emits loading and then state on click events', () => {
         for (const codeView of testcases) {
             const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b))

src/hoverifier.ts

@@ -521,7 +521,7 @@ export function createHoverifier<C extends object>({
     )
 
     /** Emits new positions including context at which a tooltip needs to be shown from clicks, mouseovers and URL changes. */
-    const resolvedPositions = merge(codeMouseOverTargets, jumpTargets, codeClickTargets).pipe(
+    const resolvedPositionEvents = merge(codeMouseOverTargets, jumpTargets, codeClickTargets).pipe(
         map(({ position, resolveContext, eventType, ...rest }) => ({
             ...rest,
             eventType,
@@ -530,6 +530,13 @@ export function createHoverifier<C extends object>({
         share()
     )
 
+    const resolvedPositions = resolvedPositionEvents.pipe(
+        // Suppress emissions from other events that refer to the same position as the current one. This makes it
+        // so the overlay doesn't temporarily disappear when, e.g., clicking to pin the overlay when it's already
+        // visible due to a mouseover.
+        distinctUntilChanged((a, b) => isEqual(a.position, b.position))
+    )
+
     /** Emits new referencesURL values. */
     subscription.add(
         resolvedPositions
@@ -713,11 +720,14 @@ export function createHoverifier<C extends object>({
     // if either the hover or the definition turn out non-empty, pin the tooltip.
     // If they both turn out empty, unpin it so we don't end up with an invisible tooltip.
     //
-    // zip together a position and the hover and definition fetches it triggered
+    // zip together the corresponding hover and definition fetches
     subscription.add(
-        zip(resolvedPositions, hoverObservables, definitionObservables)
+        combineLatest(
+            zip(hoverObservables, definitionObservables),
+            resolvedPositionEvents.pipe(map(({ eventType }) => eventType))
+        )
             .pipe(
-                switchMap(([{ eventType }, hoverObservable, definitionObservable]) => {
+                switchMap(([[hoverObservable, definitionObservable], eventType]) => {
                     // If the position was triggered by a mouseover, never pin
                     if (eventType !== 'click' && eventType !== 'jump') {
                         return [false]