Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

blob: Fix text selection visibility over selected lines and line number alignment #59712

Merged
merged 2 commits into from
Jan 19, 2024
Merged

blob: Fix text selection visibility over selected lines and line number alignment #59712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c897006
blob: Fix text selection visibility over selected lines
fkling Jan 18, 2024
1daff97
Update e2e test selectors
fkling Jan 19, 2024
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
2 changes: 1 addition & 1 deletion client/web/src/end-to-end/code-intel/repository-component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ describe('Repository component', () => {
await link.click()

const selectedLine = await driver.page.waitForSelector(
`[data-testid="repo-blob"] .cm-line:nth-child(${line}).selected-line`
`[data-testid="repo-blob"] .cm-line:nth-child(${line})[data-testid="selected-line"]`
)
expect(selectedLine).not.toBeNull()
})
Expand Down
10 changes: 9 additions & 1 deletion client/web/src/repo/blob/codemirror/blame-decorations.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,6 @@ const blameDecorationTheme = EditorView.theme({
left: '0',
height: '100%',
display: 'inline-block',
background: 'var(--body-bg)',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This didn't seem to be necessary and removing it actually lets the selected line color "come through".

verticalAlign: 'bottom',
width: 'var(--blame-decoration-width)',

Expand All @@ -157,6 +156,15 @@ const blameDecorationTheme = EditorView.theme({
'.no-line-break-msg': {
marginLeft: 'var(--blame-decoration-width)',
},

// This ensures that line numbers and folding controls are reasonably aligned
// with the top of the line, even if line wrapping is enabled.
'.cm-lineNumbers .cm-gutterElement': {
lineHeight: '1.5rem',
},
'.cm-foldGutter .cm-gutterElement': {
lineHeight: '1.5rem',
},
})

class BlameDecorationViewPlugin implements PluginValue {
Expand Down
135 changes: 67 additions & 68 deletions client/web/src/repo/blob/codemirror/linenumbers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import {
Annotation,
EditorSelection,
type Extension,
type Range,
RangeSet,
Expand All @@ -21,51 +20,8 @@ import {
type ViewUpdate,
} from '@codemirror/view'

import { isFirefox } from '@sourcegraph/common'

import { isValidLineRange, MOUSE_MAIN_BUTTON } from './utils'

const selectedLinesTheme = EditorView.theme({
/**
* [RectangleMarker.forRange](https://sourcegraph.com/github.com/codemirror/view@a0a0b9ef5a4deaf58842422ac080030042d83065/-/blob/src/layer.ts?L60-75)
* returns absolutely positioned markers. Markers top position has extra 1px (5px in case blame decorations
* are visible) more in its `top` value breaking alignment wih the line.
* We compensate this spacing by setting negative margin-top.
*
* Line highlighting breaks (highlights two lines instead of 1) in firefox if minHeight is set, so we
* add a conditional to check for the current browser, and set the css accordingly.
*
* todo(fkling): Revisit this, styling is not correct for empty lines
*/
'.selected-lines-layer .selected-line': isFirefox()
? {
marginTop: '-1px',
}
: {
marginTop: '-1px',
// Ensure selection marker height matches line height.
minHeight: '1.0rem',
},

'.selected-lines-layer .selected-line.blame-visible': {
marginTop: '-5px',

// Ensure selection marker height matches the increased line height.
minHeight: 'calc(1.5rem + 1px)',
},

/**
* Rectangle markers `left` position matches the position of the character at the start of range
* (for selected lines it is first character of the first line in a range). When line content (`.cm-line`)
* has some padding to the left (e.g. to create extra space between gutters and code) there is a gap in
* highlight (background color) between the selected line gutters (decorated with {@link selectedLineGutterMarker}) and layer.
* To remove this gap we move padding from `.cm-line` to the last gutter.
*/
'.cm-gutter:last-child .cm-gutterElement': {
paddingRight: '0.2rem',
},
})

/**
* Represents the currently selected line range. null means no lines are
* selected. Line numbers are 1-based.
Expand All @@ -74,7 +30,6 @@ const selectedLinesTheme = EditorView.theme({
export type SelectedLineRange = { line: number; character?: number; endLine?: number } | null

const selectedLineDecoration = Decoration.line({
class: 'selected-line',
attributes: {
tabIndex: '-1',
'data-line-focusable': '',
Expand Down Expand Up @@ -135,12 +90,12 @@ export const selectedLines = StateField.define<SelectedLineRange>({

/**
* We highlight selected lines using layer instead of line decorations.
* With this approach both selected lines and editor selection layers may be visible (with the latter taking precedence).
* It makes selected text highlighted even if it is on a selected line.
* With this approach both selected lines and editor selection layers are be visible (with the latter taking precedence).
*
* We can't use line decorations for this because the editor selection layer is positioned behind the document content
* With line decorations the editor selection layer is positioned behind the document content
* and thus the line background set by line decorations overrides the layer background making selected text
* not highlighted.
* not highlighted. An alternative would be to use reduce the opacity of the line decorations but this would
* change the text selection color.
*/
layer({
above: false,
Expand All @@ -150,17 +105,70 @@ export const selectedLines = StateField.define<SelectedLineRange>({
return []
}

const endLineNumber = range.endLine ?? range.line
const startLine = view.state.doc.line(Math.min(range.line, endLineNumber))
const endLine = view.state.doc.line(
Math.min(view.state.doc.lines, startLine.number === endLineNumber ? range.line : endLineNumber)
)
// We can't use RectangleMarker.fromRange because this positions the marker exactly at the start/top of
// the actual text in the line, not at start/top what we consider to be the line. This is especially
// apparent when the blame column is visible because the line hight is increased to give the blame
// information more space. The following box illustrates the problem:
//
// ┌───┬────────────────────────────────────────────────┐ ──┐
// │ │ ──┐ │ │
// │ 1 │ Some text here ├── .fromRange gives us this │ ├─── we want this
// │ │ ──┘ │ │
// └───┴────────────────────────────────────────────────┘ ──┘
//
// The left and top positions, and width and height of the rectangle marker that corresponds to the
// selected line are computed from these values:
//
// rectangle.left
// ◄─► rectangle.width
// ◄──────────────────────►
// ▲ ┌─────────────────────────┐ ▲
// │ │ │ │documentPadding.top
// │ ├─┬───────────────────────┤ ▲ ▲ ▼
// rectangle.top│ │1│First line │ │ │
// │ ├─┼───────────────────────┤ │ │topLineBlock.top
// │ │2│Second line │ │ │
// ▲▼ ├─┼───────────────────────┤ │ ▼
// rectangle.height│ │3│███████████████████████│ │topLineBlock.bottom
// ▼ ├─┼───────────────────────┤ ▼
// │4│Fourth line │
// ├─┼───────────────────────┤
// │.│... │
// └─┴───────────────────────┘
// ◄───────────────────────►
// ▲ ▲ contentRect.width
// │ │
// │ │
// │ contentRect.left
// │
// viewRect.left

const viewRect = view.dom.getBoundingClientRect()
const contentRect = view.contentDOM.getBoundingClientRect()

const topLine = view.state.doc.line(range.endLine ? Math.min(range.line, range.endLine) : range.line)
const topLineBlock = view.lineBlockAt(topLine.from)

// Markers are positioned relative to the view DOM element, i.e. position 0 would be left of the gutter.
// This computes the left position of the content element relative to the view DOM element.
const left = contentRect.left - viewRect.left

// block.top is relative to the document top, which is the top of the first line, _not_ including the
// content element's padding. So we have to add the padding to properly align the marker with the top
// of the line.
const top = topLineBlock.top + view.documentPadding.top
const width = contentRect.width
let height = topLineBlock.bottom - topLineBlock.top

if (range.endLine !== undefined) {
const bottomLine = view.state.doc.line(
Math.min(view.state.doc.lines, Math.max(range.line, range.endLine))
)
const bottomLineBlock = view.lineBlockAt(bottomLine.from)
height = bottomLineBlock.bottom - topLineBlock.top
}

return RectangleMarker.forRange(
view,
'selected-line',
EditorSelection.range(startLine.from, Math.min(endLine.to + 1, view.state.doc.length))
)
return [new RectangleMarker('selected-line', left, top, width, height)]
},
update(update) {
return (
Expand All @@ -175,8 +183,6 @@ export const selectedLines = StateField.define<SelectedLineRange>({
class: 'selected-lines-layer',
}),

selectedLinesTheme,

gutterLineClass.compute([field], state => {
const range = state.field(field)
const marks: Range<GutterMarker>[] = []
Expand Down Expand Up @@ -261,13 +267,6 @@ const selectedLineNumberTheme = EditorView.theme({
cursor: 'pointer',
color: 'var(--line-number-color)',

'& .cm-gutterElement': {
display: 'flex',
flexDirection: 'column',
alignItems: 'flex-end',
justifyContent: 'flex-end',
},

'& .cm-gutterElement:hover': {
textDecoration: 'underline',
},
Expand Down