Repository navigation
fix(web): read a sentence whole across its inline links and emphasis - #926
Merged
Merged
Conversation
Contributor
🟢 e2e web agent: 33 passed34 agent steps · 26 replayed from cache · 21 model calls · 83.4k tokens (90% cached) · $0.0041 All 33 tests in 5 files
e2e 0.18.0 · 2m 56s · web · run artifacts |
Contributor
🟢 e2e web: 91 passedAll 91 tests in 41 files
e2e 0.18.0 · 2m 5s · web · run artifacts |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Turn on auto-fix | Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
okwasniewski
force-pushed
the
oskar/inline-link-text
branch
from
October 6, 2026 14:04
8adc594 to
c589b4e
Compare
okwasniewski
commented
Oct 6, 2026
okwasniewski
left a comment
okwasniewski
left a comment
Member
Author
There was a problem hiding this comment.
On c589b4e: one question about inline clickables that have no role (inline comment on read-semantics.ts:1419).
With directTextOf(el) !== '' && !isReadInLine(el), an inline element whose only reason to be listed was its own text now drops out once its parent reads the line. That covers <span onclick>, <a> without href and <span tabindex=0>, since nothing in read-semantics detects clickability. Is it intended that the agent loses the ref for something like <p>Email: a@b.com <span onclick=...>change</span></p>? Tapping the <p> hits its center and can miss the span, and cached steps that targeted such a span would no longer find it.
Small note: REPLAY_POLICY_VERSION stays at conservative/6 while paragraph text changes and inline nodes disappear, so older cache entries will diverge and hand off to the agent.
reviewed c589b4e by Hermes on behalf of @okwasniewski
| if (implicitRole(el) !== null && !isPresentational(el)) return true; | ||
| if (accessibleName(el) !== null) return true; | ||
| return directTextOf(el) !== ''; | ||
| return directTextOf(el) !== '' && !isReadInLine(el); |
Member
Author
There was a problem hiding this comment.
Should inline elements that are plausibly interactive (a tabindex, an onclick, or an <a> without href) stay listed here? Right now <p>Text <span onclick="" tabindex="0">change</span></p> lists only the <p>, so the span has no ref to act on. Keeping those listed, or limiting the drop to presentational tags like strong, em and code, would cover it. A test with that markup would pin it.
Member
Author
There was a problem hiding this comment.
Yes, real regression vs main. Fixed in 90277f2: an inline element that offers an action (numeric tabindex, onclick attribute or script-set property, <a> without href) stays listed with its own words while the sentence still reads whole. Test pins your markup plus those cases. Went with that over a strong/em/code tag list so plain spans are not listed twice. Gap left: a span wired only via addEventListener (incl. React delegated onClick) with no tabindex/role exposes nothing readable, so it still folds into the sentence; closing that needs a cursor-style guess.
…text bound in docs
okwasniewski
force-pushed
the
oskar/inline-link-text
branch
from
October 7, 2026 09:53
c589b4e to
7eb8b4e
Compare
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The tree the agent reads dropped every word inside an inline element from its sentence.
<p>Read our <a>privacy policy</a> for details.</p>reached the model astext="Read our for details.", so explore reported missing copy that the page actually shows (13 of 17 findings on one marketing site). Every agent step had the same gap.Fixes ENG-919
What changed
directTextOf, which counts only its own text nodes. It now useslineTextOf(packages/web/src/in-page/read-semantics.ts): the element's text plus the text of each visible inline descendant (inline*,ruby,display: contents), in place. A block child, a line break, a form control, an image, or a frame splits words and contributes no text of its own.a,strong,em,code,span). Only an element with text of its own reads through its children. A wrapper made only of elements lists them the same way it did before (<button><span>Save</span></button>does not change).strong,em,code, a plainspan) is no longer listed separately once its parent's line includes that text (isReadInLine). A link, a button, or a node with a test id is still listed so the agent can act on it. The words of a link now appear twice (in the sentence and in the link's name), but the removed fragment lines save about as many tokens: a benchmark suite run used 76.4k tokens here vs 76.5k on main.getByText) is unchanged.Cache keys do not change: tree text is not part of the key. Recorded anchors can change, though. One web-benchmark entry (
promo-storefront) recorded<strong>Jonas</strong> ★★★★★ Great bass…as two text anchors, and the branch failed it withREPLAY_STALE(end-mismatch). That entry is re-recorded here (same actions, the anchor is now one line). The re-recorded entry uses the@e2e-dev/web0.13 key from #825. The other 29 entries in the suite are still under the 0.12 key, and the benchmark has been red on main since #825. Re-keying them belongs in its own re-record PR (like #833), so they are left alone here.Verified
Ran it locally: yes
packages/web/tests/integration/line-text.test.tsis the new regression test. Against main's reader it fails 4 of 6 tests (the wrapper case passes there as intended), and all 6 pass on this branch. The web package suite passes (crosscheck and conformance included), and so does crosscheck against every web-benchmark scenario (CROSSCHECK_BENCHMARK_URL)./planspage with inline links, bold, italic, and code (not in the nav), plustests-agent/judgments.e2e.ts"assert reads a sentence whole across its inline link and emphasis", paired withgetByTextchecks. Run live withgateway/openai/gpt-6-luna-fastand--no-cache.tests-agentwithCI=1 --strict-cachepassed 33/33 against the 0.12 keys before the rebase and the review fixes, and the re-recordedpromo-storefrontentry replays with no model call under the 0.13 key.e2e explore "Go to the /plans page and review its copy: report any sentence with missing or broken words"agent.assertsees for/plans(unbox-ai)text="Plans start at per seat, billed , and include the CLI."plus loose"$12","annually","e2e"linestext="Plans start at $12 per seat, billed annually, and include the e2e CLI."agent.asserton the two full sentences, 5 runsBefore the PR, a fresh-context review found two problems, and both are fixed with tests. Zero-size and
font-size: 0inline text had leaked into the line, and inline text after the 512-char cut was lost.Overlap with open reader PRs (#871, #895, #898, #900): this change touches none of their hunks in
read-semantics.ts. It adds a new test file instead of editingcrosscheck/fixtures.ts.