Skip to content

feat: Search modal UX improvements - facet removal, filters alignment, mobile layout and language support#12797

Open
Armansiddiqui9 wants to merge 31 commits into
internetarchive:masterfrom
Armansiddiqui9:feat/search-availability-language-filters
Open

feat: Search modal UX improvements - facet removal, filters alignment, mobile layout and language support#12797
Armansiddiqui9 wants to merge 31 commits into
internetarchive:masterfrom
Armansiddiqui9:feat/search-availability-language-filters

Conversation

@Armansiddiqui9
Copy link
Copy Markdown
Contributor

@Armansiddiqui9 Armansiddiqui9 commented May 23, 2026

Closes #12752
Related #11216

To-dos for @Armansiddiqui9

  • Search results - Let's understand exactly why the autocomplete in the modal has differing results from the Search page.
  • Investigate: If the user has chosen a language and set 'Borrowable' in the filter, it should only show books that are borrowable in their specified language - it does not behave this way currently. Search '1984' and set 'Borrowable' and 'English'. Click the result in the modal and you will be taken to a page that says 'preview only', because there is a Chinese edition that is 'Borrowable'. This is not a great user experience.
  • For users who have not done a search with the new search modal experience yet, we should open the modal with 'Readable online' set to selected in the Availability dropdown
  • Let's add recent searches in the modal. If the user has not entered anything in the search input, then show their recent searches in the search results area, up to 8. We'll need a way to clear the search results as well. These can be per device, not account, so using local storage, though open to other solutions. (see hardcover image example)
image - [ ] Bug: Bar code scanner button missing in header on mobile (my mistake -Lokesh)

To-do's for @lokesh

  • Make sure text inputs are 16px min size on mobile so they don't cause browser zoom
  • Make it easy to close search modal on mobile
  • UX: When one clicks on a work, not clear it’s loading. Provide instant feedback and indicating loading status.
  • Revisit visual treatment for the hierarchy of “readable online”

Builds on the search modal foundation from #12690 (Lokesh's draft), extending it with filter UI (Availability + Language), author suggestions, loading feedback, mobile improvements, and a full header search bar cleanup.

Technical

Search bar cleanup
The legacy SearchBar.js and its "All" facet <select> dropdown have been removed from the header entirely (along with the now-dead header CSS and the SearchBar.test.js suite). Filter scoping moved into the modal, so the header search box is now a single trigger button that opens <ol-search-modal> via initSearchModal().

Search modal UX polish

  • --ol-dialog-top-offset: 54px — anchors the dialog just below the header so it feels like a dropdown, not a modal takeover
  • --ol-dialog-width-large: min(680px, 92vw) — narrower and more focused (was 800px), like mek's prototype
  • --ol-dialog-backdrop-color: hsla(0,0%,0%,0.18) — lighter backdrop, less visual weight
  • Search field wrapped in an inset, rounded box; filter pill padding aligned to the input row
  • Results max-height reduced from 50vh to 320px

Availability filter — dedicated <ol-availability-filter> component
The Availability dropdown is now a bespoke component rather than the generic <ol-options-popover>, because availability has presentation the generic single-select doesn't model:

  • a per-option icon column (book, globe, unlock, clock),
  • a two-state circular indicator — a filled check on the selected option and a hollow "in scope" check on each option the selection contains, and
  • a hierarchy where a nested option ("Free to read now") is visually contained by the broader option above it ("Readable online").

It composes <ol-popover> for animation, focus trap, mobile tray, and Escape/outside-click dismissal — reusing that behaviour rather than reimplementing it. ("In scope" is purely visual; only the radio's checked state is exposed to assistive tech.) This is the revisited visual treatment for the "readable online" hierarchy.

Author suggestions
When the query names the author of one of the top works /search.json already returned, the modal surfaces an author row linking straight to that author's page — covering the common "type a name → I want that author" case (the old Author facet) with no extra Solr round-trip. Matching is self-protecting: a title search returns an author's works, but the title isn't part of their name, so nothing is surfaced. Capped at 3 rows; the matching logic lives in authorSuggestion.js as pure, unit-tested functions.

Loading feedback on result press
Pressing a result navigates the whole window, and the next page can take a beat to start painting. The pressed row now holds full opacity while the rest dim back, its cover darkens under a spinner (mirroring the <ol-button> loading spinner), and the row scales to 0.985 — matching the header search field's press feedback. Cleared on close, on a new query, and on pageshow so it never lingers after a bfcache restore.

Language filter — 20 defaults + full OL catalogue

  • DEFAULT_LANGUAGE_OPTIONS (20 curated languages) shown instantly on first open
  • On first open, _loadAllLanguages() fetches every language in the OL Solr index, merges with static labels, deduplicates and sorts alphabetically
  • Falls back silently to the 20 defaults on network failure or private browsing
  • Multiple language values are OR-ed (not AND-ed); availability + language filters persist across the modal and /search via session storage

Backend — modal results aligned with /search
The modal's availability params now map to the same Solr query the /search page uses, so the two agree. Includes anchoring the negated editions.fq so "Borrowable" returns results, and accepting public_scan/print_disabled on /search.json.

WCAG 1.3.1 fix — <ol-options-popover>
role="radiogroup" was on the <ul> directly, stripping list semantics and making <li> children invalid in the accessibility tree (flagged by accesslint on #12690). Fixed by moving the role to a wrapping <div role="radiogroup"> and keeping the <ul> a pure list.

Shared component infra
FocusableHostMixin / focus-utils.js centralize shadow-DOM focus-trap discovery so the dialog, popover, and filter components share one robust implementation (with unit tests).

Design page
/developers/design documents the components: ol-dialog, ol-options-popover, and the popovers. The options-popover demos were simplified to neutral sort/genre examples so they no longer mimic the (separate) availability filter.

Screenshot

Moving target, see it on testing instead.

Stakeholders

@lokesh

lokesh and others added 2 commits May 23, 2026 15:57
Replace the legacy inline autocomplete dropdown on the header search
bar with an OlDialog-based modal. On mobile it goes fullscreen; on
desktop it anchors near the top of the viewport.

- OlDialog: native <dialog>-based modal with focus trap, animations,
  placement="top", fullscreen-on-mobile, and a custom header slot
- OlOptionsPopover: filter trigger + popover for rich single-select
  options (used by the modal for Availability)
- search-modal/SearchModal: takes over autocomplete duties from
  SearchBar via a new disableAutocomplete option on the legacy bar
- focus-utils, slot-utils: shared helpers for shadow-DOM components
- Registration guards on OlPopover/OlSelectPopover so they don't
  double-define when imported through both lit-components and the
  search-modal webpack consumer
Copy link
Copy Markdown

@accesslint accesslint Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 5 issues across 2 rules.

Comment thread openlibrary/components/lit/OlOptionsPopover.js
Comment thread openlibrary/plugins/openlibrary/js/search-modal/SearchModal.js
Comment thread openlibrary/plugins/openlibrary/js/search-modal/SearchModal.js
Copy link
Copy Markdown

@accesslint accesslint Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 5 issues across 2 rules.

Comment thread openlibrary/components/lit/OlOptionsPopover.js
Comment thread openlibrary/plugins/openlibrary/js/search-modal/SearchModal.js
Comment thread openlibrary/plugins/openlibrary/js/search-modal/SearchModal.js
@mekarpeles
Copy link
Copy Markdown
Member

Thanks for the PR, @Armansiddiqui9!

🤖 Copilot has been assigned for an initial review.

@lokesh is assigned to this PR and currently has:

  • 3 open PR(s) of equal or higher priority to review first
PR triage checklist (maintainers / Pam)
  • PR description — not empty; explains what the change does and how to verify it
  • References an issue — PR body contains a #NNN reference
    • Linked issue is triaged — has a Priority: * label (not just Needs: Triage)
    • Linked issue is assigned — has at least one assignee
  • Commit history clean — no WIP/fixup/conflict noise; commit messages are meaningful
  • CI passing — no failing check-runs
  • Test cases present — if the change touches substantive logic, test coverage exists or is explained
  • Proof of testing — PR body includes a description of what was tested, a screenshot, or a video

Note

This comment was automatically generated by Pam, Open Library's Project AI Manager, on behalf of @mekarpeles. Pam is designed to provide status visibility, perform basic project management functions and relevant codebase research, and provide actionable feedback so contributors aren't left waiting.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Enhances the header search experience by moving filtering and autocomplete into a Lit-based search modal, alongside new reusable dialog/popover components and associated styling/JS wiring.

Changes:

  • Hide the legacy header “All” facet selector via CSS and disable the legacy inline autocomplete in SearchBar when the modal is active.
  • Introduce a new Lit ol-search-modal component with Availability + Language filters, session persistence, and modal-driven autocomplete/results.
  • Add new Lit infrastructure components/utilities (OlDialog, OlOptionsPopover, focus/slot utils) and guard custom-element registration to avoid double-definitions.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
static/css/components/header-bar.css Hides legacy facet UI and adjusts header search input styling/width.
openlibrary/plugins/openlibrary/js/SearchBar.js Adds disableAutocomplete option to allow SearchModal to take over autocomplete.
openlibrary/plugins/openlibrary/js/search-modal/SearchModal.js New Lit search modal implementation with filters, results rendering, and language loading.
openlibrary/plugins/openlibrary/js/search-modal/constants.js Defines Availability + Language option metadata and sessionStorage keys.
openlibrary/plugins/openlibrary/js/ol.js Initializes SearchBar with autocomplete disabled and mounts SearchModal on the header input.
openlibrary/components/lit/utils/slot-utils.js Adds helper for detecting meaningful slotted content.
openlibrary/components/lit/utils/focus-utils.js Adds focus helpers for shadow DOM and slotted focusables.
openlibrary/components/lit/OlSelectPopover.js Prevents double custom-element registration.
openlibrary/components/lit/OlPopover.js Prevents double custom-element registration.
openlibrary/components/lit/OlOptionsPopover.js New single-select “rich options” popover component (WCAG semantics fix included).
openlibrary/components/lit/OlDialog.js New native-<dialog> modal component with focus trap, placement options, and slots.
openlibrary/components/lit/index.js Exports newly added Lit components from the bundle entrypoint.
Comments suppressed due to low confidence (2)

openlibrary/plugins/openlibrary/js/search-modal/SearchModal.js:427

  • _loadAllLanguages() fetches /search.json?...&facets=true&facet=language and expects facet_counts.facet_fields.language, but the FastAPI /search.json endpoint currently always runs with facet=False (no facet_counts). As a result raw becomes [] and the code replaces _languageItems with an empty array, breaking the Language popover. This should use an endpoint that actually returns language facet data (or add one) and avoid overwriting the defaults when facet data is missing/empty.
    async _loadAllLanguages() {
        this._langsLoading = true;
        try {
            const res = await fetch(
                '/search.json?q=*&facets=true&limit=0&facet=language',
                { signal: AbortSignal.timeout?.(8000) }
            );
            if (!res.ok) throw new Error(`HTTP ${res.status}`);
            const data = await res.json();

            const raw = data?.facet_counts?.facet_fields?.language || [];

openlibrary/plugins/openlibrary/js/search-modal/SearchModal.js:493

  • Several new user-facing strings are hardcoded in the component (e.g. dialog aria-label, input placeholder, results headings, button text). Open Library’s frontend typically sources JS UI strings from server-rendered i18n strings (e.g. hidden *-i18n-strings inputs) so they can be translated. Please wire these strings through the existing i18n mechanism instead of embedding English literals in the component.
        return html`
            <ol-dialog
                ?open=${this.open}
                without-header
                fullscreen-on-mobile
                width="large"
                placement="top"
                aria-label="Search Open Library"
                style="
                    --ol-dialog-padding: 0;
                    --ol-dialog-top-offset: 54px;
                    --ol-dialog-animation-duration: 160ms;
                    --ol-dialog-width-large: min(680px, 92vw);
                    --ol-dialog-backdrop-color: hsla(0,0%,0%,0.18);
                "
                @ol-after-open=${this._onDialogOpened}
                @ol-after-close=${this._onDialogClosed}
            >
                <div slot="header" class="bar">
                    ${SearchModal._searchIcon}
                    <input
                        type="search"
                        class="search-input"
                        placeholder="Search books, authors…"
                        aria-label="Search"

Comment on lines +430 to +440
dialog.addEventListener('animationend', () => {
dialog.classList.remove('closing');
dialog.close();

this._restoreFocus();

this.dispatchEvent(new CustomEvent('ol-after-close', {
bubbles: true,
composed: true,
}));
}, { once: true });
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in OlDialog.js from Lokesh's base pull request #12690. The fix is valid flagging for @lokesh to address since modifying his core dialog component is outside the scope of this PR's changes. Happy to include the fix here if preferred.

Comment thread openlibrary/plugins/openlibrary/js/search-modal/SearchModal.js Outdated
Comment thread openlibrary/plugins/openlibrary/js/search-modal/SearchModal.js Outdated
Comment thread openlibrary/plugins/openlibrary/js/search-modal/constants.js Outdated
@Armansiddiqui9 Armansiddiqui9 changed the title feat: Search modal UX improvements - facet removal, filters alignment, mobile layout and language API feat: Search modal UX improvements - facet removal, filters alignment, mobile layout and language support May 23, 2026
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

@accesslint accesslint Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 5 issues across 2 rules.

Comment thread openlibrary/components/lit/OlOptionsPopover.js
Comment thread openlibrary/plugins/openlibrary/js/search-modal/SearchModal.js
Comment thread openlibrary/plugins/openlibrary/js/search-modal/SearchModal.js
Add Web Component Library entries for the two new Lit components
introduced in this branch, following the existing select-popover
partial conventions. Includes real-world examples: availability/sort
filters and custom triggers for ol-options-popover; confirmation,
form, width-preset, and search-modal dialogs for ol-dialog.
Copy link
Copy Markdown

@accesslint accesslint Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 5 issues across 2 rules.

const items = this.items || [];
const heading = this.heading || (this.label || '').toUpperCase();

// FIX (WCAG 1.3.1): role="radiogroup" must NOT be on the <ul> because
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WCAG 1.3.1: <ul> contains direct text content. Wrap in <li>.

<ul> and <ol> must only contain <li>, <script>, or <template> as direct children.

Details

Screen readers announce list structure ('list with 5 items') based on proper markup. Placing non-<li> elements directly inside <ul> or <ol> breaks this structure. Wrap content in <li> elements, or if you need wrapper divs for styling, restructure your CSS to style the <li> elements directly.

return html`
<div class="results">
<h3 class="results-heading">Top results</h3>
<ul class="results-list">${repeat(this._results, r => r.key, r => this._renderResult(r))}</ul>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WCAG 1.3.1: <ul> contains direct text content. Wrap in <li>.

<ul> and <ol> must only contain <li>, <script>, or <template> as direct children.

Details

Screen readers announce list structure ('list with 5 items') based on proper markup. Placing non-<li> elements directly inside <ul> or <ol> breaks this structure. Wrap content in <li> elements, or if you need wrapper divs for styling, restructure your CSS to style the <li> elements directly.

const cover = work.cover_i
? `https://covers.openlibrary.org/b/id/${work.cover_i}-S.jpg`
: COVER_PLACEHOLDER;
return html`<li>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WCAG 1.3.1: <li> is not contained in a <ul>, <ol>, or <menu>.

<li> elements must be contained in a <ul>, <ol>, or <menu>.

Details

List items (<li>) only have semantic meaning inside a list container (<ul>, <ol>, or <menu>). Outside of these containers, assistive technologies cannot convey the list relationship. Wrap <li> elements in the appropriate list container.

lokesh and others added 11 commits May 27, 2026 11:02
Wire the header search modal and the /search filter row through the
data-i18n pattern so their UI strings are translatable instead of
hardcoded English:

- availability option labels/descriptions via a shared
  search/availability_i18n.html, read by availabilityOptionsFromElement
- modal chrome strings (placeholders, aria-labels, status messages) via
  search/search_modal_i18n.html, read by searchModalStringsFromElement
- English fallbacks (AVAILABILITY_OPTIONS, DEFAULT_SEARCH_MODAL_STRINGS)
  kept in search-modal/constants.js; interpolated chip label uses sprintf
- unit tests for the new helpers (searchModalConstants.test.js)

Bundled with the surrounding search-filter feature work on this branch
(OLChip/popover components, header-bar + token CSS, worksearch wiring).
Selecting multiple languages emitted one fq per value, which Solr ANDs —
requiring a work to be in every selected language at once. Combine them
into a single language:("a" OR "b") clause so the filter is additive.
Field name is kept first so editions.fq rewriting (splits on first ':')
still resolves the field.
The legacy header SearchBar.js (and its SearchPage.js page-init shim and
SearchBar.test.js tests) is fully superseded by the new ol-search-bar
LIT component used by nav_head.html. Remove the .search-component
.expanded/.collapsed CSS rules that only the old bar's collapse-on-small-
screen behavior consumed, plus the related width clamps in the desktop /
tablet stylesheets and their legacy.css duplicates.

Drop addModeInputsToForm and SearchModeSelector from SearchUtils.js for
the same reason — both were only called from the deleted SearchBar.js.
PersistentValue / mode are kept; SearchFilterBar.js still imports them.
…/search

URL is the source of truth on /search; sessionStorage mirrors it so the
header search modal opens with the same filters next time and so filters
typed into one search box carry into the next one in the same session.

Server-rendered chips for availability + language (work_search_selected_
facets.html) collapse the has_fulltext / public_scan / print_disabled
trio into a single Availability chip via get_active_availability() and
get_availability_label() — kept in sync with AVAILABILITY_TO_PARAMS in
search-modal/constants.js. The chips render on the first paint, before
the async facet_counts request returns, so they're visible immediately
on page load. Other facet chips (author, subject, …) still wait for
facet_counts since they need it to resolve a display name.

SearchFilterBar.js: on init, if the URL has any filter param, mirror it
to sessionStorage; if the URL has no filter param and sessionStorage has
a non-default value, replace-navigate with the sticky filters applied.
Listen for ol-chip-select so removing the last filter via a chip clears
sessionStorage before navigation (otherwise the sticky branch would
bounce it right back).

Schema fix (works.py): facet_rewrites can emit negated fq values like
'-ebook_access:public' (from public_scan=false / 'borrowable'). Strip
the leading '-' before convert_work_field_to_edition_field() lookup and
re-apply it to the rewritten field; without this the editions.fq was
silently dropped for the Borrowable filter.

page-user.css: use visibility:hidden instead of display:none on the
header search component on /search so the also-flex:1 navigation block
doesn't absorb the freed slot and shift My Books / Browse.
a11y/focus-trap (the primary fix):
- Filter hidden elements out of the ol-dialog focus trap. display:none
  close buttons in SearchModal kept getting included, and .focus() on a
  hidden element is a silent no-op — Tab/Shift+Tab appeared stuck on the
  ESC pill and the "Clear all" button on mobile.
- Make ol-options-popover and ol-select-popover keyboard-reachable inside
  the dialog: tabindex="0" on the host + focus() override that delegates
  to the internal trigger button (otherwise the trigger lives in shadow
  DOM and the outer trap can't see it).
- Walk shadow boundaries when computing the trap's current index, so the
  delegated host is recognized even though deep activeElement is inside.
- Unit tests for focus-utils.js (isFocusable, getFocusableFromSlot,
  findFocusableIndex, getDeepActiveElement) including a regression test
  for the hidden-element bug.

Search modal / filter WIP bundled in:
- SearchModal: drop barcode button from the modal header, replace with a
  close-button shown only on touch devices (paired with the ESC pill on
  hover-capable pointers); add publish-year to result rows; track
  numFound as state.
- nav_head: surface the barcode link directly on touch devices (it was
  previously hidden and only read by the modal).
- SearchFilterBar / constants: extract shared readStoredLanguages helper.
- work_search_selected_facets, header-bar CSS, options-popover template,
  i18n strings, worksearch code: associated cleanup.
Restructure the header search area so the barcode scanner reads as its
own button on touch devices and stays reachable from /search.

- Move the barcode link out of .search-bar-component to be a sibling of
  it under .search-component, so the search-page hide rule (which now
  targets only the pill) leaves the scanner reachable on touch widths.
- Make .search-component a flex container that lays out the pill and
  the barcode side-by-side with a gap.
- Drop the visible 'Search' label on mobile/touch — the trigger renders
  as a compact icon-only pill. Desktop (>=960px) restores the wide
  'Search Q' bar with the label, via overrides in header-bar--desktop.css
  and mirrored in legacy.css.
- Add .search-page bodyclass on work_search.html and update the hide
  rule to scope to .search-bar-component only.
- Drop the vestigial margin-right: -5px on .search-component (a 2021
  nudge that no longer matches the current hamburger sizing).
Adds a mixin that lets shadow-DOM custom elements participate in outer
focus traps as single tabbable leaves:

- Sets tabindex="0" on the host so light-DOM focus-trap queries pick it
  up, and turns on delegatesFocus so .focus() and :focus-visible reach
  the inner control.
- Apply the mixin to OLChip, OlOptionsPopover, and OlSelectPopover so
  they no longer need ad-hoc focus shims.
- OlDialog skips its Tab trap when focus is inside an open nested
  <ol-popover>, letting the popover's own trap drive focus.
- Adds focusableHostMixin.test.js covering the mixin contract.
…ch-modal work

Pulls lessons from the header search-modal PR into the AI-agent docs so the
next agent (or human) starting in this area doesn't re-discover them.

web-components.md
- Registration: the customElements.get() guard idiom — needed because some
  components are imported through both the lit-components bundle and a
  downstream webpack consumer, and a second define() call throws.
- Focus and Shadow DOM: when to reach for FocusableHostMixin, why hidden
  elements have to be filtered out of trap lists (.focus() is a silent
  no-op), the deep-active-element + shadow-boundary walk for finding the
  current trap index, and the stash-and-restore pattern for keeping focus
  alive across Lit repeat() re-renders.
- ARIA on lists: role="radiogroup" on a <ul> strips list semantics; wrap
  in a <div> instead. Plus the whitespace-in-<ul> accesslint gotcha.
- Autofocus on mobile: skip text-input autofocus at the mobile breakpoint
  so the soft keyboard doesn't eat the panel.

design.md
- New Mobile section: 16px input font-size to dodge iOS Safari auto-zoom,
  and the @media (hover: hover) and (pointer: fine) pattern for both
  hover styles and touch-vs-keyboard affordance swaps (e.g. close button
  on touch vs. ESC pill on hover-capable pointers).

i18n.md (new)
- Documents the data-i18n bridge pattern: _i18n.html partial renders a
  JSON dict via $_(), drops it onto a data-* attribute, JS reads and
  merges over an English-default fallback. JS-side ugettext is a
  pass-through, so this is the only working path for translating
  client-rendered UI strings.
The site-wide lit-components bundle (ol-components.js, loaded from
footer.html) already registers every <ol-*> custom element via the
index.js re-exports. Side-effect imports from page bundles (e.g.
SearchModal) were re-running customElements.define(), so each
component file carried a defensive customElements.get() guard, and
OlDialog also had an isServer guard for SSR safety.

Drop the guards and stop re-importing component modules from
SearchModal — the elements are registered before any page-JS handler
runs. Update docs/ai/web-components.md to reflect the single
registration site.
lokesh added 2 commits May 28, 2026 14:52
The header search modal autocomplete and the /search results page were
producing different counts for the same query + availability filter
("red" + Readable Books Only: 2 unfiltered vs 0 filtered) because the
FastAPI /search.json handler silently dropped the availability params.

`PublicQueryOptions` declared only the Solr field `public_scan_b` (the
boolean storage field, `_b` is Solr's dynamicField convention). The
URL-API names `public_scan` and `print_disabled` are different — they
aren't Solr fields; they're keys in `WorkSearchScheme.facet_rewrites`
that get translated to `ebook_access:public` / `ebook_access:printdisabled`
fq clauses. The web.py /search handler explicitly whitelists both names,
but FastAPI's Pydantic model only knew about `public_scan_b`, so
`?public_scan=true` from the modal was discarded as an unknown param
before reaching the facet_rewrites step.

Declare both fields on `PublicQueryOptions` so the FastAPI handler mirrors
the web.py whitelist. After the fix, all four availability values
(all/readable/borrowable/open) produce identical numFound on both
endpoints.
Request the editions subquery from /search.json so the modal opts into
the same edition-level block-join the /search page uses. Without it, the
availability filters (public_scan/print_disabled) only matched the
work-level ebook_access aggregate, so the modal surfaced works the page
hid — e.g. a work whose only query-matching edition is non-readable.
@lokesh
Copy link
Copy Markdown
Collaborator

lokesh commented May 28, 2026

Latest version of the search:

search.mp4

The header autocomplete in the new search modal fired a Solr request at
2 chars and on the bare query "the", a perf/UX regression from the
legacy SearchBar which gated autocomplete at 3+ chars and skipped "the".

Bump MIN_QUERY_LENGTH 2 -> 3 and add a _shouldAutocomplete() predicate
(length + AUTOCOMPLETE_STOPWORDS) used by the fetch-triggering gates.
Navigation to /search stays length-only, so submitting "the" still works.
@Armansiddiqui9
Copy link
Copy Markdown
Contributor Author

Armansiddiqui9 commented May 30, 2026

@lokesh, the new modal looks great overall! Just noticed a couple of things from the previous iteration that seem to have been dropped:

  • Language search input - the OlSelectPopover has a built-in search/filter input that was showing before. Without it, patrons can't easily find their language in the list. Was this intentional?
  • Also, the barcode scanner button - the modal bar had a barcode icon linking to #barcode_scanner_link for patrons on mobile. It seems to have been removed in the latest commits.
  • Language list ordering - the default languages showing are Telugu, Bengali, Gujarati, etc. before English - was this from the /languages.json endpoint sorting by catalogue volume? Might be worth showing the most globally common languages first as defaults.

Happy to re-add these.

@github-actions github-actions Bot added the Needs: Response Issues which require feedback from lead label May 31, 2026
@lokesh
Copy link
Copy Markdown
Collaborator

lokesh commented Jun 2, 2026

@Armansiddiqui9

Language search input - the OlSelectPopover has a built-in search/filter input that was showing before. Without it, patrons can't easily find their language in the list. Was this intentional?

Our <ol-select-popover> shows the input automatically when there are more than 8 items in the list. This number is customizable, though not documented outside of the code. So on production and testing we should always see the input. <ol-select-popover search-threshold="8" ...></ol-select-popover>

https://openlibrary.org/developers/design#select-popover

Also, the barcode scanner button - the modal bar had a barcode icon linking to #barcode_scanner_link for patrons on mobile. It seems to have been removed in the latest commits.

I hid the mobile scanner button on non-mobile devices. We should confirm that it is still visible when viewing the site at mobile resolutions.

Language list ordering - the default languages showing are Telugu, Bengali, Gujarati, etc. before English - was this from the /languages.json endpoint sorting by catalogue volume? Might be worth showing the most globally common languages first as defaults.

This might be a bug, or it might be just the make up of the local dev db and the languages in it, as it should sort by frequency of language in the catalog. Good thing to verify.

One thing about the endpoint, we could hardcode all the languages, but we need to support i18n, so this ends up being non-trivial amount of data, a matrix of possibly hundreds of languages x user's local language. An endpoint gets us just the data the user needs and avoids the drift issue of hardcoded data. I still don't love the flash/delay of loading the languages data when the user opens the dropdown. There is probably a more elegant way to handle this.

@lokesh
Copy link
Copy Markdown
Collaborator

lokesh commented Jun 2, 2026

What Claude thinks of the PR:

Code Review — PR #12797: Search modal UX improvements

Overview

Substantial, well-engineered PR that completes the migration of header search from the legacy jQuery SearchBar to a Lit-based search modal. It:

  • Deletes SearchBar.js, SearchPage.js, and the facet <select> dropdown, replacing them with ol-search-modal (a <dialog>-based modal) wired to a header trigger button.
  • Adds two reusable Lit components (OlDialog, OlOptionsPopover), a FocusableHostMixin, and shadow-DOM-aware focus utilities.
  • Adds Availability + Language filters with sessionStorage stickiness, shared between the modal and the /search filter row.
  • Fixes backend gaps: public_scan/print_disabled whitelisting on both /search.json (FastAPI) and /search (web.py), edition-query negation, and multi-language OR semantics.
  • Strong documentation (i18n.md, web-components.md, design.md) and good JS unit coverage for the focus utils, mixin, and i18n readers.

Code quality is high — thoughtful comments explaining why, consistent token usage, careful a11y, graceful degradation on sessionStorage/network failure. The bulk of my comments are about behavior changes that exceed the modal's scope and test coverage for the riskiest backend logic.

Significant behavior changes worth surfacing to stakeholders

1. Header-level scoped search is removed entirely. The old facet dropdown offered Title, Author, Text (full-text "search inside"), Subject, and Lists scoping (FACET_TO_ENDPOINT/search/inside, /search/authors, etc.). The new modal only ever hits /search.json (works). So full-text search and author/subject/lists scoping are no longer reachable from the header — users must go to advanced search. This is a real product decision, not a bug, but it's larger than "facet removal for redundancy" and the issue lead (@lokesh) should confirm it's intended.

2. Multi-language filtering switched from AND to OR (schemes/works.py via _prepare_solr_query_params in code.py):

if field == "language" and len(non_empty) > 1:
    or_clause = " OR ".join(f'"{val}"' for val in non_empty)
    params.append(("fq", f"{field}:({or_clause})"))

Previously two selected languages produced two fq clauses, which Solr ANDs (a work must be in both). Now they OR. OR is almost certainly the correct UX, but this changes the existing /search sidebar behavior, not just the new modal — worth calling out explicitly, and it's exactly the kind of change that warrants a regression test (see below).

Test coverage gap (main concern)

The JS side is well-covered, but the highest-risk, correctness-critical Python changes have no tests, despite tests/unit/.../test_worksearch.py already testing _prepare_solr_query_params:

  • The multi-language language:("a" OR "b") OR clause.
  • The negation handling in convert_work_query_to_edition_query (-ebook_access:public from public_scan=false) — this directly affects the borrowable filter and the edition block-join.
  • The get_active_availability / availabilityFromParams Python mirror (the JS twin is tested in searchModalConstants.test.js).

These are the parts most likely to silently break a future refactor. I'd ask for at least a couple of _prepare_solr_query_params cases (single lang, multi lang, public_scan=false → negated edition fq) before merge.

Correctness notes

  • AVAILABILITY_TO_PARAMS duplicated in JS and Python (constants.jsworksearch/code.py). Both sides flag the duplication in comments, which is the pragmatic call here, but it's a drift hazard with no test asserting they stay equal. A tiny "params round-trip" test on each side guards it cheaply.
  • PR description is stale re: the barcode button. The body says initSearchModal() reads #barcode_scanner_link and passes barcodeHref into the modal "bar row." The actual implementation renders a standalone .search-by-barcode link in nav_head.html (touch-only via @media (hover: hover) and (pointer: fine)), and there's no barcodeHref prop in SearchModal.js. The implementation is fine — just update the description so reviewers test the right thing.
  • Sticky-filter auto-apply could surprise users (maybeApplyStickyFilters). Arriving at /search?q=foo from another page (or the address bar) with sticky filters in sessionStorage triggers a location.replace that silently applies a language/availability filter set earlier in the session. No redirect loop (the replaced URL has filter params, so the guard short-circuits), and the chip-removal sync prevents the clear-then-bounce-back case — but the implicit re-filtering of an explicit address-bar query is a debatable UX choice. Worth a deliberate confirm.
  • AbortSignal.timeout?.(timeout) (languages.js) — nice defensive optional-chaining; if unsupported, signal: undefined is harmless. Good.
  • Result-list keyboard nav regression (minor): the old SearchBar let ArrowUp/Down move focus through autocomplete results. The new modal relies on the dialog focus trap + Tab; arrow keys in the input do nothing. Acceptable, but a small loss for keyboard users.

Style / conventions

  • Python follows project conventions (double quotes, type hints, @public, sorted imports). req_context and get_language_name imports verified present in worksearch/code.py.
  • JS is clean ES modules, no jQuery in new code, single quotes.
  • CSS uses tokens throughout (no raw hex except inside design/*.html demo inline styles, which are demo-only). New chip-variant tokens are well-organized in colors.css.
  • Heads-up: this adds brand-new Templetor templates (search/availability_i18n.html, search_modal_i18n.html, design/dialog.html, design/options-popover.html). New templates 500 with KeyError until web is restarted — make sure local verification was done after a web restart, and confirm the i18n partials render on both web.py and the FastAPI partials endpoint (the get_request_lang fallback exists precisely because FastAPI doesn't populate web.ctx.lang).

Verdict

Solid, careful work — strong component design, good a11y, and honest comments. Before merge I'd want:

  1. Python tests for the OR clause, the edition-query negation, and ideally the availability params round-trip.
  2. Stakeholder sign-off on the two scope-expanding behavior changes (removal of full-text/scoped header search; AND→OR for multi-language).
  3. PR description corrected re: the barcode button.

None of these are blocking correctness issues I can see in the code — they're about verification and intent. The "Needs: Testing" / "Needs: Response" labels are apt.

lokesh added 2 commits June 2, 2026 00:10
The "Borrowable Only" availability filter (has_fulltext=true +
public_scan=false) rewrites to a negated `-ebook_access:public` clause.
A bare pure-negative query matches nothing inside the block-join
`filters=$editions.fq` local param (no top-level `*:*` fixup), so the
modal and Solr-editions /search returned zero results for borrowable.

Anchor negated editions.fq clauses as `(*:* -field:val)` so they
subtract instead of matching nothing. This is generic — it also covers
any future negated facet_rewrite (e.g. print_disabled=false). Positive
clauses are untouched.

Adds q_to_solr_params tests covering the anchored-negation and
positive-passthrough cases.
Rewrite the availability filter copy around what a patron can *do*, drop
jargon ("Card Catalog", "Open Access"), and indent the two subsets under
"Readable online" so the hierarchy reads at a glance:

  All books (~23M)
  Readable online (~4.6M)
    Free to read now (~1.9M)
    Borrow online (~2.8M)

While relabeling, found the value->param mapping was scrambled: "readable"
filtered to public scans only and "open" filtered to print-disabled. Fixed
so labels match behavior — readable=has_fulltext, open=public_scan — in both
the JS source (constants.js) and its server-side mirror (worksearch/code.py).

Counts verified against production openlibrary.org facets (2026-06-02);
"All books" was ~50M but the filter returns ~23M works. The nested counts
sum to their parent exactly (1.86M public + 2.75M borrowable = 4.62M).

Adds a `nested` flag to ol-options-popover for the indented rendering.
@lokesh lokesh removed the Needs: Response Issues which require feedback from lead label Jun 2, 2026
@lokesh
Copy link
Copy Markdown
Collaborator

lokesh commented Jun 2, 2026

The new search experience is up on testing. Kick the tires!

Screenshot 2026-06-02 at 1 16 31 AM

@tfmorris
Copy link
Copy Markdown
Contributor

tfmorris commented Jun 2, 2026

Where did author search go? That's what I use 90% of the time. It looks the previous 7 search options have been restricted to just title search. Is that intentional?

lokesh and others added 9 commits June 2, 2026 13:51
Replace the generic options-popover with a dedicated OlAvailabilityFilter
that renders availability options with icons and indented nesting, wiring
SearchFilterBar to its change event. Bump the OlSelectPopover filter input
to 16px on mobile to suppress iOS focus-zoom.
Group the search icon, input, and ESC pill into a .search-field wrapper.
On mobile it renders as an inset rounded box with the close (X) sitting
outside it; on desktop it stays a flat row. Also drops the native
type=search clear affordance in favor of the modal's own close control.
Lighten the overlay blur behind the mobile popover tray so it's less
heavy-handed. Applied across all three backdrop states (base,
entering/open, exiting).
deriveAuthors() inspects the top results and, for each author the query
names, renders a "go to the author page" row above the works — with a
circular avatar (author photo over a person-glyph fallback that shows on
404). author_key is added to SEARCH_FIELDS so results can link to authors.

Also update the input placeholder to "Search books and authors…".
Pressing a result navigates the whole window, and the next page can take
a beat to start painting. Flag the chosen row so during that gap it holds
full opacity while the rest dim back, its cover darkens under a spinner
(mirroring the ol-button loading spinner), and the row scales to 0.985 —
matching the header search field's press feedback. Cleared on close, on a
new query, and on pageshow so it never lingers after a bfcache restore.
Nested options use a smaller 13px label and 20px icon so they read as
secondary to the top-level option they sit under.
…ty filter

The lead demo mirrored the real availability popover (its labels, ~23M
counts, and nested rows), but that's the separate ol-availability-filter
component. Swap in neutral single-select examples — a sort menu and a
genre facet — so the demos showcase ol-options-popover on its own terms.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Availability and Language filters to top-level search

5 participants