fix: droppable search navigates to results page; lock html+body scroll#44
Merged
mekarpeles merged 7 commits intomainfrom Apr 27, 2026
Merged
fix: droppable search navigates to results page; lock html+body scroll#44mekarpeles merged 7 commits intomainfrom
mekarpeles merged 7 commits intomainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes droppable (showFacets=true) search submit behavior to navigate to a results URL and improves mobile overlay scroll-lock by locking both <body> and <html>, with accompanying Playwright/Vitest coverage and process documentation updates.
Changes:
- Add fallback navigation for droppable submit + “See all results” via
_buildSearchUrl()and a cancelableol-searchevent. - Lock/unlock both
document.bodyanddocument.documentElementscrolling when the mobile overlay is open/closed (including cleanup on disconnect). - Expand Playwright coverage for navigation + scroll lock; update testing/process docs (TDD guidance).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/ol-search-bar.js | Adds _buildSearchUrl(), cancelable ol-search, fallback navigation, and html+body scroll lock. |
| frontend/tests/facet-and-submit.spec.js | Adds Playwright navigation tests for submit/Enter/See-all and cancelable-event suppression. |
| frontend/tests/mobile-overlay.spec.js | Adds Playwright assertions for html+body overflow locking/restoration during overlay. |
| frontend/src/components/ol-search-bar.panel-overlay.test.js | Adjusts static-analysis slice length to keep overlay-focus contract test stable. |
| AGENTS.md | Documents a TDD workflow expectation for interactive bug fixes and navigation assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mekarpeles
added a commit
that referenced
this pull request
Apr 27, 2026
- Use _scrollLockActive flag in disconnectedCallback instead of
_mobileExpanded to correctly release scroll lock when component
is removed while overlay is open
- Save/restore _prevBodyOverflow and _prevDocumentOverflow before
locking so we never clobber pre-existing inline overflow styles
- Use new URL('/search', siteBase) to safely handle trailing slash
in siteBase (fixes potential double-slash in navigation URL)
- Add Playwright test asserting active filters appear in navigation URL
- Widen updated() slice to 2000 chars in panel-overlay static test
Issue #44 — two critical bugs found because tests only checked that ol-search fired, not that navigation happened: Navigation (droppable mode): - Add _buildSearchUrl(q, filters) that constructs siteBase/search?q=... with full filter params serialized as OL search URL query params - Make ol-search event cancelable — hosts that handle navigation themselves call e.preventDefault() to suppress the fallback - _submit() and "See all N results" both navigate via window.location.href when showFacets=true and event not prevented - Embedded mode (showFacets=false) is unaffected Body scroll lock: - Lock document.documentElement.style.overflow in addition to document.body; many browsers/frameworks scroll <html>, leaving a page scrollbar behind the position:fixed overlay - Restore both on close and in disconnectedCallback TDD: Playwright tests written BEFORE the fix (red → green): - submit button / Enter key / "See all N" each navigate to /search?q= - preventDefault() suppresses navigation - document.body + documentElement overflow:hidden while overlay open - both restored to '' when overlay closes AGENTS.md: explicit TDD section requiring a failing test before any fix
- Use _scrollLockActive flag in disconnectedCallback instead of
_mobileExpanded to correctly release scroll lock when component
is removed while overlay is open
- Save/restore _prevBodyOverflow and _prevDocumentOverflow before
locking so we never clobber pre-existing inline overflow styles
- Use new URL('/search', siteBase) to safely handle trailing slash
in siteBase (fixes potential double-slash in navigation URL)
- Add Playwright test asserting active filters appear in navigation URL
- Widen updated() slice to 2000 chars in panel-overlay static test
Vitest: assert scroll lock is gated on _open + showFacets, not
only _mobileExpanded, so desktop droppable panel also locks scroll.
Playwright: two desktop tests — overflow:hidden while panel open,
overflow restored after Escape closes it.
Tests fail red against current code; will go green once the lock
trigger is moved from changed.has('_mobileExpanded') to changed.has('_open').
…e overlay
Move scroll-lock trigger from changed.has('_mobileExpanded') to
changed.has('_open'), with condition this._open && this.showFacets.
This ensures the page behind the search panel cannot scroll on
desktop viewports just as it cannot on mobile.
mekarpeles
force-pushed
the
fix/droppable-navigation
branch
from
6eb686e to
4acf1fd
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Two critical bugs exposed by the gap between what tests checked vs. what actually needed to work.
Bug 1 — Submit/Enter/See-all never navigated in droppable mode
`_submit()` always dispatched `ol-search` but never navigated. The existing test only checked `window.__olSearchFired === true` — not that the URL changed. Nobody caught it.
Fix:
Bug 2 — Page behind overlay still scrollable
`document.body.style.overflow = 'hidden'` only locks ``. Many browsers (and the demo page) actually scroll ``. The overlay was `position:fixed` but the scrollbar remained visible behind it.
Fix: Lock both `document.documentElement` and `document.body` whenever the droppable search panel is open on any viewport (not only the mobile full-screen overlay). Restore both on close and in `disconnectedCallback`. The lock is keyed on `changed.has('_open') && this.showFacets` so embedded mode is unaffected.
TDD process
Playwright tests were written before the fixes (failing red), proving the regression, then the fixes made them green:
`AGENTS.md` updated with an explicit TDD section so this pattern is the default going forward.
Test plan