From 0bd1a5232173538053eccba24198c92381989701 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guychard?= Date: Tue, 3 Dec 2019 18:42:39 -0500 Subject: [PATCH 1/5] feat: add HoverifyOptions.scrollBoundaries Rel [RFC 76](https://docs.google.com/document/d/1albi6HaAU9JAqdQXjsE-50NidxG4icE-36XqxxKgfM4/edit) Introduces `HoverifyOptions.scrollBoundaries`, an array of `HTMLElement`, which would typically contain elements with a lower z-index than the hover overlay but a higher z-index than the code view, such as a sticky file header. At runtime, in reaction to scroll events, hide the hover overlay when it overlaps with a hover boundary. `IntersectionObserver` could not be used here, as it requires an ancestor relationship between the root and target elements. --- src/helpers.ts | 14 +++++++++++++ src/hoverifier.ts | 52 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/helpers.ts b/src/helpers.ts index 0306c233..317ce306 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -31,3 +31,17 @@ export const scrollIntoCenterIfNeeded = (container: HTMLElement, content: HTMLEl container.scrollTop = scrollTop } } + +/** + * Returns a curried function that retusn `true` if `e1` and `e2` overlap. + */ +export const elementOverlaps = (e1: HTMLElement) => (e2: HTMLElement) => { + const e1Rect = e1.getBoundingClientRect() + const e2Rect = e2.getBoundingClientRect() + return !( + e1Rect.right < e2Rect.left || + e1Rect.left > e2Rect.right || + e1Rect.bottom < e2Rect.top || + e1Rect.top > e2Rect.bottom + ) +} diff --git a/src/hoverifier.ts b/src/hoverifier.ts index 4189931a..31f59e74 100644 --- a/src/hoverifier.ts +++ b/src/hoverifier.ts @@ -1,12 +1,14 @@ import { Position, Range } from '@sourcegraph/extension-api-types' import { isEqual } from 'lodash' import { + animationFrameScheduler, combineLatest, concat, EMPTY, from, fromEvent, merge, + NEVER, Observable, of, Subject, @@ -22,6 +24,7 @@ import { distinctUntilChanged, filter, map, + observeOn, share, switchMap, takeUntil, @@ -29,7 +32,7 @@ import { } from 'rxjs/operators' import { Key } from 'ts-key-enum' import { asError, ErrorLike, isErrorLike } from './errors' -import { scrollIntoCenterIfNeeded } from './helpers' +import { elementOverlaps, scrollIntoCenterIfNeeded } from './helpers' import { calculateOverlayPosition } from './overlay_position' import { DiffPart, PositionEvent, SupportedMouseEvent } from './positions' import { createObservableStateContainer } from './state' @@ -187,6 +190,15 @@ export interface HoverifyOptions extends Pick, Exclude, 'codeViewId'>> { positionEvents: Subscribable + /** + * An array of elements used to hide the hover overlay if any of them + * overlap with the hovered token. Overlapping is checked in reaction to scroll events. + * + * scrollBoundaries are typically elements with a lower z-index than the hover overlay + * but a higher z-index than the code view, such as a sticky file header. + */ + scrollBoundaries?: HTMLElement[] + /** * Emit on this Observable to trigger the overlay on a position in this code view. * This Observable is intended to be used to trigger a Hover after a URL change with a position. @@ -868,6 +880,14 @@ export function createHoverifier({ }) ) + /** + * An Observable of scroll events on the document. + */ + const scrollEvents = fromEvent(document, 'scroll').pipe( + observeOn(animationFrameScheduler), + share() + ) + return { get hoverState(): Readonly> { return internalToExternalState(container.values) @@ -876,7 +896,12 @@ export function createHoverifier({ map(internalToExternalState), distinctUntilChanged((a, b) => isEqual(a, b)) ), - hoverify({ positionEvents, positionJumps = EMPTY, ...eventOptions }: HoverifyOptions): Subscription { + hoverify({ + positionEvents, + positionJumps = EMPTY, + scrollBoundaries, + ...eventOptions + }: HoverifyOptions): Subscription { const codeViewId = Symbol('CodeView') const subscription = new Subscription() // Broadcast all events from this code view @@ -897,6 +922,29 @@ export function createHoverifier({ resetHover() } }) + if (scrollBoundaries && scrollBoundaries.length > 0) { + // When the hovered token is in this code view, listen to scroll events, + // and reset the hover if the hovered token overlaps with any of the scrollBoundaries. + subscription.add( + container.updates + .pipe( + distinctUntilChanged( + ({ hoveredTokenElement: e1 }, { hoveredTokenElement: e2 }) => e1 === e2 + ), + switchMap(({ codeViewId: id, hoveredTokenElement }) => { + if (id !== codeViewId || !hoveredTokenElement) { + return NEVER + } + return scrollEvents.pipe( + filter(() => scrollBoundaries.some(elementOverlaps(hoveredTokenElement))) + ) + }) + ) + .subscribe(() => { + resetHover() + }) + ) + } return subscription }, unsubscribe(): void { From 72c46ab41e81372e53e823af5c6f12b273e07f14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guychard?= Date: Thu, 5 Dec 2019 14:18:16 -0500 Subject: [PATCH 2/5] Update src/helpers.ts Co-Authored-By: Felix Becker --- src/helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/helpers.ts b/src/helpers.ts index 317ce306..c8dbe724 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -35,7 +35,7 @@ export const scrollIntoCenterIfNeeded = (container: HTMLElement, content: HTMLEl /** * Returns a curried function that retusn `true` if `e1` and `e2` overlap. */ -export const elementOverlaps = (e1: HTMLElement) => (e2: HTMLElement) => { +export const elementOverlaps = (e1: HTMLElement) => (e2: HTMLElement): boolean => { const e1Rect = e1.getBoundingClientRect() const e2Rect = e2.getBoundingClientRect() return !( From c753c88c47f6cb19fecce46ed51872b34b9d8034 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guychard?= Date: Fri, 6 Dec 2019 11:13:57 -0800 Subject: [PATCH 3/5] refactor: move scrollBoundaries logic into createHoverifier() --- src/hoverifier.ts | 211 +++++++++++++++++++++++++++------------------- 1 file changed, 123 insertions(+), 88 deletions(-) diff --git a/src/hoverifier.ts b/src/hoverifier.ts index 31f59e74..219e30d4 100644 --- a/src/hoverifier.ts +++ b/src/hoverifier.ts @@ -8,7 +8,6 @@ import { from, fromEvent, merge, - NEVER, Observable, of, Subject, @@ -23,7 +22,9 @@ import { delay, distinctUntilChanged, filter, + first, map, + mapTo, observeOn, share, switchMap, @@ -181,14 +182,6 @@ export interface EventOptions { adjustPosition?: PositionAdjuster dom: DOMFunctions codeViewId: symbol -} - -/** - * @template C Extra context for the hovered token. - */ -export interface HoverifyOptions - extends Pick, Exclude, 'codeViewId'>> { - positionEvents: Subscribable /** * An array of elements used to hide the hover overlay if any of them @@ -198,6 +191,14 @@ export interface HoverifyOptions * but a higher z-index than the code view, such as a sticky file header. */ scrollBoundaries?: HTMLElement[] +} + +/** + * @template C Extra context for the hovered token. + */ +export interface HoverifyOptions + extends Pick, Exclude, 'codeViewId'>> { + positionEvents: Subscribable /** * Emit on this Observable to trigger the overlay on a position in this code view. @@ -619,6 +620,84 @@ export function createHoverifier({ distinctUntilChanged((a, b) => isEqual(a.position, b.position)) ) + /** + * An Observable of scroll events on the document. + */ + const scrollEvents = fromEvent(document, 'scroll').pipe( + observeOn(animationFrameScheduler), + share() + ) + + /** + * Returns the highlighted range for the given hover result and position. + * + * Returns `undefined` if the hover result is not successful. + * + * Uses the range specified by the hover result if present, or `position` oherwise, + * which will be expanded into a full token in getTokenAtPosition(). + */ + const getHighlightedRange = ({ + hoverOrError, + position, + }: { + hoverOrError?: typeof LOADING | (HoverAttachment & D) | ErrorLike | null + position: Position | undefined + }): Range | undefined => { + if (hoverOrError && !isErrorLike(hoverOrError) && hoverOrError !== LOADING) { + if (hoverOrError.range) { + // The result is 0-indexed; the code view is treated as 1-indexed. + return { + start: { + line: hoverOrError.range.start.line + 1, + character: hoverOrError.range.start.character + 1, + }, + end: { + line: hoverOrError.range.end.line + 1, + character: hoverOrError.range.end.character + 1, + }, + } + } + if (position) { + return { start: position, end: position } + } + } + return undefined + } + + /** + * Returns an Observable that emits the hover result immediately, + * and will emit a result resetting the hover when the hoveredTokenElement intersects + * with the scrollBoundaries. + */ + const resetOnBoundaryIntersection = ({ + hoveredTokenElement, + scrollBoundaries, + ...rest + }: Omit, 'mouseIsMoving' | 'hoverOverlayIsFixed'> & + Omit, 'resolveContext' | 'dom'> & { codeView: HTMLElement }): Observable< + Omit, 'mouseIsMoving' | 'hoverOverlayIsFixed'> & { codeView: HTMLElement } + > => { + const result = of({ hoveredTokenElement, ...rest }) + if (!hoveredTokenElement || !scrollBoundaries) { + return result + } + return merge( + result, + scrollEvents.pipe( + filter(() => scrollBoundaries.some(elementOverlaps(hoveredTokenElement))), + first(), + mapTo({ + ...rest, + hoveredTokenElement, + hoverOverlayIsFixed: false, + hoverOrError: undefined, + hoveredToken: undefined, + actionsOrError: undefined, + }) + ) + ) + } + /** * For every position, emits an Observable with new values for the `hoverOrError` state. * This is a higher-order Observable (Observable that emits Observables). @@ -631,6 +710,7 @@ export function createHoverifier({ adjustPosition?: PositionAdjuster codeView: HTMLElement codeViewId: symbol + scrollBoundaries?: HTMLElement[] hoverOrError?: typeof LOADING | (HoverAttachment & D) | ErrorLike | null position?: HoveredToken & C part?: DiffPart @@ -674,7 +754,7 @@ export function createHoverifier({ hoverObservables .pipe( switchMap(hoverObservable => hoverObservable), - switchMap(({ hoverOrError, position, adjustPosition, ...rest }) => { + switchMap(({ hoverOrError, position, adjustPosition, codeView, part, ...rest }) => { let pos = hoverOrError && hoverOrError !== LOADING && @@ -685,7 +765,13 @@ export function createHoverifier({ : position if (!pos) { - return of({ hoverOrError, position: undefined as Position | undefined, ...rest }) + return of({ + hoverOrError, + codeView, + part, + position: undefined as Position | undefined, + ...rest, + }) } // The requested position is is 0-indexed; the code here is currently 1-indexed @@ -695,66 +781,51 @@ export function createHoverifier({ const adjustingPosition = adjustPosition ? from( adjustPosition({ - codeView: rest.codeView, + codeView, direction: AdjustmentDirection.ActualToCodeView, position: { ...pos, - part: rest.part, + part, }, }) ) : of(pos) - return adjustingPosition.pipe(map(position => ({ position, hoverOrError, ...rest }))) + return adjustingPosition.pipe( + map(position => ({ position, hoverOrError, codeView, part, ...rest })) + ) + }), + switchMap(({ scrollBoundaries, hoverOrError, position, codeView, codeViewId, dom, part }) => { + const highlightedRange = getHighlightedRange({ hoverOrError, position }) + const hoveredTokenElement = highlightedRange + ? getTokenAtPosition(codeView, highlightedRange.start, dom, part, tokenize) + : undefined + return resetOnBoundaryIntersection({ + scrollBoundaries, + codeViewId, + codeView, + highlightedRange, + hoverOrError, + hoveredTokenElement, + hoverOverlayPosition: undefined, + }) }) ) - .subscribe(({ hoverOrError, position, codeView, codeViewId, 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) { - // The result is 0-indexed; the code view is treated as 1-indexed. - highlightedRange = { - start: { - line: hoverOrError.range.start.line + 1, - character: hoverOrError.range.start.character + 1, - }, - end: { - line: hoverOrError.range.end.line + 1, - character: hoverOrError.range.end.character + 1, - }, - } - } else if (position) { - highlightedRange = { start: position, end: position } - } - } - + .subscribe(({ codeView, highlightedRange, hoveredTokenElement, ...rest }) => { container.update({ - codeViewId, - hoverOrError, highlightedRange, - // Reset the hover position, it's gonna be repositioned after the hover was rendered - hoverOverlayPosition: undefined, + hoveredTokenElement, + ...rest, }) - // 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 (!highlightedRange) { - container.update({ hoveredTokenElement: undefined }) - return + if (hoveredTokenElement) { + hoveredTokenElement.classList.add('selection-highlight') } - const token = getTokenAtPosition(codeView, highlightedRange.start, dom, part, tokenize) - container.update({ hoveredTokenElement: token }) - if (!token) { - return - } - token.classList.add('selection-highlight') }) ) @@ -880,14 +951,6 @@ export function createHoverifier({ }) ) - /** - * An Observable of scroll events on the document. - */ - const scrollEvents = fromEvent(document, 'scroll').pipe( - observeOn(animationFrameScheduler), - share() - ) - return { get hoverState(): Readonly> { return internalToExternalState(container.values) @@ -896,12 +959,7 @@ export function createHoverifier({ map(internalToExternalState), distinctUntilChanged((a, b) => isEqual(a, b)) ), - hoverify({ - positionEvents, - positionJumps = EMPTY, - scrollBoundaries, - ...eventOptions - }: HoverifyOptions): Subscription { + hoverify({ positionEvents, positionJumps = EMPTY, ...eventOptions }: HoverifyOptions): Subscription { const codeViewId = Symbol('CodeView') const subscription = new Subscription() // Broadcast all events from this code view @@ -922,29 +980,6 @@ export function createHoverifier({ resetHover() } }) - if (scrollBoundaries && scrollBoundaries.length > 0) { - // When the hovered token is in this code view, listen to scroll events, - // and reset the hover if the hovered token overlaps with any of the scrollBoundaries. - subscription.add( - container.updates - .pipe( - distinctUntilChanged( - ({ hoveredTokenElement: e1 }, { hoveredTokenElement: e2 }) => e1 === e2 - ), - switchMap(({ codeViewId: id, hoveredTokenElement }) => { - if (id !== codeViewId || !hoveredTokenElement) { - return NEVER - } - return scrollEvents.pipe( - filter(() => scrollBoundaries.some(elementOverlaps(hoveredTokenElement))) - ) - }) - ) - .subscribe(() => { - resetHover() - }) - ) - } return subscription }, unsubscribe(): void { From 7c25343f6a0cebbb68eac109f58982ec28fb07da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guychard?= Date: Fri, 6 Dec 2019 11:16:45 -0800 Subject: [PATCH 4/5] Update src/helpers.ts --- src/helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/helpers.ts b/src/helpers.ts index c8dbe724..3cb5cabd 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -33,7 +33,7 @@ export const scrollIntoCenterIfNeeded = (container: HTMLElement, content: HTMLEl } /** - * Returns a curried function that retusn `true` if `e1` and `e2` overlap. + * Returns a curried function that returns `true` if `e1` and `e2` overlap. */ export const elementOverlaps = (e1: HTMLElement) => (e2: HTMLElement): boolean => { const e1Rect = e1.getBoundingClientRect() From 05adfe2e5b2f8e3fa8defea0f3318ad9414ccf9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guychard?= Date: Tue, 10 Dec 2019 09:29:35 -0800 Subject: [PATCH 5/5] chore: add karma unit test --- src/hoverifier.test.ts | 60 +++++++++++++++++++++++++++++++++++-- testdata/github/generate.ts | 1 + testdata/github/styles.css | 15 ++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/src/hoverifier.test.ts b/src/hoverifier.test.ts index dca367ef..5b27587f 100644 --- a/src/hoverifier.test.ts +++ b/src/hoverifier.test.ts @@ -1,10 +1,10 @@ import { Range } from '@sourcegraph/extension-api-types' import { isEqual } from 'lodash' import { EMPTY, NEVER, Observable, of, Subject, Subscription } from 'rxjs' -import { delay, distinctUntilChanged, filter, first, map } from 'rxjs/operators' +import { delay, distinctUntilChanged, filter, first, map, takeWhile } from 'rxjs/operators' import { TestScheduler } from 'rxjs/testing' import { ErrorLike } from './errors' -import { propertyIsDefined } from './helpers' +import { isDefined, propertyIsDefined } from './helpers' import { AdjustmentDirection, createHoverifier, @@ -30,6 +30,13 @@ describe('Hoverifier', () => { testcases = dom.createCodeViews() }) + let subscriptions = new Subscription() + + afterEach(() => { + subscriptions.unsubscribe() + subscriptions = new Subscription() + }) + 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)) @@ -322,6 +329,55 @@ describe('Hoverifier', () => { } }) + it('hides the hover overlay when the hovered token intersects with a scrollBoundary', async () => { + const gitHubCodeView = testcases[1] + const hoverifier = createHoverifier({ + closeButtonClicks: NEVER, + hoverOverlayElements: of(null), + hoverOverlayRerenders: EMPTY, + getHover: createStubHoverProvider({ + range: { + start: { line: 4, character: 9 }, + end: { line: 4, character: 9 }, + }, + }), + getActions: createStubActionsProvider(['foo', 'bar']), + pinningEnabled: true, + }) + subscriptions.add(hoverifier) + subscriptions.add( + hoverifier.hoverify({ + dom: gitHubCodeView, + positionEvents: of(gitHubCodeView.codeView).pipe( + findPositionsFromEvents({ domFunctions: gitHubCodeView }) + ), + positionJumps: new Subject(), + resolveContext: () => gitHubCodeView.revSpec, + scrollBoundaries: [gitHubCodeView.codeView.querySelector('.sticky-file-header')!], + }) + ) + + gitHubCodeView.codeView.scrollIntoView() + + // Click https://sourcegraph.sgdev.org/github.com/gorilla/mux@cb4698366aa625048f3b815af6a0dea8aef9280a/-/blob/mux.go#L5:9 + // and wait for the hovered token to be defined. + const hasHoveredToken = hoverifier.hoverStateUpdates + .pipe(takeWhile(({ hoveredTokenElement }) => !isDefined(hoveredTokenElement))) + .toPromise() + dispatchMouseEventAtPositionImpure('click', gitHubCodeView, { + line: 5, + character: 9, + }) + await hasHoveredToken + + // Scroll down: the hover overlay should get hidden. + const hoverIsHidden = hoverifier.hoverStateUpdates + .pipe(takeWhile(({ hoverOverlayProps }) => isDefined(hoverOverlayProps))) + .toPromise() + gitHubCodeView.getCodeElementFromLineNumber(gitHubCodeView.codeView, 2)!.scrollIntoView({ behavior: 'smooth' }) + await hoverIsHidden + }) + describe('pinning', () => { it('unpins upon clicking on a different position', () => { for (const codeView of testcases) { diff --git a/testdata/github/generate.ts b/testdata/github/generate.ts index 5731c8de..51cb13d6 100644 --- a/testdata/github/generate.ts +++ b/testdata/github/generate.ts @@ -19,6 +19,7 @@ export function generateGithubCodeTable(lines: string[]): string {
+
${code}
diff --git a/testdata/github/styles.css b/testdata/github/styles.css index 952e8f87..e89614da 100644 --- a/testdata/github/styles.css +++ b/testdata/github/styles.css @@ -10,6 +10,21 @@ color: #24292e; } +.github-testcase .sticky-file-header { + position: sticky; + z-index: 6; + top: 0px; +} + +.github-testcase .file-header { + height: 60px; + padding: 5px 10px; + background-color: #fafbfc; + border-bottom: 1px solid #e1e4e8; + border-top-left-radius: 2px; + border-top-right-radius: 2px; +} + .github-testcase .container { width: 980px; margin-right: auto;