Просмотр исходного кода

fix: integrate hash-based auto-scroll into SearchResultContent with container-relative scroll strategy

Yuki Takei 1 день назад
Родитель
Сommit
815fb0e69a

+ 77 - 5
.kiro/specs/auto-scroll/design.md

@@ -6,17 +6,17 @@
 
 **Users**: End users navigating to hash-linked sections benefit from reliable scroll positioning. Developers integrating the hook into new views (PageView, SearchResultContent, future views) benefit from a standardized, configurable API.
 
-**Impact**: Refactors the existing `useHashAutoScroll` hook from a PageView-specific implementation into a shared, configurable hook. Renames and updates the rendering status attribute protocol for clarity and declarative usage.
+**Impact**: Refactors the existing `useHashAutoScroll` hook from a PageView-specific implementation into a shared, configurable hook. Renames and updates the rendering status attribute protocol for clarity and declarative usage. Also integrates hash-based auto-scroll into `SearchResultContent`, where the content pane has an independent scroll container.
 
 ### Goals
 - Provide a single reusable hook for hash-based auto-scroll across all content views
 - Support customizable target resolution and scroll behavior per caller
 - Establish a clear, declarative rendering-status attribute protocol for async-rendering components
 - Maintain robust resource cleanup with timeout-based safety bounds
+- Integrate `SearchResultContent` as a second consumer with container-relative scroll strategy
 
 ### Non-Goals
 - Adding `data-growi-is-content-rendering` to attachment-refs (Ref/Refs/RefImg/RefsImg/Gallery), or RichAttachment — these also cause layout shifts but require more complex integration; deferred to follow-up
-- Replacing SearchResultContent's keyword-highlight scroll with this hook (requires separate evaluation)
 - Supporting non-browser environments (SSR) — this is a client-only hook
 
 ## Architecture
@@ -57,8 +57,8 @@ graph TB
         LSX[Lsx]
     end
 
-    PV -->|calls| HOOK
-    SRC -->|calls| HOOK
+    PV -->|calls, default scrollTo| HOOK
+    SRC -->|calls, custom scrollTo| HOOK
     HOOK -->|delegates| WATCH
     WATCH -->|queries| CONST
     DV -->|sets/toggles| CONST
@@ -137,7 +137,7 @@ Key decisions:
 | 3.1–3.6 | Re-scroll after rendering | watchRenderingAndReScroll | scrollTo callback | Auto-Scroll Lifecycle |
 | 4.1–4.7 | Rendering attribute protocol | Rendering Status Constants, DrawioViewer, MermaidViewer, PlantUmlViewer, Lsx | GROWI_IS_CONTENT_RENDERING_ATTR | — |
 | 4.8 | ResizeObserver re-render cycle | DrawioViewer | GROWI_IS_CONTENT_RENDERING_ATTR | — |
-| 5.1–5.5 | Page-type agnostic design | useContentAutoScroll | UseContentAutoScrollOptions | — |
+| 5.1–5.5 | Page-type agnostic design | useContentAutoScroll, SearchResultContent | UseContentAutoScrollOptions | — |
 | 5.6, 5.7, 6.1–6.3 | Cleanup and safety | useContentAutoScroll, watchRenderingAndReScroll | cleanup functions | — |
 
 ## Components and Interfaces
@@ -151,6 +151,7 @@ Key decisions:
 | MermaidViewer (modification) | features/mermaid | Add rendering-status attribute lifecycle to async SVG render | 4.3, 4.4, 4.7 | Rendering Status Constants (P0) | State |
 | PlantUmlViewer (new) | features/plantuml | Wrap PlantUML `<img>` to provide rendering-status attribute lifecycle | 4.3, 4.4, 4.7 | Rendering Status Constants (P0) | State |
 | Lsx (modification) | remark-lsx | Add rendering-status attribute lifecycle to async page list fetch | 4.3, 4.4, 4.7 | Rendering Status Constants (P0) | State |
+| SearchResultContent (modification) | features/search | Integrate useContentAutoScroll with container-relative scrollTo; suppress keyword scroll when hash is present | 5.1, 5.2, 5.3, 5.5 | useContentAutoScroll (P0), scrollWithinContainer (P0) | State |
 
 ### Client Hooks
 
@@ -369,6 +370,77 @@ const GROWI_IS_CONTENT_RENDERING_SELECTOR =
 
 ---
 
