fix: delay highlighting the hovered token until the hover is shown

Fixes an issue where the token would be highlighted before the hover was shown (if the hover was taking a while to load). The reason behind not showing the hover loading immediately is to avoid UI jitter. In keeping with this principle, it makes sense to not show any changes in the UI (including highlighting the hovered range) until we know they will not need to be immediately redrawn. If the token is highlighted and then a few hundred msec later the hover is determined to be empty, the highlight will go away. That looks jittery, and that problem is fixed by this commit.

Author: sqs

src/helpers.ts

@@ -21,7 +21,7 @@ export const propertyIsDefined = <T extends object, K extends keyof T>(key: K) =
 ): val is K extends any ? ({ [k in Exclude<keyof T, K>]: T[k] } & { [k in K]: NonNullable<T[k]> }) : never =>
     isDefined(val[key])
 
-export const isEmptyHover = (hover: HoverMerged | null): boolean =>
+const isEmptyHover = (hover: HoverMerged | null): boolean =>
     !hover ||
     !hover.contents ||
     (Array.isArray(hover.contents) && hover.contents.length === 0) ||

src/hoverifier.test.ts

@@ -1,8 +1,8 @@
 import { isEqual } from 'lodash'
-import { EMPTY, Observable, of, Subject, Subscription } from 'rxjs'
+import { EMPTY, NEVER, Observable, of, Subject, Subscription } from 'rxjs'
 import { distinctUntilChanged, filter, map } from 'rxjs/operators'
 import { TestScheduler } from 'rxjs/testing'
-import { Position } from 'vscode-languageserver-types'
+import { Position, Range } from 'vscode-languageserver-types'
 
 import { noop } from 'lodash'
 import { propertyIsDefined } from './helpers'
@@ -30,6 +30,74 @@ describe('Hoverifier', () => {
         testcases = dom.createCodeViews()
     })
 
