Skip to content

Initial support for using range.getClientRects to measure text.#5922

Open
nightwing wants to merge 27 commits into
masterfrom
varchar3
Open

Initial support for using range.getClientRects to measure text.#5922
nightwing wants to merge 27 commits into
masterfrom
varchar3

Conversation

Comment thread src/layer/font_metrics.js Outdated
Comment thread src/layer/font_metrics.js Outdated
Comment thread src/virtual_renderer.js
Comment thread src/layer/font_metrics.js Outdated
Comment thread src/layer/font_metrics.js Outdated
Comment thread src/layer/font_metrics.js Outdated
Comment thread src/layer/font_metrics.js
Comment thread src/layer/font_metrics.js Outdated
Comment thread src/layer/font_metrics.js Outdated
Comment thread src/layer/font_metrics.js Outdated
Comment thread src/layer/font_metrics.js Outdated

@xyos xyos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@codecov

codecov Bot commented May 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.82093% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.92%. Comparing base (f82010e) to head (8105fe2).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/layer/font_metrics.js 93.18% 36 Missing ⚠️
src/virtual_renderer.js 88.37% 10 Missing ⚠️
src/ext/diff/inline_diff_view.js 77.77% 2 Missing ⚠️
src/layer/marker.js 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5922      +/-   ##
==========================================
+ Coverage   93.73%   93.92%   +0.19%     
==========================================
  Files         641      647       +6     
  Lines      138167   140266    +2099     
  Branches    14561    14702     +141     
==========================================
+ Hits       129510   131748    +2238     
+ Misses       8657     8518     -139     
Flag Coverage Δ
unittests 93.92% <93.82%> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@github-actions

github-actions Bot commented May 3, 2026

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

1 similar comment
@github-actions

github-actions Bot commented May 4, 2026

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

nightwing and others added 4 commits July 8, 2026 18:03
…ed text-to-pixel accuracy and update related cursor positioning logic

fix: remove unused `offsetX` parameter from `screenToDocumentPosition` and related methods

fix: set $fontMetrics to session instead of adding new selection methods

fix: refine cursor positioning using pixel-based calculations

# Conflicts:
#	src/layer/font_metrics.js
#	src/selection.js
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@github-actions

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@github-actions

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@github-actions

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@github-actions

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@github-actions

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@github-actions

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@github-actions

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

@xyos xyos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work, this is awesome!

I built #5988 on an earlier snapshot of this branch, so below are a few things I hit while testing grapheme/bidi edge cases against real browser measurements. Most are small and cherry-pickable from #5988. Details inline.

Once this merges I'd like us to review and maybe do a follow-up PR picking up the rest of #5988: the grapheme-cluster model (Intl.Segmenter-based helpers in lib/lang, cluster-snapping moveCursorTo, cluster-aware $getDisplayTokens/$computeWrapSplits, $alignTokensToClusters). It touches the same code this PR rewrites, so rebasing it on top of merged master is cleaner than growing this PR's scope. With the current branch a wrap limit crossing 👨‍👩‍👧‍👦 still splits its 11 code units across screen rows, and moveCursorTo only snaps surrogate pairs, not ZWJ/combining-mark clusters; the follow-up addresses #460, #4142, #5431, #4602, #3753, #3866, #3617.