+### SearchResultContent Integration
+
+#### SearchResultContent (modification)
+
+| Field | Detail |
+|-------|--------|
+| Intent | Integrate `useContentAutoScroll` for hash-based navigation within the search result content pane; coordinate with the existing keyword-highlight scroll to prevent position conflicts |
+| Requirements | 5.1, 5.2, 5.3, 5.5 |
+
+**Background**: `SearchResultContent` renders page content inside a div with `overflow-y-scroll` (`#search-result-content-body-container`). It already has a separate keyword-highlight scroll mechanism — a `useEffect` with no dependency array that uses `MutationObserver` to scroll to the first `.highlighted-keyword` element using `scrollWithinContainer`. These two scroll mechanisms must coexist without overriding each other.
+
+**Container-Relative Scroll Problem**
+
+`element.scrollIntoView()` (the hook's default `scrollTo`) scrolls the viewport, not the scrolling container. Since `#search-result-content-body-container` is the scrolling unit, a custom `scrollTo` is required:
+
+```
+scrollTo(target):
+  distance = target.getBoundingClientRect().top
+            - container.getBoundingClientRect().top
+            - SCROLL_OFFSET_TOP
+  scrollWithinContainer(container, distance)
+```
+
+The `container` reference is obtained via `scrollElementRef.current` (the existing React ref already present in the component). The `SCROLL_OFFSET_TOP = 30` constant is reused from the keyword scroll for visual consistency.
+
+**Scroll Conflict Resolution**
+
+When a URL hash is present, both mechanisms would fire:
+1. `useContentAutoScroll` → scroll to the hash target element
+2. Keyword MutationObserver → scroll to the first `.highlighted-keyword` element (500ms debounced)
+
+This creates a race condition where the keyword scroll overrides the hash scroll. Resolution strategy:
+
+> **When `window.location.hash` is non-empty, the keyword-highlight `useEffect` returns early.** Hash-based scroll takes priority.
+
+Concretely, the existing keyword-scroll `useEffect` gains a guard at the top:
+
+```
+if (window.location.hash.length > 0) return;
+```
+
+When no hash is present, keyword scroll proceeds exactly as before — no behavior change for the common case.
+
+**Hook Call Site**
+
+```typescript
+const scrollTo = useCallback((target: HTMLElement) => {
+  const container = scrollElementRef.current;
+  if (container == null) return;
+  const distance =
+    target.getBoundingClientRect().top -
+    container.getBoundingClientRect().top -
+    SCROLL_OFFSET_TOP;
+  scrollWithinContainer(container, distance);
+}, []);
+
+useContentAutoScroll({
+  key: page._id,
+  contentContainerId: 'search-result-content-body-container',
+  scrollTo,
+});
+```
+
+- `resolveTarget` defaults to `document.getElementById` — heading elements have `id` attributes set by the remark processing pipeline, so the default resolver works without customization.
+- `scrollTo` uses the existing `scrollElementRef` directly to avoid redundant `getElementById` lookup.
+- `useCallback` with empty deps array ensures callback identity is stable across renders (the hook wraps it in a ref internally, but stable identity avoids any risk of spurious re-renders).
+
+**File**: `apps/app/src/features/search/client/components/SearchPage/SearchResultContent.tsx`
+
+---
+
 ## Error Handling
 
 ### Error Strategy

+ 11 - 6
.kiro/specs/auto-scroll/research.md

@@ -23,11 +23,15 @@
 - **Sources**: `apps/app/src/features/search/client/components/SearchPage/SearchResultContent.tsx`
 - **Findings**:
   - Container ID: `search-result-content-body-container`
-  - Uses MutationObserver to find `.highlighted-keyword` elements and scroll to the first one
-  - Debounced at 500ms
-  - Does NOT use URL hash for scrolling — scrolls to highlighted search terms
-  - No cleanup function (intentional, per inline comment)
-- **Implications**: SearchResultContent's scroll target is NOT hash-based. The auto-scroll hook must accept a custom target resolver to support non-hash targets. The two scroll mechanisms (hash-based and keyword-based) may coexist or the keyword-based one may be replaced by the generalized hook.
+  - Container has `overflow-y-scroll` — is the scroll unit, not the viewport
+  - Uses MutationObserver to find `.highlighted-keyword` elements and scroll to the first one using `scrollWithinContainer`
+  - Debounced at 500ms; `SCROLL_OFFSET_TOP = 30`
+  - Does NOT use URL hash — scrolls to highlighted search terms
+  - `useEffect` has no dependency array (fires on every render); no cleanup (intentional per inline comment)
+- **Implications (updated)**:
+  - `scrollIntoView()` default is inappropriate; custom `scrollTo` using `scrollWithinContainer` is required
+  - When `window.location.hash` is non-empty, the keyword scroll overrides hash scroll after 500ms debounce — must be suppressed via early return guard
+  - The `resolveTarget` default (`document.getElementById`) works correctly; heading `id` attributes are set by the remark pipeline
 
 ### DrawioViewer Rendering Attribute Pattern
 - **Context**: Requirement 4.4 mandates declarative true/false toggling
@@ -90,7 +94,8 @@
 - **Rationale**: With declarative true/false toggling, bare `[attr]` matches both states; value selector is required
 
 ## Risks & Mitigations
-- **Risk**: SearchResultContent's existing scroll logic may conflict with the generalized hook — **Mitigation**: Allow callers to provide custom `resolveTarget` so SearchResultContent can resolve its keyword target independently
+- **Risk**: SearchResultContent's existing keyword-highlight scroll may conflict with hash-based scroll — **Mitigation**: Guard the keyword-scroll `useEffect` with `if (window.location.hash.length > 0) return;` so hash scroll takes priority when a hash is present; keyword scroll proceeds unchanged otherwise
+- **Risk**: `scrollIntoView()` default scrolls the viewport when SearchResultContent's container has `overflow-y-scroll` — **Mitigation**: Provide a custom `scrollTo` closure using `scrollWithinContainer` with offset from the container's bounding rect
 - **Risk**: Renaming the attribute requires coordinated changes across `@growi/core`, `remark-drawio`, and consuming apps — **Mitigation**: Constants are centralized; single constant rename propagates via imports
 - **Risk**: MutationObserver on `subtree: true` may be expensive on large pages — **Mitigation**: Retained 10s maximum watch timeout from current implementation
 

+ 4 - 4
.kiro/specs/auto-scroll/spec.json

@@ -1,9 +1,9 @@
 {
   "feature_name": "auto-scroll",
   "created_at": "2026-04-02T00:00:00.000Z",
-  "updated_at": "2026-04-02T00:00:00.000Z",
+  "updated_at": "2026-04-06T00:00:00.000Z",
   "language": "en",
-  "phase": "implementation-complete",
+  "phase": "tasks-generated",
   "approvals": {
     "requirements": {
       "generated": true,
@@ -15,8 +15,8 @@
     },
     "tasks": {
       "generated": true,
-      "approved": true
+      "approved": false
     }
   },
-  "ready_for_implementation": true
+  "ready_for_implementation": false
 }

+ 20 - 0
.kiro/specs/auto-scroll/tasks.md

@@ -81,3 +81,23 @@
   - Delete the old hook file and its test file from the PageView directory
   - Verify that PageView auto-scroll behavior is preserved with manual testing or existing test coverage
   - _Requirements: 5.1, 5.4, 5.5_
+
+- [ ] 6. Integrate useContentAutoScroll into SearchResultContent
+- [ ] 6.1 (P) Add hash-based auto-scroll with container-relative scroll strategy
+  - Call `useContentAutoScroll` with `key: page._id` and `contentContainerId: 'search-result-content-body-container'`
+  - Provide a custom `scrollTo` closure that calculates the target element's offset relative to the container's bounding rect and calls `scrollWithinContainer` with the same `SCROLL_OFFSET_TOP` constant already used for keyword scroll
+  - Capture the container via the existing `scrollElementRef` in the closure to avoid a redundant `getElementById` lookup
+  - Do not provide a custom `resolveTarget` — heading elements have `id` attributes set by the remark pipeline, so the default `getElementById` resolver works correctly
+  - _Requirements: 5.1, 5.2, 5.3, 5.5_
+
+- [ ] 6.2 (P) Suppress keyword-highlight scroll when a URL hash is present
+  - Add an early return guard at the top of the existing keyword-scroll `useEffect`: if `window.location.hash` is non-empty, return immediately so hash-based scroll is not overridden by the debounced keyword scroll
+  - Preserve the existing keyword-scroll behavior fully when no hash is present — the MutationObserver, debounce interval, and `scrollWithinContainer` call remain unchanged
+  - _Requirements: 5.1, 5.5_
+
+- [ ]* 6.3 Write tests for SearchResultContent auto-scroll integration
+  - Test that `useContentAutoScroll` is called with the correct `key` and `contentContainerId` when the component mounts
+  - Test that the custom `scrollTo` scrolls within the container (not the viewport) by verifying `scrollWithinContainer` is called with the correct distance
+  - Test that the keyword-scroll `useEffect` skips observation when `window.location.hash` is non-empty
+  - Test that the keyword-scroll `useEffect` sets up the MutationObserver normally when no hash is present
+  - _Requirements: 5.1, 5.2, 5.3, 5.5_