+    it('highlights token when hover is fetched (not before)', () => {
+        for (const codeView of testcases) {
+            const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b))
+
+            const delayTime = 100
+            const hoverRange = { start: { line: 1, character: 2 }, end: { line: 3, character: 4 } }
+
+            scheduler.run(({ cold, expectObservable }) => {
+                const hoverifier = createHoverifier({
+                    closeButtonClicks: NEVER,
+                    goToDefinitionClicks: NEVER,
+                    hoverOverlayElements: of(null),
+                    hoverOverlayRerenders: EMPTY,
+                    fetchHover: createStubHoverFetcher({ range: hoverRange }, LOADER_DELAY + delayTime),
+                    fetchJumpURL: () => of(null),
+                    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 highlightedRangeUpdates = hoverifier.hoverStateUpdates.pipe(
+                    map(hoverOverlayProps => (hoverOverlayProps ? hoverOverlayProps.highlightedRange : null)),
+                    distinctUntilChanged((a, b) => isEqual(a, b))
+                )
+
+                const inputDiagram = 'a'
+
+                const outputDiagram = `${MOUSEOVER_DELAY}ms a ${LOADER_DELAY + delayTime - 1}ms b`
+
+                const outputValues: {
+                    [key: string]: Range | undefined
+                } = {
+                    a: undefined, // highlightedRange is undefined when the hover is loading
+                    b: hoverRange,
+                }
+
+                // Hover over https://sourcegraph.sgdev.org/github.com/gorilla/mux@cb4698366aa625048f3b815af6a0dea8aef9280a/-/blob/mux.go#L24:6
+                cold(inputDiagram).subscribe(() =>
+                    dispatchMouseEventAtPositionImpure('mouseover', codeView, {
+                        line: 24,
+                        character: 6,
+                    })
+                )
+
+                expectObservable(highlightedRangeUpdates).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

@@ -27,10 +27,10 @@ import {
     withLatestFrom,
 } from 'rxjs/operators'
 import { Key } from 'ts-key-enum'
-import { Position } from 'vscode-languageserver-types'
-import { asError, ErrorLike } from './errors'
+import { Position, Range } from 'vscode-languageserver-types'
+import { asError, ErrorLike, isErrorLike } from './errors'
 import { isDefined } from './helpers'
-import { isEmptyHover, overlayUIHasContent, scrollIntoCenterIfNeeded } from './helpers'
+import { overlayUIHasContent, scrollIntoCenterIfNeeded } from './helpers'
 import { HoverOverlayProps, isJumpURL } from './HoverOverlay'
 import { calculateOverlayPosition } from './overlay_position'
 import { DiffPart, PositionEvent, SupportedMouseEvent } from './positions'
@@ -193,6 +193,11 @@ export interface HoverState {
      */
     hoverOverlayProps?: Pick<HoverOverlayProps, Exclude<keyof HoverOverlayProps, 'linkComponent'>>
 
+    /**
+     * The highlighted range, which is the range in the hover result or else the range of the hovered token.
+     */
+    highlightedRange?: Range
+
     /**
      * The currently selected position, if any.
      * Can be a single line number or a line range.
@@ -223,6 +228,11 @@ interface InternalHoverifierState<C extends object> {
     /** The currently hovered token */
     hoveredToken?: HoveredToken & C
 
+    /**
+     * The highlighted range, which is the range in the hoverOrError data or else the range of the hovered token.
+     */
+    highlightedRange?: Range
+
     mouseIsMoving: boolean
 
     /**
@@ -244,6 +254,7 @@ const shouldRenderOverlay = (state: InternalHoverifierState<{}>): boolean =>
  */
 const internalToExternalState = (internalState: InternalHoverifierState<{}>): HoverState => ({
     selectedPosition: internalState.selectedPosition,
+    highlightedRange: shouldRenderOverlay(internalState) ? internalState.highlightedRange : undefined,
     hoverOverlayProps: shouldRenderOverlay(internalState)
         ? {
               overlayPosition: internalState.hoverOverlayPosition,
@@ -537,7 +548,7 @@ export function createHoverifier<C extends object>({
             target: HTMLElement
             adjustPosition?: PositionAdjuster<C>
             codeView: HTMLElement
-            hoverOrError?: typeof LOADING | HoverMerged | Error | null
+            hoverOrError?: typeof LOADING | HoverMerged | ErrorLike | null
             position?: HoveredToken & C
             part?: DiffPart
         }>
@@ -558,7 +569,7 @@ export function createHoverifier<C extends object>({
                             ? hoverMergedOrNull
                             : new Error(`Invalid hover response: ${JSON.stringify(hoverMergedOrNull)}`)
                 ),
-                catchError(error => [asError(error)]),
+                catchError((error): [ErrorLike] => [asError(error)]),
                 share()
             )
             // 1. Reset the hover content, so no old hover content is displayed at the new position while fetching
@@ -618,20 +629,35 @@ export function createHoverifier<C extends object>({
                 })
             )
             .subscribe(({ hoverOrError, position, codeView, dom, part }) => {
+                // Update the highlighted token if the hover result is successful. If the hover result specifies a
+                // range, use that; otherwise use the hover position (which will be expanded into a full token in
+                // getTokenAtPosition).
+                let highlightedRange: Range | undefined
+                if (hoverOrError && !isErrorLike(hoverOrError) && hoverOrError !== LOADING) {
+                    if (hoverOrError.range) {
+                        highlightedRange = hoverOrError.range
+                    } else if (position) {
+                        highlightedRange = { start: position, end: position }
+                    }
+                }
+
                 container.update({
                     hoverOrError,
+                    highlightedRange,
                     // Reset the hover position, it's gonna be repositioned after the hover was rendered
                     hoverOverlayPosition: undefined,
                 })
+
+                // Ensure the previously highlighted range is not highlighted and the new highlightedRange (if any)
+                // is highlighted.
                 const currentHighlighted = codeView.querySelector('.selection-highlight')
                 if (currentHighlighted) {
                     currentHighlighted.classList.remove('selection-highlight')
                 }
-                if (!position || !hoverOrError || (HoverMerged.is(hoverOrError) && isEmptyHover(hoverOrError))) {
+                if (!highlightedRange) {
                     return
                 }
-
-                const token = getTokenAtPosition(codeView, position, dom, part)
+                const token = getTokenAtPosition(codeView, highlightedRange.start, dom, part)
                 if (!token) {
                     return
                 }