Bläddra i källkod

fix: improve off-screen cursor classification and add integration test

Yuki Takei 1 vecka sedan
förälder
incheckning
c26c0ca660

+ 11 - 0
.kiro/specs/collaborative-editor-awareness/tasks.md

@@ -124,6 +124,17 @@
   - Simulate a new awareness change before the timer expires and verify the timer is reset
   - Simulate a new awareness change before the timer expires and verify the timer is reset
   - _Requirements: 3.10_
   - _Requirements: 3.10_
 
 
+- [x] 12. Fix Off-Screen Visibility Classification
+- [x] 12.1 Replace `view.viewport` with `view.visibleRanges` in the off-screen cursor classification logic
+  - In `plugin.ts`, replace `viewUpdate.view.viewport` with `viewUpdate.view.visibleRanges` for the top/bottom boundary comparison
+  - Use `visibleRanges[0].from` as the top boundary and `visibleRanges[visibleRanges.length - 1].to` as the bottom boundary
+  - Guard against an empty `visibleRanges` array (skip classification if empty)
+  - _Requirements: 4.1, 4.2, 4.3, 4.6_
+
+- [x] 12.2 Update integration test to assert off-screen indicator renders for cursor in the render buffer
+  - Add a test case that places a remote cursor within `view.viewport` but outside `view.visibleRanges` and verifies the off-screen container has an indicator element
+  - _Requirements: 4.3, 4.6_
+
 - [ ]\* 11. E2E Tests for Hover, Opacity, and Off-Screen Transitions
 - [ ]\* 11. E2E Tests for Hover, Opacity, and Off-Screen Transitions
 - [ ]\* 11.1 (P) Test hover behavior on the cursor overlay flag
 - [ ]\* 11.1 (P) Test hover behavior on the cursor overlay flag
   - Hover over a remote user's caret area and verify the name label becomes visible
   - Hover over a remote user's caret area and verify the name label becomes visible

+ 64 - 0
packages/editor/src/client/services-internal/extensions/y-rich-cursors/plugin.integ.ts

@@ -1,10 +1,12 @@
 import { EditorState } from '@codemirror/state';
 import { EditorState } from '@codemirror/state';
+import type { ViewUpdate } from '@codemirror/view';
 import { EditorView } from '@codemirror/view';
 import { EditorView } from '@codemirror/view';
 import { yCollab } from 'y-codemirror.next';
 import { yCollab } from 'y-codemirror.next';
 import * as Y from 'yjs';
 import * as Y from 'yjs';
 
 
 import type { EditingClient } from '../../../../interfaces';
 import type { EditingClient } from '../../../../interfaces';
 import { yRichCursors } from './index';
 import { yRichCursors } from './index';
