feat(ol-mobile): OL mobile search plugin for ?ref=ol embeds#1552
Open
mekarpeles wants to merge 13 commits into
Open
feat(ol-mobile): OL mobile search plugin for ?ref=ol embeds#1552mekarpeles wants to merge 13 commits into
mekarpeles wants to merge 13 commits into
Conversation
When ?q=<term> is in the URL, BookReader already searches on load.
When ?q= is present but empty, it now opens the search panel without
running a search — making search-inside discoverable by default.
Two changes:
- BookReader.js: use `!= null` instead of truthiness check so that an
empty string from URLSearchParams.get('q') passes through to
initialSearchTerm (previously empty ?q= was silently ignored)
- plugin.search.js: add else-if branch for initialSearchTerm === '' that
calls toggleSidebar() to open the panel without triggering a search
Closes: internetarchive/openlibrary#11912
Related: #1166
Without ?q= in the URL, urlParams.get('q') returns null. The previous
ternary `searchTerm ? searchTerm : ''` converted null to '' which
caused the new `else if (initialSearchTerm === '')` branch in
plugin.search.js to fire toggleSidebar() on every page load, opening
the search panel unexpectedly and breaking the e2e tests.
There was a problem hiding this comment.
Pull request overview
Adds an Open Library–specific mobile search UI for BookReader embeds, intended to replace the current mobile search drawer UX with an inline header + inline results panel when ?ref=ol is present on small viewports.
Changes:
- Introduces a new
OLMobilePluginthat injects a two-row mobile header and renders inline search results via BookReader search events. - Adds OL-mobile-scoped styling (
.BRolMobile) and wires it into the main SCSS build. - Emits and loads a new plugin bundle entry (
plugins/plugin.search.ol-mobile.js) in the IA demo page.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
webpack.config.js |
Adds a new webpack entry to emit the OL mobile search plugin bundle. |
src/plugins/search/plugin.search.ol-mobile.js |
New plugin implementing the OL mobile header, search input row, and inline results rendering. |
src/css/BookReader.scss |
Imports the new OL mobile SCSS partial into the main stylesheet build. |
src/css/_BRolMobile.scss |
New styles for the OL mobile header + inline results panel, scoped via .BRolMobile. |
BookReaderDemo/demo-internetarchive.html |
Loads the new plugin bundle after plugin.search.js for demo/testing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When ?ref=ol is in the URL and the viewport is <= 640 px, replaces the standard BookReader sidebar/toolbar with a native-style mobile header: Row 1 (always visible): IA logo | collapse/expand chevron | bookmark | search | more Row 2 (toggled by search icon): search field with clear button | Cancel Result navigator bar (appears after clicking a result): < prev | (x/y) page + plain-text snippet | next > Key behaviors: - Inline search results panel below Row 2 instead of sidebar drawer - Sidebar / ToggleSearchMenu suppressed via event interception + shadow-DOM style injection (two-level: ia-bookreader → iaux-item-navigator) - BRcontainer.top kept correct via MutationObserver (BookReader's own layout code overwrites style.top; the observer re-applies our value reactively) - Collapse: entire bar → 25 px down-chevron strip (dark #313131 background) preserving chevron x-position via visibility:hidden on logo/actions - Result nav bar: click a result → compact (x/y) strip; click search input → full results list; click outside wrapper → re-enters preview mode if a match was selected; Cancel clears everything - Search term preserved across focus shifts: type=text + _savedQuery guard against iOS browser clearing the field during page navigation Architecture: - OLMobilePlugin extends BookReaderPlugin - All DOM refs cached at build time (_searchBtn, _clearBtn, _navPrevBtn, _navNextBtn, _navPosEl, _navSnippetEl) — no querySelector in hot paths - CHEVRON_UP/DOWN/LEFT/RIGHT as module-level SVG constants - MutationObserver + ResizeObserver for reactive BRcontainer offset - _deactivateSearch delegates fully to _closeSearch + _clearResults Files added: src/plugins/search/plugin.search.ol-mobile.js src/css/_BRolMobile.scss src/css/BookReader.scss (+1 line: @use import) webpack.config.js (+1 line: entry point) BookReaderDemo/demo-internetarchive.html (+1 line: script tag)
Tests act as living documentation of intended behaviour — each group has a WHY comment explaining the edge case or design constraint it guards. Test groups: 1. Layout — BRcontainer must not be occluded on load or after collapse/expand (guards against BookReader's MutationObserver offset being overwritten) 2. Bookmark — first click adds, second triggers delete confirmation (delegates to ia-bookmarks.bookmarkButtonClicked via shadow DOM) 3. Chevron — ~40px expanded, ~25px collapsed, same x-position in both states, y near 0 when collapsed, restored after cycle 4. Search — magnifying glass toggle, Cancel clears input+results, Enter shows results, result click hides panel, page navigation, refocus restores results 5. Result nav bar — hidden on load, appears after result click, (x/y) format, non-empty snippet, prev/next disabled state, Next advances counter, input click restores results, Cancel clears, new search resets, no occlusion 6. Search term preservation — iOS type=search quirk guard; _savedQuery restore 7. Click-outside — tapping book while results visible collapses them and restores preview nav bar if a match was selected Infrastructure: playwright.config.js — iPhone 12 device profile, local http-server package.json — test:playwright and test:playwright:ci scripts
mekarpeles
force-pushed
the
1551/mobile-ol-reader-ui
branch
from
372e2d0 to
ac5519c
Compare
mekarpeles
changed the title
…s book pins - _prefillFromUrl(): on init, reads br.plugins.search.options.initialSearchTerm (set by BookReader from ?q= before any plugin init runs), opens Row 2, fills the input, sets _savedQuery, and shows the loading spinner so results appear automatically when the page loads with a search term in the URL. - _deactivateSearch(): now calls removeSearchResults(suppressFragmentChange=true) so Cancel also clears the yellow pin overlays BookReader rendered on book pages, not just the plugin's own UI. - Tests: 5 new Playwright tests covering URL pre-fill (row visible, input pre-filled, results appear, regression when ?q= absent) and 1 test verifying Cancel clears br.plugins.search.searchTerm.
- Add `const BookReader = window.BookReader` at module level (same pattern as plugin.search.js) so BookReader?.registerPlugin() doesn't throw ReferenceError in strict/module scope. - _shouldActivate(): use br.readQueryString() instead of window.location.search so ?ref=ol in the URL hash is also detected. - _bindSearchEvents(): filter all search event handlers by `instance === br` to prevent cross-talk when multiple BookReader instances share the same document. - _submitSearch(): guard against br.search being undefined (only bound when search plugin is enabled) by calling br.plugins.search.search(query) directly.
- Bookmark icon turns red when the current page has a bookmark (uses
`.BRolMobileBookmarkBtn--active` toggled via `_updateBookmarkIcon()`)
- A count bubble (blue circle) appears between the bookmark and search
icons showing total bookmark count; hidden when 0 bookmarks
- Clicking the bubble opens a bookmark nav bar:
< (x/y) [note input / 'add note' placeholder, × to clear] >
matching the search result nav bar in layout
- Prev/next arrows navigate between bookmarked pages via `jumpToIndex`
- Note input saves on blur/Enter via `iaBookmarks.saveBookmark()`
- Opening bookmark nav collapses search row; opening search hides nav
- State refreshes on `bookmarksChanged` (composed CustomEvent) and on
`BookReader:fragmentChange` for page navigation
Two bugs:
1. ia-bookmarks is created via document.createElement() but not attached
to the DOM until the bookmarks panel opens. Events dispatched on it
don't bubble to document. Fix: bind the bookmarksChanged listener
directly on the ia-bookmarks element (listeners on the element fire
regardless of DOM connection).
2. bookmarks property starts as [] (before API fetch) then becomes {}
keyed by leafNum. Object.keys([]) returns [] so the initial state
always showed 0 bookmarks. Fix: skip Array.isArray() case.
Also: after bookmarkButtonClicked(), bookmarksChanged fires asynchronously
via LitElement's updated() lifecycle. Add a 50ms setTimeout refresh so
the icon turns red without waiting for the async event.
New bookmark interaction model:
- Clicking bookmark on an unbookmarked page: adds bookmark, turns Row 1
red, and slides down a note bar with input + blue Save button
- Clicking bookmark on a bookmarked page: if note exists, shows a JS
confirm dialog ("Removing this bookmark will also remove note: '...'"),
then removes the bookmark; note bar closes and bar returns to normal
- Note bar loads the existing note for the current page; Save persists
it via iaBookmarks.saveBookmark() with brief "Saved!" feedback
- Navigating to a bookmarked page auto-shows the note bar + red row
- Navigating away auto-hides both
Bookmark count bubble:
- Now red (matching the bookmark color) rather than blue
- Clicking opens a scrollable bookmark list (like search results):
each entry shows "Page N" + first line of note if present
- Clicking an item jumps to that page; note bar opens automatically
Removed: the prev/next nav bar between bookmarks (replaced by the list)
…row viewports Add an always-visible page navigation bar inside the BookReader scrubber (.BRolMobilePageNav) with: - ‹ / › prev/next buttons, disabled at first/last leaf - Leaf chip (tap to edit inline): "N / total", Enter navigates - Page chip (only when page number is asserted): "Page X", tap to edit - Chapter chip (only when TOC data available): section label, tap to open TOC - Section chip scrolls horizontally via overflow-x: auto when title is long Fix horizontal overflow on narrow viewports (Pixel 6a, 412px CSS width): - br-mode-1up gets overflow-x: hidden so a wide page-1 scan (432px) cannot be swipe-scrolled inside the Lit element, leaving asymmetric margins - getToolBarHeight override is set immediately in _setupMobileUI() (not PostInit) so resizeBRcontainer() at BR line 692 already sees our wrapper height - PostInit handler fires applyOffset() + synchronous window.resize to correct the initial container.style.top; no debounce, which avoids triggering switchNavbarControls() after injectPageNav() has already hidden the flip buttons All 81 Playwright tests pass.
- Remove unused _header class field (was only needed locally in _setupMobileUI) - Fix redundant CSS padding: 14px 14px → 14px on .BRolMobileStatus - Remove double blank line between .BRolMobileRow1 and .BRolMobileChromeToggle - Drop unused suffixRect variable in scroll-overflow spec (ESLint no-unused-vars) All 81 tests pass; lint clean.
…s to Nth result When ?q=TERM is present in the URL (or in #search/TERM hash) and the OL mobile plugin is active, the search bar (Row 2) now opens automatically on load with the term pre-filled and the search button in its active state. Previously the sidebar suppression meant ?q= produced no visible feedback. Also falls back to reading ?q= directly from the URL in case the embedder sets it after BookReader's early URL parsing pass, and closes any ia-item-navigator search panel that may have opened in OL production. New: ?result=N (1-based) in the query string auto-navigates to the Nth search result once results arrive, consuming the param once and clearing it. Out-of-range values clamp to the last result. Example shareable URL: ?ocaid=...&ref=ol&q=parian&result=2 4 new Playwright tests added; 85/85 pass.
cdrini
force-pushed
the
11912/open-search-panel-on-preview
branch
2 times, most recently
from
f5be0b0 to
21c0fe1
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
Adds a mobile-optimized search UI for BookReader when embedded from Open Library (
?ref=ol+ viewport ≤ 640 px). Replaces the sidebar drawer with a lightweight native-style header that lives above the book content.UI structure:
< (x/y) p. N — snippet >strip that appears after a result is clicked; tap search input to restore full results; tap outside to re-enter preview modeKey design decisions:
ToggleSearchMenufully suppressed — event interception + shadow-DOM style injection (ia-bookreader → iaux-item-navigator, two levels deep)BRcontainer.topkept correct viaMutationObserver(BookReader's own layout code overwritesstyle.top; observer re-applies our value reactively)visibility: hiddenon logo/actionstype=text+_savedQueryguard (iOS Safari quirk withtype=search)querySelectorin hot pathsCommits
feat— full plugin (plugin.search.ol-mobile.js,_BRolMobile.scss) + infra wiring (webpack entry, SCSS import, demo HTML)test— Playwright regression suite (29 tests across 7 groups, each with a WHY comment)Testing
npm run build— clean webpack compile/?ocaid=goody&ref=olon 390 px viewportnpm run test:playwright(requiresnpm run buildfirst)