Comment thread src/layer/text.js Outdated
$renderToken(parent, screenColumn, token, value) {
var self = this;
var re = /(\t)|( +)|([\x00-\x1f\x80-\xa0\xad\u1680\u180E\u2000-\u200f\u2028\u2029\u202F\u205F\uFEFF\uFFF9-\uFFFC\u2066\u2067\u2068\u202A\u202B\u202D\u202E\u202C\u2069\u2060\u2061\u2062\u2063\u2064\u206A\u206B\u206B\u206C\u206D\u206E\u206F]+)|(\u3000)|([\u1100-\u115F\u11A3-\u11A7\u11FA-\u11FF\u2329-\u232A\u2E80-\u2E99\u2E9B-\u2EF3\u2F00-\u2FD5\u2FF0-\u2FFB\u3001-\u303E\u3041-\u3096\u3099-\u30FF\u3105-\u312D\u3131-\u318E\u3190-\u31BA\u31C0-\u31E3\u31F0-\u321E\u3220-\u3247\u3250-\u32FE\u3300-\u4DBF\u4E00-\uA48C\uA490-\uA4C6\uA960-\uA97C\uAC00-\uD7A3\uD7B0-\uD7C6\uD7CB-\uD7FB\uF900-\uFAFF\uFE10-\uFE19\uFE30-\uFE52\uFE54-\uFE66\uFE68-\uFE6B\uFF01-\uFF60\uFFE0-\uFFE6]|[\uD800-\uDBFF][\uDC00-\uDFFF])/g;
var re = /(\t)|( +)|([\x00-\x1f\x80-\xa0\xad\u1680\u180E\u2000-\u200f\u2028\u2029\u202F\u205F\uFEFF\uFFF9-\uFFFC\u2066\u2067\u2068\u202A\u202B\u202D\u202E\u202C\u2069\u2060\u2061\u2062\u2063\u2064\u206A\u206B\u206B\u206C\u206D\u206E\u206F]+)|(\u3000+)/g;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This control-character class still includes U+200D (ZWJ) via the \u2000-\u200f range, so joined emoji like 👨‍👩‍👧‍👦 render chopped up by invalid-char dots. Splitting the range to \u2000-\u200C\u200E\u200f keeps ZWJ sequences intact and fixes #5813. (This is the one-character version of what I did in #5988.)

Comment thread src/layer/font_metrics.js Outdated
var rects = getRects(child);
for (var j = 0; j < rects.length; j++) {
let rect = rects[j];
if (rect.left < x && x < rect.left + rect.width) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Strict inequalities here mean a click landing exactly on the seam between two spans (e.g. with showInvisibles on) matches neither child rect, falls through, and accumulates the wrong column. rect.left <= x && x <= rect.left + rect.width fixes it. Also worth clamping to column 0 when x is left of the line start; right now that path can return a large column.

Comment thread src/layer/font_metrics.js
maxDistance = d;
}
}
if (blockCursor) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if (blockCursor) { // TODO } is a no-op, so vim block-cursor mode gets round-to-nearest instead of floor semantics. Either implement or drop the branch and note the limitation.

Comment thread src/layer/cursor.js

var cursorTop = (pos.row - (onScreen ? this.config.firstRowScreen : 0)) *
this.config.lineHeight;
var cursorWidth = this.config.fontMetrics.textWidth(pos.row, pos.column + 1) - textWidth;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If textWidth(col+1) and textWidth(col) measure equal (zero-width cluster tail, or a measurement fallback), cursorWidth is 0 and the cursor becomes invisible. Suggest (... - textWidth) || this.config.characterWidth.

Comment thread src/bidihandler.js
this.updateBidiMap();
}
return this.bidiMap.bidiLevels;
return true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isBidiRow now returns true for every row once the document contains any bidi character. One Hebrew word in a 10k-line file routes every marker on every line through drawBidiSingleLineMarkergetRects → DOM range measurement. A cheap per-line bidiRE.test(line) here would keep the fast path for the 99% of lines that aren't bidi.

Comment thread src/selection.js
if (useFontMetrics && this.$desiredColumn == null) {
offsetX = fontMetrics.textWidth(screenPos.row, screenPos.column);
if (isFinite(offsetX))
this.$desiredColumn = offsetX / fontMetrics.config.characterWidth;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$desiredColumn now stores a fractional pixel-derived value (offsetX / characterWidth). When the target row isn't rendered, $pixelToColumn returns it unchanged and a non-integer column flows into screenToDocumentPosition. It works in the cases I tested, but rounding the fallback would keep the column contract integer-valued.

}

var rowCount = 10;
var rowCount = 5.5;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rowCount = 5.5 looks like a leftover from testing fractional row heights; intentional?

Comment thread src/layer/font_metrics.js Outdated
}

/**
* Calculates the width of the text up to a specific scrrenColumn on a given screen row.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: typo, scrrenColumn (also ends up in the generated types/ace-modules.d.ts)

var m0 = 0.7, m1 = 0.1, m2 = 0.3, m3 = 0.82;
var t1 = 100, t2 = 20;
function testTransform() {
fontMetrics.config.$transformData = null; //FIXME

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: remove this comment

Comment thread types/ace-modules.d.ts
* @related EditSession.documentToScreenPosition
**/
screenToDocumentPosition(screenRow: number, screenColumn: number, offsetX?: number): Point;
screenToDocumentPosition(screenRow: number, screenColumn: number): Point;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a public API break: screenToDocumentPosition loses its documented offsetX param, and BidiHandler loses getPosLeft/getSelections/offsetToCol/updateCharacterWidths. Embedders calling these will break at runtime, not just in types. Probably fine given the old bidi pixel model is gone, but worth a changelog note and a decision on whether this ships as a minor or major bump.

@github-actions

Copy link
Copy Markdown

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants