-
Notifications
You must be signed in to change notification settings - Fork 0
fix(cursor): z-stacking suppression for occluded cursors (A24) #86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,204 @@ | ||
| /** | ||
| * Tests for the cursor z-stacking suppression check applied at finalize | ||
| * time in `ink.tsx`. The check answers: "is the declared cursor cell | ||
| * actually visible to the user, or is something painted on top?" | ||
| * | ||
| * Pure-helper style: hand-built DOM trees + manual nodeCache rect | ||
| * population. Mirrors `hit-test.test.ts`. | ||
| * | ||
| * The motivating bug (A24, issue #83): a `<TextInput>`'s caret kept | ||
| * rendering through any modal that didn't itself contain a focused | ||
| * input. Cursors are paint primitives and should follow paint order. | ||
| */ | ||
|
|
||
| import { describe, expect, it } from 'vitest' | ||
| import { isCursorVisibleAt, isDescendantOrSelf } from './cursor-visibility.js' | ||
| import { type DOMElement, appendChildNode, createNode } from './dom.js' | ||
| import { nodeCache } from './node-cache.js' | ||
|
|
||
| function setRect(node: DOMElement, x: number, y: number, w: number, h: number): void { | ||
| nodeCache.set(node, { x, y, width: w, height: h }) | ||
| } | ||
|
|
||
| describe('isDescendantOrSelf', () => { | ||
| it('returns true for the same node', () => { | ||
| const n = createNode('ink-box') | ||
| expect(isDescendantOrSelf(n, n)).toBe(true) | ||
| }) | ||
|
|
||
| it('returns true for a descendant', () => { | ||
| const ancestor = createNode('ink-box') | ||
| const child = createNode('ink-box') | ||
| appendChildNode(ancestor, child) | ||
| const grandchild = createNode('ink-box') | ||
| appendChildNode(child, grandchild) | ||
| expect(isDescendantOrSelf(grandchild, ancestor)).toBe(true) | ||
| }) | ||
|
|
||
| it('returns false for an unrelated subtree', () => { | ||
| const a = createNode('ink-box') | ||
| const b = createNode('ink-box') | ||
| expect(isDescendantOrSelf(a, b)).toBe(false) | ||
| }) | ||
|
|
||
| it('returns false when ancestor is below node in the tree', () => { | ||
| // Reversed direction: descendant→ancestor walk does not match | ||
| // ancestor→descendant. | ||
| const ancestor = createNode('ink-box') | ||
| const child = createNode('ink-box') | ||
| appendChildNode(ancestor, child) | ||
| expect(isDescendantOrSelf(ancestor, child)).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe('isCursorVisibleAt', () => { | ||
| // Helpers — the screen-painted predicate is provided by the caller. | ||
| // Tests pass a stub that either reports all cells painted (modal has | ||
| // background / border / text) or all cells empty (transparent layout | ||
| // wrapper). | ||
| const allPainted = () => true | ||
| const allEmpty = () => false | ||
| const paintedAt = (cells: Array<[number, number]>) => (c: number, r: number) => | ||
| cells.some(([cx, cy]) => cx === c && cy === r) | ||
|
|
||
| it('returns true when the declared cell is on the declared node', () => { | ||
| const root = createNode('ink-root') | ||
| setRect(root, 0, 0, 100, 100) | ||
| const input = createNode('ink-box') | ||
| setRect(input, 10, 10, 30, 5) | ||
| appendChildNode(root, input) | ||
|
|
||
| // Cursor declared at input's caret cell (15, 12) — input owns it. | ||
| expect(isCursorVisibleAt(root, input, 15, 12, allPainted)).toBe(true) | ||
| }) | ||
|
|
||
| it('returns true when the declared cell is on a descendant of the declared node', () => { | ||
| const root = createNode('ink-root') | ||
| setRect(root, 0, 0, 100, 100) | ||
| const wrapper = createNode('ink-box') | ||
| setRect(wrapper, 10, 10, 30, 5) | ||
| appendChildNode(root, wrapper) | ||
| const innerText = createNode('ink-text') | ||
| setRect(innerText, 12, 11, 20, 1) | ||
| appendChildNode(wrapper, innerText) | ||
|
|
||
| expect(isCursorVisibleAt(root, wrapper, 15, 11, allPainted)).toBe(true) | ||
| }) | ||
|
|
||
| it('returns FALSE when a higher-z modal paints over the declared cell (A24 repro)', () => { | ||
| const root = createNode('ink-root') | ||
| setRect(root, 0, 0, 100, 100) | ||
| const input = createNode('ink-box') | ||
| setRect(input, 10, 10, 30, 5) | ||
| appendChildNode(root, input) | ||
|
|
||
| // Modal on top, absolute + zIndex, COVERING the cursor cell, AND | ||
| // it actually painted (background/border/text). Cursor suppressed. | ||
| const modal = createNode('ink-box') | ||
| modal.style = { position: 'absolute', zIndex: 100 } | ||
| setRect(modal, 5, 5, 50, 20) | ||
| appendChildNode(root, modal) | ||
|
|
||
| expect(isCursorVisibleAt(root, input, 15, 12, allPainted)).toBe(false) | ||
| }) | ||
|
|
||
| it('returns TRUE when a transparent layout-only Box overlays the cell (codex review)', () => { | ||
| // The codex P2: an empty absolute Box with no background / no border | ||
| // / no text covers the input's cell at the layout level but never | ||
| // wrote to the screen. hit-test resolves to the layout Box, but the | ||
| // cell's screen contents are blank → NOT occluded → cursor visible. | ||
| const root = createNode('ink-root') | ||
| setRect(root, 0, 0, 100, 100) | ||
| const input = createNode('ink-box') | ||
| setRect(input, 10, 10, 30, 5) | ||
| appendChildNode(root, input) | ||
|
|
||
| const transparentOverlay = createNode('ink-box') | ||
| transparentOverlay.style = { position: 'absolute', zIndex: 100 } | ||
| setRect(transparentOverlay, 0, 0, 100, 100) | ||
| appendChildNode(root, transparentOverlay) | ||
|
|
||
| // Cell at (15, 12) is empty in the screen buffer (overlay never | ||
| // painted). isCellPainted returns false → suppression skipped → | ||
| // cursor visible. | ||
| expect(isCursorVisibleAt(root, input, 15, 12, allEmpty)).toBe(true) | ||
| }) | ||
|
|
||
| it('mixed: overlay only paints part of its rect — cursor under painted region suppressed, under empty region visible', () => { | ||
| const root = createNode('ink-root') | ||
| setRect(root, 0, 0, 100, 100) | ||
| const input = createNode('ink-box') | ||
| setRect(input, 10, 10, 50, 5) | ||
| appendChildNode(root, input) | ||
|
|
||
| // Overlay covers the input area at the layout level; in the screen | ||
| // buffer it actually painted only at (20, 12) — say, a single | ||
| // glyph. The other cells are blank (consumer used it as a positioning | ||
| // wrapper around a small visible element). | ||
| const overlay = createNode('ink-box') | ||
| overlay.style = { position: 'absolute', zIndex: 100 } | ||
| setRect(overlay, 10, 10, 50, 5) | ||
| appendChildNode(root, overlay) | ||
|
|
||
| const painted = paintedAt([[20, 12]]) | ||
| expect(isCursorVisibleAt(root, input, 20, 12, painted)).toBe(false) // painted cell → occluded | ||
| expect(isCursorVisibleAt(root, input, 15, 12, painted)).toBe(true) // empty cell → visible | ||
| }) | ||
|
|
||
| it('returns true when the modal covers a different cell than the cursor', () => { | ||
| const root = createNode('ink-root') | ||
| setRect(root, 0, 0, 100, 100) | ||
| const input = createNode('ink-box') | ||
| setRect(input, 10, 10, 30, 5) | ||
| appendChildNode(root, input) | ||
| const modal = createNode('ink-box') | ||
| modal.style = { position: 'absolute', zIndex: 100 } | ||
| setRect(modal, 50, 50, 30, 10) | ||
| appendChildNode(root, modal) | ||
|
|
||
| expect(isCursorVisibleAt(root, input, 15, 12, allPainted)).toBe(true) | ||
| }) | ||
|
|
||
| it('returns true when the declared cell is inside a focused TextInput inside the modal', () => { | ||
| const root = createNode('ink-root') | ||
| setRect(root, 0, 0, 100, 100) | ||
| const bgInput = createNode('ink-box') | ||
| setRect(bgInput, 5, 5, 40, 10) | ||
| appendChildNode(root, bgInput) | ||
|
|
||
| const modal = createNode('ink-box') | ||
| modal.style = { position: 'absolute', zIndex: 100 } | ||
| setRect(modal, 10, 10, 50, 20) | ||
| appendChildNode(root, modal) | ||
|
|
||
| const modalInput = createNode('ink-box') | ||
| setRect(modalInput, 15, 15, 30, 5) | ||
| appendChildNode(modal, modalInput) | ||
|
|
||
| expect(isCursorVisibleAt(root, modalInput, 17, 16, allPainted)).toBe(true) | ||
| }) | ||
|
|
||
| it('returns false when the cell is off any rendered element', () => { | ||
| const root = createNode('ink-root') | ||
| setRect(root, 0, 0, 100, 100) | ||
| const input = createNode('ink-box') | ||
| setRect(input, 10, 10, 30, 5) | ||
| appendChildNode(root, input) | ||
|
|
||
| expect(isCursorVisibleAt(root, input, 200, 200, allPainted)).toBe(false) | ||
| }) | ||
|
|
||
| it('returns true for a sibling that has higher z but DOES NOT cover the cursor cell', () => { | ||
| const root = createNode('ink-root') | ||
| setRect(root, 0, 0, 100, 100) | ||
| const input = createNode('ink-box') | ||
| setRect(input, 10, 10, 30, 5) | ||
| appendChildNode(root, input) | ||
| const otherModal = createNode('ink-box') | ||
| otherModal.style = { position: 'absolute', zIndex: 100 } | ||
| setRect(otherModal, 60, 60, 30, 10) | ||
| appendChildNode(root, otherModal) | ||
|
|
||
| expect(isCursorVisibleAt(root, input, 15, 12, allPainted)).toBe(true) | ||
| }) | ||
| }) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /** | ||
| * Cursor-visibility helpers for the z-stacking suppression check | ||
| * applied at finalize time in `ink.tsx`. Extracted so the logic can be | ||
| * unit-tested against hand-built DOM trees (same pattern as | ||
| * `hit-test.ts` itself) without spinning up the full render pipeline. | ||
| * | ||
| * Why this exists: `useDeclaredCursor` is last-write-wins. Without | ||
| * occlusion-awareness, a `<TextInput>`'s caret keeps rendering at its | ||
| * declared cell even when a modal Box paints over that cell. Cursors | ||
| * are paint primitives and should follow the same z-stacking the rest | ||
| * of the renderer uses; this module supplies the predicate that says | ||
| * "is the declared cell still the declarer's to own?" | ||
| */ | ||
|
|
||
| import type { DOMElement } from './dom' | ||
| import { hitTest } from './hit-test' | ||
|
|
||
| /** | ||
| * True if `node` is `ancestor` itself or any descendant of it (walks | ||
| * the parentNode chain). | ||
| */ | ||
| export function isDescendantOrSelf(node: DOMElement, ancestor: DOMElement): boolean { | ||
| let n: DOMElement | undefined = node | ||
| while (n) { | ||
| if (n === ancestor) return true | ||
| n = n.parentNode | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| /** | ||
| * True if a cursor declared by `declaredNode` at screen cell (col, row) | ||
| * should be visually rendered. | ||
| * | ||
| * Two-stage check: | ||
| * | ||
| * 1. **Cell painted?** — `isCellPainted(col, row)` asks the screen | ||
| * buffer whether anything was actually written to the cell this | ||
| * frame. An empty cell can't occlude — even if a higher-z layout | ||
| * wrapper's rect contains the cell, it didn't paint anything | ||
| * visible, so the declarer's subtree (or just blank space) shows | ||
| * through and the cursor should render. Without this guard, an | ||
| * empty `<Box position="absolute" zIndex={100}>` overlay covering | ||
| * the screen would suppress every cursor below it (codex review | ||
| * on PR #86). | ||
| * | ||
| * 2. **Topmost layout owner is the declarer?** — `hitTest` returns | ||
| * the topmost paint-order element whose layout rect contains the | ||
| * cell. If it's the declarer or a descendant, the declarer owns | ||
| * the cell visually and the cursor parks. Otherwise something | ||
| * above the declarer wrote into this cell and the cursor must be | ||
| * suppressed to match what the user sees. | ||
| * | ||
| * The screen-buffer check via `isCellPainted` is provided by the | ||
| * caller (ink.tsx wraps `screen.isEmptyCellAt`) so this module stays | ||
| * dependency-light and unit-testable against stubbed cells. | ||
| */ | ||
| export function isCursorVisibleAt( | ||
| root: DOMElement, | ||
| declaredNode: DOMElement, | ||
| col: number, | ||
| row: number, | ||
| isCellPainted: (col: number, row: number) => boolean, | ||
| ): boolean { | ||
| // Empty cells can never occlude. Cursor renders. | ||
| if (!isCellPainted(col, row)) return true | ||
| const hit = hitTest(root, col, row) | ||
| if (hit === null) return false | ||
| return isDescendantOrSelf(hit, declaredNode) | ||
| } | ||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a higher-z absolute Box is used only as a positioning/layout wrapper (no
backgroundColor,opaque, border, or child at this cell), it does not actually write any terminal cells over the focused input, buthitTeststill returns that Box because the point is inside its layout rect. This makes the new visibility check hide the TextInput cursor even though the underlying input remains visible; for example, an empty transparent overlay wrapper covering the screen will suppress every cursor below it. The occlusion test needs to account for what painted the cell, not just which layout rect contains it.Useful? React with 👍 / 👎.