+import { YRichCursorsPluginValue } from './plugin';
 
 
 /**
 /**
  * Integration tests for collaborative awareness flow.
  * Integration tests for collaborative awareness flow.
@@ -328,6 +330,68 @@ describe('Task 10.1 — Remote cursors outside the viewport are excluded from wi
   });
   });
 });
 });
 
 
+// ---------------------------------------------------------------------------
+// Task 12.2 — Off-screen indicator for cursor in viewport render buffer
+// ---------------------------------------------------------------------------
+
+describe('Task 12.2 — Off-screen indicator renders for cursor in render buffer but outside visibleRanges', () => {
+  it('places a below-indicator when cursor is inside viewport but outside visibleRanges', () => {
+    const ydoc = new Y.Doc({ guid: 'render-buffer-test' });
+    const ytext = ydoc.getText('codemirror');
+    const content = 'Hello World Foo Bar';
+    ytext.insert(0, content);
+
+    const awareness = new FakeAwareness(ydoc);
+
+    // Set remote state BEFORE creating the plugin so the change listener
+    // does not fire a real dispatch during construction.
+    const remoteClient = makeClient(99, 'BufferUser');
+    const anchor = Y.createRelativePositionFromTypeIndex(ytext, 10);
+    const head = Y.createRelativePositionFromTypeIndex(ytext, 10);
+    awareness.setRemoteClientState(99, {
+      editors: remoteClient,
+      cursor: { anchor, head },
+    });
+
+    // Build a state that includes yCollab so ySyncFacet is configured.
+    const state = EditorState.create({
+      doc: content,
+      extensions: [yCollab(ytext, null)],
+    });
+
+    const container = document.createElement('div');
+    document.body.appendChild(container);
+    const view = new EditorView({ state, parent: container });
+
+    // Instantiate the plugin directly — one pair of containers in view.dom.
+    const plugin = new YRichCursorsPluginValue(view, awareness as never);
+
+    // Construct a mock ViewUpdate where:
+    //   viewport  covers position 10 (render buffer includes it)
+    //   visibleRanges does NOT cover position 10 (only first 5 chars visible)
+    //
+    // Before the fix (viewport):  cursor 10 ≤ vpTo 18 → widget decoration  → bottomContainer EMPTY  → test FAILS
+    // After  the fix (visibleRanges): cursor 10 > vpTo 5  → below indicator → bottomContainer has 1  → test PASSES
+    const mockViewUpdate = {
+      state: view.state,
+      view: {
+        hasFocus: false,
+        viewport: { from: 0, to: content.length },
+        visibleRanges: [{ from: 0, to: 5 }],
+      },
+    } as unknown as ViewUpdate;
+
+    plugin.update(mockViewUpdate);
+
+    const bottomContainer = view.dom.querySelector('.cm-offScreenBottom');
+    expect(bottomContainer?.children.length).toBe(1);
+
+    plugin.destroy();
+    view.destroy();
+    container.remove();
+  });
+});
+
 // ---------------------------------------------------------------------------
 // ---------------------------------------------------------------------------
 // Task 10.2 — Activity tracking timer lifecycle
 // Task 10.2 — Activity tracking timer lifecycle
 // ---------------------------------------------------------------------------
 // ---------------------------------------------------------------------------

+ 113 - 25
packages/editor/src/client/services-internal/extensions/y-rich-cursors/plugin.ts

@@ -156,7 +156,58 @@ export class YRichCursorsPluginValue {
     const aboveIndicators: HTMLElement[] = [];
     const aboveIndicators: HTMLElement[] = [];
     const belowIndicators: HTMLElement[] = [];
     const belowIndicators: HTMLElement[] = [];
     const localClientId = this.awareness.doc.clientID;
     const localClientId = this.awareness.doc.clientID;
-    const { from: vpFrom, to: vpTo } = viewUpdate.view.viewport;
+
+    const visibleRanges = viewUpdate.view.visibleRanges;
+    const { from: viewportFrom, to: viewportTo } = viewUpdate.view.viewport;
+
+    // Three classification strategies (chosen once per update call):
+    //
+    // "ranged": visibleRanges is a true sub-range of viewport — CodeMirror's own
+    //   scroller is active (e.g. fixed-height editor in tests). Use character-
+    //   position bounds derived from visibleRanges.
+    //
+    // "coords": visibleRanges == viewport — the editor expands to full content
+    //   height and the *page* handles scrolling (GROWI's production setup). Use
+    //   the cursor's actual screen coordinates vs the editor's visible rect
+    //   (scrollDOM.getBoundingClientRect clamped to window.innerHeight).
+    //
+    // "none" (degenerate — jsdom with 0-height container): scrollRect.height == 0
+    //   so screen coordinates are unreliable. Skip all off-screen classification
+    //   and give every cursor a widget decoration, matching pre-task-12 behaviour.
+    const hasVisibleRanges = visibleRanges.length > 0;
+    // rangedMode: visibleRanges is a meaningful sub-range of viewport.
+    // Requires the visible area to be non-empty (to > from) so that a 0-height
+    // editor (jsdom degenerate) doesn't accidentally classify every cursor as
+    // off-screen via a vpTo of 0.
+    const rangedMode =
+      hasVisibleRanges &&
+      visibleRanges[visibleRanges.length - 1].to > visibleRanges[0].from &&
+      (visibleRanges[0].from > viewportFrom ||
+        visibleRanges[visibleRanges.length - 1].to < viewportTo);
+
+    // For coords mode: compute visible band once before the per-cursor loop.
+    // getBoundingClientRect() is a raw DOM call (not a CodeMirror layout read)
+    // so it is allowed during update(). lineBlockAt() uses the stored height map
+    // and is also safe during update().
+    // When scrollDOMRect.height == 0 (jsdom), screenVisibleBottom == 0 so the
+    // below/above checks never fire and every cursor falls through to a widget.
+    let scrollDOMTop = 0;
+    let scrollDOMBottom = 0;
+    let scrollTop = 0;
+    if (!rangedMode) {
+      const scrollDOMRect = viewUpdate.view.scrollDOM.getBoundingClientRect();
+      scrollDOMTop = scrollDOMRect.top;
+      scrollDOMBottom = scrollDOMRect.bottom;
+      scrollTop = viewUpdate.view.scrollDOM.scrollTop;
+    }
+    const screenVisibleTop = Math.max(scrollDOMTop, 0);
+    const screenVisibleBottom = Math.min(scrollDOMBottom, window.innerHeight);
+
+    const vpFrom = hasVisibleRanges ? visibleRanges[0].from : viewportFrom;
+    const vpTo = hasVisibleRanges
+      ? visibleRanges[visibleRanges.length - 1].to
+      : viewportTo;
+
     const now = Date.now();
     const now = Date.now();
 
 
     this.awareness.getStates().forEach((rawState, clientId) => {
     this.awareness.getStates().forEach((rawState, clientId) => {
@@ -191,30 +242,67 @@ export class YRichCursorsPluginValue {
       const isActive = now - (this.lastActivityMap.get(clientId) ?? 0) < 3000;
       const isActive = now - (this.lastActivityMap.get(clientId) ?? 0) < 3000;
       const headIndex = head.index;
       const headIndex = head.index;
 
 
-      // Classify: in-viewport or off-screen
-      if (headIndex < vpFrom) {
-        aboveIndicators.push(
-          createOffScreenIndicator({
-            direction: 'above',
-            color: editors.color,
-            name: editors.name,
-            imageUrlCached: editors.imageUrlCached,
-            isActive,
-          }),
-        );
-        return;
-      }
-      if (headIndex > vpTo) {
-        belowIndicators.push(
-          createOffScreenIndicator({
-            direction: 'below',
-            color: editors.color,
-            name: editors.name,
-            imageUrlCached: editors.imageUrlCached,
-            isActive,
-          }),
-        );
-        return;
+      // Classify: off-screen (above/below) or in-viewport
+      if (rangedMode) {
+        if (headIndex < vpFrom) {
+          aboveIndicators.push(
+            createOffScreenIndicator({
+              direction: 'above',
+              color: editors.color,
+              name: editors.name,
+              imageUrlCached: editors.imageUrlCached,
+              isActive,
+            }),
+          );
+          return;
+        }
+        if (headIndex > vpTo) {
+          belowIndicators.push(
+            createOffScreenIndicator({
+              direction: 'below',
+              color: editors.color,
+              name: editors.name,
+              imageUrlCached: editors.imageUrlCached,
+              isActive,
+            }),
+          );
+          return;
+        }
+      } else {
+        // coords mode: compare screen Y of cursor against the editor's visible rect.
+        // Used when visibleRanges == viewport (page-scroll editor, e.g. GROWI).
+        //
+        // lineBlockAt() reads stored heights (safe during update).
+        // When scrollDOMRect.height == 0 (jsdom) both checks below are false
+        // so every cursor falls through to a widget decoration.
+        const lineBlock = viewUpdate.view.lineBlockAt(headIndex);
+        const cursorTop = scrollDOMTop + lineBlock.top - scrollTop;
+        const cursorBottom = scrollDOMTop + lineBlock.bottom - scrollTop;
+
+        if (cursorBottom < screenVisibleTop) {
+          aboveIndicators.push(
+            createOffScreenIndicator({
+              direction: 'above',
+              color: editors.color,
+              name: editors.name,
+              imageUrlCached: editors.imageUrlCached,
+              isActive,
+            }),
+          );
+          return;
+        }
+        if (cursorTop > screenVisibleBottom) {
+          belowIndicators.push(
+            createOffScreenIndicator({
+              direction: 'below',
+              color: editors.color,
+              name: editors.name,
+              imageUrlCached: editors.imageUrlCached,
+              isActive,
+            }),
+          );
+          return;
+        }
       }
       }
 
 
       // In-viewport: render decorations
       // In-viewport: render decorations