Skip to content
This repository was archived by the owner on Nov 25, 2021. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion src/hoverifier.ts
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,50 @@ export const createHoverifier = ({
.pipe(
// with the latest target that came from either a mouseover, click or location change (whatever was the most recent)
withLatestFrom(merge(codeMouseOverTargets, codeClickTargets, jumpTargets)),
map(([{ hoverOverlayElement, relativeElement }, { target }]) =>
map(
([
{ hoverOverlayElement, relativeElement },
{ target, position, codeView, dom, adjustPosition, resolveContext },
]) => ({
hoverOverlayElement,
relativeElement,
target,
position,
codeView,
dom,
adjustPosition,
resolveContext,
})
),
switchMap(({ position, codeView, adjustPosition, resolveContext, ...rest }) => {
if (!position || !adjustPosition) {
return of({ position, codeView, ...rest })
}

return adjustPosition({
position: { ...position, ...resolveContext(position) },
codeView,
direction: AdjustmentDirection.ActualToCodeView,
}).pipe(
map(({ line, character }) => ({
position: { ...position, line, character },
codeView,
...rest,
}))
)
}),
map(({ target, position, codeView, dom, ...rest }) => {
// We should ensure we have the correct dom element to place the overlay above.
// It is possible that tokens span multiple elements meaning that its possible for the
// hover overlay to be placed in the middle of a token.
const token = position ? getTokenAtPosition(codeView, position, dom, position.part) : target

return {
target: token || target,
...rest,
}
}),
map(({ hoverOverlayElement, relativeElement, target }) =>
calculateOverlayPosition({ relativeElement, target, hoverOverlayElement })
)
)
Expand Down
20 changes: 17 additions & 3 deletions src/token_position.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,13 @@ describe('token_positions', () => {
})

it('gets the full token, even when it crosses multiple elements', () => {
const codeView = dom.createElementFromString('<div>To<span>ken</span></div>')
const codeView = dom.createElementFromString('<div> To<span>ken </span></div>')

const positions = [
// Test walking to the right
{ line: 1, character: 1 },
{ line: 1, character: 2 },
// Test walking to the left
{ line: 1, character: 3 },
{ line: 1, character: 4 },
]

for (const position of positions) {
Expand All @@ -177,6 +177,20 @@ describe('token_positions', () => {
chai.expect(token!.textContent).to.equal('Token')
}
})

it("doesn't wrap tokens that span multiple elements more than once", () => {
const codeView = dom.createElementFromString('<div> To<span>ken </span></div>')

const domFn = {
getCodeElementFromLineNumber: (code: HTMLElement) => code.children.item(0) as HTMLElement,
}
const position = { line: 1, character: 2 }

const token1 = getTokenAtPosition(codeView, position, domFn)
const token2 = getTokenAtPosition(codeView, position, domFn)

chai.expect(token1).to.equal(token2, 'getTokenAtPosition is wrapping tokens more than once')
})
})

describe('locateTarget()', () => {
Expand Down
22 changes: 20 additions & 2 deletions src/token_position.ts
Original file line number Diff line number Diff line change
Expand Up @@ -310,8 +310,26 @@ export function findElementWithOffset(codeElement: HTMLElement, offset: number):
tokenRange.setStartBefore(startNode)
tokenRange.setEndAfter(endNode)

// Return the common ancestor as the full token.
return tokenRange.commonAncestorContainer as HTMLElement
// If the text nodes are the same, its safe to return the common ancester which is the container element.
if (startNode === endNode || (tokenRange.commonAncestorContainer as HTMLElement).classList.contains('wrapped')) {
return tokenRange.commonAncestorContainer as HTMLElement
}

// Otherwise, we can't guarantee that the common ancester container doesn't contain
// whitespace or other characters around it. To solve for this case, we'll just
// surround the contents of the range with a new span.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the range span multiple lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a case where a token would span multiple lines. Can you?

const wrapper = document.createElement('span')
wrapper.classList.add('wrapped')

// NOTE: We can't use tokenRange.surroundContents(wrapper) because(from https://developer.mozilla.org/en-US/docs/Web/API/Range/surroundContents):
//
// An exception will be thrown, however, if the Range splits a non-Text node with only one of its
// boundary points. That is, unlike the alternative above, if there are partially selected nodes,
// they will not be cloned and instead the operation will fail.
wrapper.appendChild(tokenRange.extractContents())
tokenRange.insertNode(wrapper)

return wrapper
}

/**
Expand Down