Skip to content

DRAFT: feat: add mobile-friendly header search modal#12690

Draft
lokesh wants to merge 2 commits into
internetarchive:masterfrom
lokesh:search-mobile
Draft

DRAFT: feat: add mobile-friendly header search modal#12690
lokesh wants to merge 2 commits into
internetarchive:masterfrom
lokesh:search-mobile

Conversation

@lokesh
Copy link
Copy Markdown
Collaborator

@lokesh lokesh commented May 9, 2026

Screenshot 2026-05-08 at 10 31 24 PM

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 -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

Closes #

Technical

Testing

Screenshot

Stakeholders

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 3 issues across 2 rules.

_renderItem(item) {
const isSelected = item.value === this.selected;
return html`
<li class="item ${isSelected ? 'item--selected' : ''}">
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looked into this one! The issue is that role="radiogroup" on the

    strips is a list semantic, which makes the <li> children invalid in the accessibility tree. The fix is to separate the role from the list element:

    _renderPanel() {
        const items = this.items || [];
        const heading = this.heading || (this.label || '').toUpperCase();
        return html`
            <div class="panel">
                <div
                    role="radiogroup"
                    aria-label=${this.label}
                    @keydown=${this._onListKeydown}
                >
                    ${heading ? html`<div class="group-heading" aria-hidden="true">${heading}</div>` : nothing}
                    <ul class="group" id=${this._panelId}>
                        ${repeat(items, it => it.value, it => this._renderItem(it))}
                    </ul>
                </div>
            </div>
        `;
    }
    

    Tested locally - Availability dropdown still works correctly after this change. Happy to open a follow-up PR if that's easier!

return html`
<div class="results">
<h3 class="results-heading">Top results</h3>
<ul class="results-list">
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one is a Lit-specific quirk - whitespace/newlines in HTML template literals create actual text nodes, which accesslint flags as direct text content inside <ul>. Fix is to put ${repeat} on the same line as the opening <ul> tag with no whitespace between them:
<ul class="results-list">${repeat(this._results, r => r.key, r => this._renderResult(r))}</ul>
Tested locally, results still render correctly!

? `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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same lit whitespace issue - the newline before

  • in _renderResult() creates an orphaned text node. Fix:

    return html`<li>
            <a class="result" href=${work.key}>
                ...
            </a>
        </li>`;
    

    Tested this also alongside comment 2 fix; both work together correctly!

  • Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants