Repository navigation
feat(core): add geometry fields to element predicates - #1635
CloudCorpRecords wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Thanks for your first pull request to Reticle! A maintainer aims to review within two days. Before then, |
|
CI caught a real gap in the new commit's predecessor, fixed in 7b27d6c: the geometry fields were added to the zod PredicateSchema but not to the hand-written Predicate type the engine compiles against, so the workspace build failed with TS2339 in engine's predicate.ts. The type now declares fullyInViewport, textClipped, and overlaps with the same docs as the schema, and the engine builds its geometry asks with conditional spreads so no explicitly-undefined property reaches ElementGeometryAsks under exactOptionalPropertyTypes. Verified locally before pushing: full workspace |
|
Follow-up on the latest red verify run: the build and typecheck were fine this time; lint failed on two errors from this change. a11y.ts compared against null with the literal on the right (the repo's yoda rule wants it on the left), and query.ts crossed the 1000-line maximum at 1022 lines once the geometry plumbing landed in it. Both are fixed in the newest commits: the comparison now reads null === clipped, and the overlap helper plus the geometry attachment moved out of query.ts into their own module, adapters/realm/browser/src/dom/geometry.ts, bringing query.ts to 997 lines. The flat-directory guard's record for the dom directory rises to 14 for the new module, with the reason noted beside the entry. Verified locally: eslint clean across the browser package, prettier clean on the touched files, the geometry and a11y suites pass 110/110, the flat-directory guard passes, and the workspace typecheck is clean. |
divshekhar
left a comment
There was a problem hiding this comment.
Thank you for this. The browser measurement and the engine grading are a good shape, and the inconclusive answer for an older SDK is right. Some problems block the merge. The first three can produce a wrong pass.
- A missing
overlapstarget is a plain failure. Undernot, it becomes a pass:not { element: card, overlaps: { testid: "typo" } }passes when the second element does not exist. Please make "the overlaps query matched no element" inconclusive. TheElementGeometrycomment already says that this is a different fact from "the boxes do not intersect". textClippedreads only the element's own box. Overflow does not apply to an inline element, and a browser reportsscrollWidthandclientWidthas 0 for it. So a<span>inside a truncated<div>readstextClipped: false, and that clause passes while the text is cut off. The test setsoverflow-x: hiddenon a<span>with mocked widths, which a real browser does not do. Please measure the nearest box that clips (the element if it is a block, else the nearest ancestor that clips), or answer inconclusive for an inline element. Please change the fixture to a block element.- With
absent: true, the geometry fields are ignored, because the absent branch returns beforecheckGeometry. Please refuse that combination with an inconclusive answer or a schema error, so the caller knows. - The overlap target is
findCandidates(...).candidates[0]: the first candidate in document order, hidden or not. Please use the first visible match, or name in the evidence which element was measured.
Also:
- The branch has merge conflicts with
main. Please rebase (do not mergemainin). - This changes how a verdict is reached. Please run
pnpm gate:conformanceand report the result. CI skips that job for this PR. geometryandoverlapsQueryare new MATCH arguments. Please define their names incoreand use the constants in the engine and the browser, as the wire rule in AGENTS.md says.- #1634 also adds a file to
adapters/realm/browser/src/domand adds lines toquery.ts. Together the two PRs takequery.tsover 1,000 lines and the directory count to 15. Please coordinate with that PR.
3b40320 to
0d19a86
Compare
divshekhar (re-review on reticlehq#1635): - A missing overlaps target is inconclusive, not a failure: under not a plain failure became a pass. A query that matched no visible element can no longer manufacture a verified verdict. - textClipped measures the nearest box that clips: overflow does not apply to inline boxes, so a span inside a truncated div is read at the div. The pixel fixture is now a block element, as a real browser requires. - absent:true with geometry asks is refused as inconclusive instead of silently ignoring the asks. - The overlap target is the first visible match, not the first candidate in document order, hidden or not. - MATCH arg names live in core as MatchArg (types.ts: constants.ts is at the 1000-line cap) and both sides use the constants. - Rebased onto main (no merge); measureGeometry moved to geometry.ts and the overlap resolver with it, keeping a11y.ts and query.ts under the 1000-line lint cap. Guards: core/engine/browser unit suites green (902/1606/2202); gate:conformance green (self/desktop/cli exit 0, self score identical to main baseline). Coupling ceiling 65->66 recorded with the argument. Note for merge order: reticlehq#1634 also adds a dom/ file, so the second PR to merge bumps the recorded dom count 14->15.
|
Re-review addressed and rebased onto main (no merge): missing overlaps target is now inconclusive (no more wrong pass under |
divshekhar (re-review on reticlehq#1635): - A missing overlaps target is inconclusive, not a failure: under not a plain failure became a pass. A query that matched no visible element can no longer manufacture a verified verdict. - textClipped measures the nearest box that clips: overflow does not apply to inline boxes, so a span inside a truncated div is read at the div. The pixel fixture is now a block element, as a real browser requires. - absent:true with geometry asks is refused as inconclusive instead of silently ignoring the asks. - The overlap target is the first visible match, not the first candidate in document order, hidden or not. - MATCH arg names live in core as MatchArg (types.ts: constants.ts is at the 1000-line cap) and both sides use the constants. - Rebased onto main (no merge); measureGeometry moved to geometry.ts and the overlap resolver with it, keeping a11y.ts and query.ts under the 1000-line lint cap. Guards: core/engine/browser unit suites green (902/1606/2202); gate:conformance green (self/desktop/cli exit 0, self score identical to main baseline). Coupling ceiling 65->66 recorded with the argument. Note for merge order: reticlehq#1634 also adds a dom/ file, so the second PR to merge bumps the recorded dom count 14->15. Signed-off-by: Rene Turcios <95189389+CloudCorpRecords@users.noreply.github.com>
0d19a86 to
3c5bcfc
Compare
divshekhar
left a comment
There was a problem hiding this comment.
Thank you. You fixed all four points, and the MatchArg constants and the rebase are correct. Three new problems can still give a wrong answer.
checkGeometryreads onlymatch.elements, which is capped.match.countcan be larger. Main refuses this case for the other fields it checks after the match ("only M were described … narrow the locator"). Give the same inconclusive answer whencountis larger thanelements.length. Otherwise anotcan pass while an element that was not described satisfies the ask.nearestClippingBoxstops at an inline element whose computedoverflowishidden, for example a Tailwindtruncatespan. Overflow does not apply to that box, so its widths read 0. Skip boxes whose computeddisplayisinlineorcontents, and add a test.- If the
overlapsquery also matches the measured element,intersectsis always true. Exclude the measured element from the target.
Minor: text-overflow: ellipsis has no effect without overflow: hidden. Treat it as a clip only when overflow also clips.
#1634 is queued. After it merges, query.ts goes over 1,000 lines with this change, and the dom count must be 15. Rebase after it merges, and move code out of query.ts to stay under the limit.
divshekhar (re-review on reticlehq#1635): - A missing overlaps target is inconclusive, not a failure: under not a plain failure became a pass. A query that matched no visible element can no longer manufacture a verified verdict. - textClipped measures the nearest box that clips: overflow does not apply to inline boxes, so a span inside a truncated div is read at the div. The pixel fixture is now a block element, as a real browser requires. - absent:true with geometry asks is refused as inconclusive instead of silently ignoring the asks. - The overlap target is the first visible match, not the first candidate in document order, hidden or not. - MATCH arg names live in core as MatchArg (types.ts: constants.ts is at the 1000-line cap) and both sides use the constants. - Rebased onto main (no merge); measureGeometry moved to geometry.ts and the overlap resolver with it, keeping a11y.ts and query.ts under the 1000-line lint cap. Guards: core/engine/browser unit suites green (902/1606/2202); gate:conformance green (self/desktop/cli exit 0, self score identical to main baseline). Coupling ceiling 65->66 recorded with the argument. Note for merge order: reticlehq#1634 also adds a dom/ file, so the second PR to merge bumps the recorded dom count 14->15. Signed-off-by: Rene Turcios <95189389+CloudCorpRecords@users.noreply.github.com>
divshekhar (re-review on reticlehq#1635): - A missing overlaps target is inconclusive, not a failure: under not a plain failure became a pass. A query that matched no visible element can no longer manufacture a verified verdict. - textClipped measures the nearest box that clips: overflow does not apply to inline boxes, so a span inside a truncated div is read at the div. The pixel fixture is now a block element, as a real browser requires. - absent:true with geometry asks is refused as inconclusive instead of silently ignoring the asks. - The overlap target is the first visible match, not the first candidate in document order, hidden or not. - MATCH arg names live in core as MatchArg (types.ts: constants.ts is at the 1000-line cap) and both sides use the constants. - Rebased onto main (no merge); measureGeometry moved to geometry.ts and the overlap resolver with it, keeping a11y.ts and query.ts under the 1000-line lint cap. Guards: core/engine/browser unit suites green (902/1606/2202); gate:conformance green (self/desktop/cli exit 0, self score identical to main baseline). Coupling ceiling 65->66 recorded with the argument. Note for merge order: reticlehq#1634 also adds a dom/ file, so the second PR to merge bumps the recorded dom count 14->15. Signed-off-by: Rene Turcios <95189389+CloudCorpRecords@users.noreply.github.com>
3c5bcfc to
2c7a18a
Compare
divshekhar
left a comment
There was a problem hiding this comment.
Thank you. You fixed all four points from my last review, and each fix has a test.
Two problems can still give a wrong verdict:
- A missing element passes under
not. The geometry check runs onlyif (geometry !== undefined && match.matched), so a locator that matches nothing stays a plain miss.notturns a plain miss into a pass (engine/src/question/predicate/predicate.ts:476-478). Sonot { element: { testid: "typo" }, overlaps: … }passes. When geometry asks are present and the locator matched nothing, return inconclusive. textClippedcan read the wrong box.nearestClippingBoxwalks past a block element that does not clip, up to any ancestor that clips. A short<li>in anoverflow-x: hiddencarousel then reports the overflow of the whole list. Stop at the element when it is a block, and walk up only for inline content. Add a test: a block that fits, inside a clipping parent that overflows, must not read as clipped.
Also:
- Remove the
### Fixedsection from the.changesfile. Those bugs were in this unreleased feature, so users never had them. Keep only theAddedentry. - Run
pnpm gate:conformanceagain on the final commit, and put the result in the PR.
Rebuilt cleanly on current main. The conflicts in query.ts, predicate-element.ts and the two guard tests were resolved by hand, keeping both sides: main's framework-dialog coverage and text-match split, and this branch's geometry module and MatchArg symbol. Latest review fixes: - Geometry asks are inconclusive when the locator matched nothing, so a missing element cannot pass under not. - textClipped reads the element's own box when it is a block and walks up only for inline content. New test: a block that fits inside a clipping parent that overflows is not clipped. - The .changes file keeps only the Added entry. The Fixed section described bugs in this unreleased feature, so it is gone. Signed-off-by: Rene Turcios <95189389+CloudCorpRecords@users.noreply.github.com>
2c7a18a to
2b080ef
Compare
Closes #1539
An element predicate could ask that some of an element shows (
state: "inViewport") and nothing else about its geometry, so responsive-layout claims had no verdict: fully on screen versus half off the bottom, two elements overlapping, a label whose text is cut off.This change adds the three fields the report names, measured in the page from the box
reticle_lookalready reports:fullyInViewport: boolean— the whole box is inside the viewport and no clipping ancestor cuts any part of it. It reuses the clipping walkvisibleBoundsalready does, so it composes with the Text clipped byoverflow: hidden/line-clampcounts as visible #1237 clipping work instead of re-deriving it.textClipped: boolean— content beyond the client box underoverflow: hidden | clip, ortext-overflow: ellipsisin effect, with the overflow amounts (pixels per axis) carried on the evidence.overlaps: { <query> }— another element as a query; the boxes must intersect with positive area. Assert the negative with thenotcombinator around the predicate.How it flows:
coredeclares the fields on the element predicate and theElementGeometryevidence on the descriptor. The browser measures only when a MATCH carries the ask (measureGeometryina11y.ts, stamped inmatchQuery, including the second box foroverlaps). The engine grades it inpredicate-element.ts: one element must satisfy every ask, a page that reports no geometry is inconclusive rather than a guess, and failures print the measured box, the viewport, and the overflow amounts. Because the fields live in the schema, they appear in the derived predicate grammar automatically.Proof: the new tests failed before this change and pass after it. The browser cases (
query.geometry.test.ts) cover two viewport widths, a box cut by a clipping ancestor, pixel-exact text clipping (120px), and intersection both ways. The engine cases cover pass, fail with the measured numbers in the message, and the no-geometry inconclusive. Full suites pass locally: browser 2,159 tests, engine 1,578 tests, core 881 tests.