Fix: Prevent Accidental Map Zooming While Scrolling on Landing Page #2508
Conversation
Summary by CodeRabbit
WalkthroughLeaflet map in ChapterMap now initializes with scrollWheelZoom disabled and shows an accessible overlay ("Click to interact with map"). Clicking or pressing Enter/Space on the overlay enables scrollWheelZoom and sets an active state; mouseout disables zoom unless cursor moves into map/popups. Tests updated for these behaviors and accessibility. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
164-168: Consider testing event listener behavior, not just registration.While this test verifies that event listeners are registered, it doesn't verify what those handlers actually do when triggered. The overlay tests cover overlay interaction, but the map's own click and mouseout handlers' behavior remains untested.
Consider adding assertions to verify the behavior:
it('sets up event listeners for map interaction', () => { + const L = require('leaflet') render(<ChapterMap {...defaultProps} />) expect(mockMap.on).toHaveBeenCalledWith('click', expect.any(Function)) expect(mockMap.on).toHaveBeenCalledWith('mouseout', expect.any(Function)) + + // Verify click handler behavior + const clickHandler = mockMap.on.mock.calls.find(call => call[0] === 'click')[1] + clickHandler() + expect(mockMap.scrollWheelZoom.enable).toHaveBeenCalled() })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ChapterMap.test.tsx(7 hunks)frontend/src/components/ChapterMap.tsx(4 hunks)
🔇 Additional comments (10)
frontend/__tests__/unit/components/ChapterMap.test.tsx (5)
1-1: LGTM! Mock setup properly supports new interaction testing.The addition of
fireEventand the extended mock withonandscrollWheelZoomprovide the necessary infrastructure to test the interactive overlay and scroll wheel zoom behavior.Also applies to: 9-13
102-113: LGTM! Structural assertions properly validate layout changes.The test correctly verifies that the map container has the expected Tailwind utility classes and that the parent container has the
relativepositioning needed for the overlay.
135-135: LGTM! Correctly expects scroll wheel zoom disabled by default.This assertion properly validates that the map is initialized with
scrollWheelZoom: false, which is the core of the fix to prevent accidental zooming during page scrolling.
240-290: LGTM! Comprehensive interactive overlay test coverage.The test suite thoroughly covers:
- Initial overlay presence
- Click and keyboard interactions (Enter, Space)
- ScrollWheelZoom enablement on interaction
- Accessibility attributes (role, tabIndex, aria-label)
The repeated pattern of querying the overlay element is acceptable for test clarity, though you could optionally extract a helper function if the pattern becomes more complex.
349-361: LGTM! Properly validates style application hierarchy.The test correctly verifies that custom styles are applied to the parent container while the map container maintains its Tailwind utility classes. This demonstrates good separation between user-provided styles and component-internal styling.
frontend/src/components/ChapterMap.tsx (5)
3-3: LGTM!The
useStateimport is correctly added to support the new interactive state management.
22-22: LGTM!The state initialization correctly defaults to
false, ensuring the map starts in inactive mode with the overlay visible.
26-34: LGTM!Disabling
scrollWheelZoomby default is the correct approach to prevent accidental map interaction during page scrolling.
41-45: LGTM!The click handler enables scroll wheel zoom on user interaction, correctly implementing the intended behavior. While this logic overlaps with the overlay's onClick handler, it serves as a useful fallback during state transitions and ensures zoom is re-enabled if the user clicks the map directly after it becomes inactive.
119-146: Excellent accessibility implementation!The overlay implementation is well-structured with comprehensive accessibility features:
- Proper semantic role (
button)- Keyboard navigation support (Enter/Space keys)
- Clear aria-label for screen readers
- Preventive
preventDefault()to avoid page scroll on Space keyThe conditional rendering logic correctly shows/hides the overlay based on map active state, and the z-index ensures the overlay properly blocks map interaction when inactive.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Caution Docstrings generation - FAILED No docstrings were generated. |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@mdkaifansari04 please run make check-test locally and address found issues before we review
…n and improve accessibility, and run make check-test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
164-168: Add coverage for re-disabling scroll zoom on mouseoutRight now we only assert that the Leaflet
mouseoutlistener is registered. If that handler stopped callingscrollWheelZoom.disable, the suite would still pass and the original scroll-trap bug would come back unnoticed. Please capture the registered handler and assert that invoking it actually issues an extradisable—that keeps the regression net tight.it('sets up event listeners for map interaction', () => { render(<ChapterMap {...defaultProps} />) expect(mockMap.on).toHaveBeenCalledWith('click', expect.any(Function)) expect(mockMap.on).toHaveBeenCalledWith('mouseout', expect.any(Function)) }) + + it('re-disables scroll wheel zoom when the cursor leaves the map', () => { + const { getByText } = render(<ChapterMap {...defaultProps} />) + + const mouseoutHandler = (mockMap.on as jest.Mock).mock.calls.find( + ([eventName]) => eventName === 'mouseout' + )?.[1] as (event: { originalEvent?: { relatedTarget: Element | null } }) => void + + expect(mouseoutHandler).toBeDefined() + if (!mouseoutHandler) { + throw new Error('Leaflet mouseout handler was not registered') + } + + const overlay = getByText('Click to interact with map').closest('button') + expect(overlay).not.toBeNull() + fireEvent.click(overlay!) + + const disableCallsBefore = mockMap.scrollWheelZoom.disable.mock.calls.length + + mouseoutHandler({ originalEvent: { relatedTarget: null } }) + + expect(mockMap.scrollWheelZoom.disable.mock.calls.length).toBe(disableCallsBefore + 1) + })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ChapterMap.test.tsx(6 hunks)frontend/src/components/ChapterMap.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/ChapterMap.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Looks and works great!
One small request ⏬
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChapterMap.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/src/components/ChapterMap.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/ChapterMap.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ChapterMap.tsx (1)
128-156: Well-implemented overlay with good accessibility—consider improving focus management.The overlay button properly supports both mouse and keyboard interaction with appropriate ARIA attributes. The implementation successfully solves the accidental zoom issue.
Recommended improvement: Manage keyboard focus after activation.
When the overlay is dismissed, keyboard focus is lost because the button is removed from the DOM. Transfer focus to the map container to maintain keyboard navigation flow:
+ const mapContainerRef = useRef<HTMLDivElement | null>(null) + useEffect(() => { if (!mapRef.current) { mapRef.current = L.map('chapter-map', { worldCopyJump: false, maxBounds: [ [-90, -180], [90, 180], ], maxBoundsViscosity: 1.0, scrollWheelZoom: false, }).setView([20, 0], 2) + + // Make container focusable for keyboard users + const container = mapRef.current.getContainer() + if (container) { + container.setAttribute('tabindex', '0') + container.setAttribute('aria-label', 'Interactive chapter map') + } return ( <div className="relative" style={style}> - <div id="chapter-map" className="w-full h-full" /> + <div ref={mapContainerRef} id="chapter-map" className="w-full h-full" /> {!isMapActive && ( <button type="button" tabIndex={0} className="absolute inset-0 z-[1000] flex cursor-pointer items-center justify-center rounded-[inherit] bg-black/10" onClick={() => { mapRef.current?.scrollWheelZoom.enable() setIsMapActive(true) + // Move focus to map container after overlay is dismissed + setTimeout(() => mapContainerRef.current?.focus(), 0) }} onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault() mapRef.current?.scrollWheelZoom.enable() setIsMapActive(true) + setTimeout(() => mapContainerRef.current?.focus(), 0) } }}Optional refinement: Extract activation logic.
The enable-and-activate logic appears in both
onClickandonKeyDown. If you prefer, extract it into a helper:const activateMap = () => { mapRef.current?.scrollWheelZoom.enable() setIsMapActive(true) setTimeout(() => mapContainerRef.current?.focus(), 0) }Then use
onClick={activateMap}and callactivateMap()in theonKeyDownhandler afterpreventDefault(). Given the minimal duplication, this is entirely optional.Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChapterMap.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/src/components/ChapterMap.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/ChapterMap.tsx
🪛 GitHub Actions: Run CI/CD
frontend/src/components/ChapterMap.tsx
[error] 1-1: Unstaged changes detected. Run 'make check' and use 'git add' to address it.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
frontend/src/components/ChapterMap.tsx (3)
3-3: LGTM—State management setup is correct.The
useStateimport andisMapActivestate are properly initialized to support the click-to-activate interaction pattern.Also applies to: 22-22
33-33: LGTM—Map initialization and event handlers work correctly.The map initializes with scroll-wheel zoom disabled, and the event handlers properly manage the activation state. The mouseout logic correctly ignores movement within the map container (including controls and popups), which preserves full interactivity after activation.
Also applies to: 41-54
1-1: Original review comment is too vague; request manual verification.The review doesn't specify which checks failed (formatting, linting, spelling, or code generation). The developer should:
- Check the CI pipeline logs to identify the specific issue(s)
- Run the corresponding fix command locally:
pnpm run format(for formatting issues)pnpm run lint(for linting issues)pnpm run graphql-codegen(for schema generation)- Stage and commit the changes
Running
make checkblindly may not work locally depending on environment setup (requires Docker for spelling checks).
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Looks good! Thank you @mdkaifansari04 ! 👍🏼
2 participants
Proposed change
Fixes: #1493
Implemented a click-to-activate interaction pattern that prevents accidental zooming while preserving full map functionality
Video Demonstration
2025-10-29.00-35-53.mov
Additional: Project detail test failing
The ProjectDetails test failure was NOT related to your ChapterMap changes. It was caused by:
Checklist
make check-testlocally; all checks and tests passed.