Add OlDrawer component for hamburger menu by lokesh · Pull Request #12326 · internetarchive/openlibrary

lokesh · 2026-04-09T05:59:24Z

Part of #13240 — Workstream 1, Foundation → ol-* components.

Replaces the hamburger menu's <details> dropdown with an ol-drawer web component that slides in from the viewport edge. [refactor]

Technical

Built on native <dialog>.showModal() rather than a hand-rolled overlay, matching ol-dialog and the overlay table in docs/ai/web-components.md. The browser then provides:

The top layer — renders above all page content with no z-index, and no ancestor transform/contain/isolation can trap it. This is the motivation for the PR, solved by the platform.
Background inerting, which makes the aria-modal="true" claim true. The old JS trap caught Tab, but a screen reader could still browse the page behind.
Escape, via the cancel event.

That removed the hand-rolled backdrop, z-index, document-level key listener, and animation state machine.

Also here: Sign Up now appears in the header below 480px, where the auth links were hidden entirely and the hamburger was a logged-out patron's only route to an account. And Browse destinations move to one shared table (openlibrary/plugins/openlibrary/nav.py) used by both the drawer and the Browse popover, which had drifted — the popover was missing Advanced Search.

Testing

Open the hamburger menu on desktop, tablet, and mobile viewports.
Confirm it paints above the header, and that Escape, backdrop click, and swipe all dismiss it with a smooth slide out.
Logged out, confirm Sign Up is visible in the header on a phone-width viewport.
Tab through the open drawer — focus should cycle inside it and return to the trigger on close.
/developers/design#drawer exercises both placements and a custom width.

Screenshot

drawer.mp4

Stakeholders

@cdrini

Remove lightDismiss prop (always allow backdrop dismiss), tune animation durations, improve swipe-to-close backdrop opacity calculation, and move drawer to document.body in demo page. Simplify header_dropdown template by removing hamburger-specific logic now handled by the drawer. Add aria-expanded toggling and auto-close on link/button click to nav_head. Fix drawer menu CSS: use design tokens, scope :hover behind media query, fix login link underlines via :link/:visited specificity, and add last-child border removal.

Replace the loop-based hamburgerLinks rendering with hardcoded markup. Remove unused loginLinks, contributeLinks, and resourceLinks arrays.

for more information, see https://pre-commit.ci

Copilot

Pull request overview

This PR replaces the header hamburger menu’s <details> dropdown with a new ol-drawer Lit web component that renders as a root-level overlay (solving stacking-context/z-index issues) and adds a design pattern library page documenting the drawer.

Changes:

Added OlDrawer Lit component (<ol-drawer>) with backdrop, animation, focus trapping, scroll lock, and swipe-to-dismiss.
Reworked the header hamburger markup/CSS to use the new drawer + new light-DOM menu styles.
Added design system documentation/demo for the drawer and bumped the globals npm dependency.

Reviewed changes

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

Show a summary per file

File	Description
static/css/legacy.css	Extends existing header icon sizing to the new hamburger markup; removes unused drawer/mask positioning rules.
static/css/components/header-bar.css	Removes `<details>` hamburger drawer styles and adds new `.hamburger-trigger` + `.drawer-menu*` light-DOM styling.
static/css/components/header-bar--desktop.css	Removes old drawer/mask sizing rules; updates icon sizing selectors.
package.json	Bumps `globals` dependency version.
package-lock.json	Updates lockfile for `globals` bump and related metadata normalization.
openlibrary/templates/site/head.html	Hides `<ol-drawer>` until defined to prevent flash of unstyled content.
openlibrary/templates/lib/nav_head.html	Replaces hamburger `<details>` dropdown with `<ol-drawer>` + inline wiring script.
openlibrary/templates/lib/header_dropdown.html	Removes hamburger-specific rendering/mask/drawer logic now that hamburger is handled separately.
openlibrary/templates/design/drawer.html	Adds drawer component documentation + live demos.
openlibrary/templates/design.html	Adds “Drawer” section to the design system index and renders the new drawer template.
openlibrary/plugins/openlibrary/js/index.js	Removes old hamburger mask click handler tied to the `<details>` implementation.
openlibrary/i18n/messages.pot	Updates message catalog references/strings for the new drawer docs and hamburger markup.
openlibrary/components/lit/OlDrawer.js	Introduces the new `<ol-drawer>` Lit component implementation.
openlibrary/components/lit/index.js	Exports/registers `OlDrawer` in the Lit components entrypoint.

Copilot · 2026-04-09T06:12:34Z

+    var trigger = document.querySelector('.hamburger-trigger');
+    var drawer = document.getElementById('hamburger-drawer');
+    if (trigger && drawer) {
+      trigger.addEventListener('click', function() {
+        drawer.open = !drawer.open;
+      });


ol-components.js (which defines ol-drawer) is loaded in the footer, but this click handler toggles the drawer via the open property immediately. If a user clicks before the custom element upgrades, this will set an expando property on the undefined element and the value will be lost on upgrade (menu appears to not open until a second click). Prefer toggling the open attribute (toggleAttribute/setAttribute/removeAttribute) or waiting for customElements.whenDefined('ol-drawer') before wiring the handler.

Copilot · 2026-04-09T06:12:35Z

+    disconnectedCallback() {
+        super.disconnectedCallback();
+        this._removeListeners();
+        this._unlockBodyScroll();
+    }


disconnectedCallback() unconditionally calls _unlockBodyScroll(), which will call window.scrollTo(0, this._savedScrollY) even if the drawer was never opened/locked. If the element is removed/reparented while closed, this can unexpectedly scroll the page to the top. Guard this so unlock/scroll restoration only happens if a scroll lock is currently active.

mekarpeles · 2026-04-09T06:17:24Z

Previously, the drawer panel itself received focus on open. Pressing Tab then hit the start focus-sentinel, which incorrectly wrapped to the last focusable element. Now the first focusable slotted element is focused instead, so Tab moves forward naturally.

Co-authored-by: Copilot <[email protected]>

for more information, see https://pre-commit.ci

mekarpeles · 2026-04-13T18:48:16Z

Thank you @lokesh! Having an issue for this PR would help with the review process. Assigning @cdrini for now

# Conflicts: # openlibrary/plugins/openlibrary/js/index.js

for more information, see https://pre-commit.ci

Previously _get_pr_drift only returned head_sha/drift/merged, so stale titles ("PR #12326") and missing author/assignee persisted indefinitely. Now on a cache miss, _get_drift_info also writes fresh metadata back to the state file, making "Refresh Status" actually update what you see.

- Staged UX: Add/Fetch Latest/Enable/Disable/Remove all stage changes; only "Perform Deploy" applies them and triggers Jenkins - Memcache drift cache (5-min TTL) to avoid hitting GitHub API on every page load; "Refresh Status" evicts the cache explicitly - On cache miss, refreshes title/author/assignee from GitHub and writes back to state file so stale data ("PR #12326") self-corrects - GitHub API token support via github_api_token in openlibrary.yml - PR author + assignee with avatars (no extra API calls) - Columns: Enabled (was Active), Pinned Commit with pending arrow, Branch HEAD, Drift, Added (with added_by tooltip) - Jenkins job URL in deploy banner; rel="noopener noreferrer" on links - Backward-compat: old bare-array state file format still loads cleanly

Resolve conflicts and update OlDrawer to adopted component conventions: - Use shared scroll-lock.js and focus-utils.js (keydown Tab-trap like OlDialog) instead of hand-rolled scroll lock and focus sentinels - Design tokens without fallbacks; rename --size to --ol-drawer-width, add --ol-drawer-* cssprops; use --z-index-modal and semantic colors - Guard customElements.define; regenerate custom-elements manifest - Move ol-drawer pre-hydration FOUC rule into ol-components.css - Drop dead .app-drawer/.mask-menu CSS; instant hover on drawer login links - Regenerate messages.pot

Pressing a menu link no longer closes the drawer. Instead the pressed row holds full opacity while the rest of the menu dims and a spinner shows at its trailing edge — mirroring the search modal's result loading treatment — so the drawer stays open until the new page takes over. Modified/new-tab clicks are left untreated, and the state is cleared on bfcache restore.

Match the <ol-button> press affordance: menu links and the logout button scale to 0.97 on :active with an 0.08s transform transition. Hover color changes stay instant per docs/ai/design.md.

A fast toggle could set open=false after _show() scheduled its updateComplete callback but before it ran. The callback then set _animState back to 'entering', re-opening the drawer while open was false — leaving it stuck open and scroll-locked, since Escape and backdrop dismissal both gate on this.open. Bail out of the open sequence if open is no longer true.

Pinning <body> to lock background scroll removes the document scrollbar; on platforms with classic (space-consuming) scrollbars, reclaiming that width shifted the page. Measure the scrollbar width before locking and hold it as body padding (border-box) so the layout stays put. No-op on overlay-scrollbar platforms where the width is 0.

# Conflicts: # openlibrary/i18n/messages.pot # openlibrary/templates/lib/header_dropdown.html # openlibrary/templates/lib/nav_head.html # static/css/components/header-bar.css

accesslint

Found 1 issue across 1 rule.

Reviewed by AccessLint, which checks every pull request for accessibility issues.

accesslint · 2026-08-21T22:36:57Z

 </header>
+
+<ol-drawer id="hamburger-drawer" label="$_('Menu')">
+  <ul class="drawer-menu hamburger-drawer-menu">


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

<ul> and <ol> must only contain <li>, <script>, <template>, or <style> 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, apply styles to <li> elements directly and remove the wrapper (e.g., change <ul>``<div>item</div>``</ul> to <ul>``<li>item</li>``</ul>).

for more information, see https://pre-commit.ci

# Conflicts: # openlibrary/i18n/messages.pot # openlibrary/templates/lib/browse_popover.html

# Conflicts: # static/css/ol-components.css

The merge kept the drawer's description string as a third positional arg, which landed on `group` and collided with the keyword.

Both the drawer's horizontal swipe and the popover tray's vertical one armed on displacement alone, so scrolling the panel with a little drift in the dismiss direction started a drag the browser had already claimed for its own scroll — every preventDefault() on those non-cancelable touchmoves logged an intervention. The drawer now locks to whichever axis dominates the gesture, both components bail out when the touch is no longer cancelable, and preventDefault() is guarded either way.

_openDrawer() bailed on an already-open dialog, which is exactly the state a reopen mid-exit lands in: showModal() is still in effect, so the early return skipped the _cycle bump and the queued close callback ran anyway. `open` stayed true against a closed dialog, and it took two more taps to get the drawer back — an accidental double-tap on the hamburger reads as a broken menu. Bump the cycle first so the pending close cancels, and re-run the entry. showModal() and the _previouslyFocusedElement capture are skipped on a reopen: the first throws on an open dialog, and the second would record an element inside the drawer, losing the real restore target.

--dark-beige is defined nowhere in the repo, so the border-top shorthand was invalid at computed-value time and the style fell back to none. Rather than pick a replacement token, remove the rule: the subsections read fine on spacing alone.

navIcons[link.track] was a hard subscript on a dict only browse_popover maintains, while nav.py told you adding a destination is one edit there. Following that advice raised KeyError in a template nav_head renders on every page. Fall back to a generic icon, and say so in the docstring.

The deleted header_dropdown.html gated both merge-count badges on is_super_librarian_or_higher(); folding the markup into nav_head reused its is_librarian_or_higher() and quietly showed the counts to every librarian. The /merges link itself stays librarian-gated as before.

accesslint

Found 1 issue across 1 rule.

Reviewed by AccessLint, which checks every pull request for accessibility issues.

accesslint · 2026-09-03T18:00:40Z

 </header>
+
+<ol-drawer id="hamburger-drawer" label="$_('Menu')">
+  <ul class="drawer-menu hamburger-drawer-menu">


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

<ul> and <ol> must only contain <li>, <script>, <template>, or <style> 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, apply styles to <li> elements directly and remove the wrapper (e.g., change <ul>``<div>item</div>``</ul> to <ul>``<li>item</li>``</ul>).

The hamburger's loginLinks was its only caller, and the drawer rewrite replaced that with drawer-menu__login-links in nav_head. The .login-links* rules went with it, so the branch survived only to render unstyled markup nothing reaches.

cdrini

Niiiice lgtm! A few code cleanup opportunities, but nothing major.

A11y feedback:

Tabbing through the items in the sidebar doesn't auto-scroll the item into view like it did before.

cdrini · 2026-05-22T15:31:53Z

+        // Touch drag state (horizontal swipe-to-dismiss)
+        this._touchStartX = 0;
+        this._touchStartTime = 0;
+        this._isDragging = false;
+        this._lastDragX = 0;
+
+        this._onKeydownGlobal = this._onKeydownGlobal.bind(this);
+        this._onTouchStart = this._onTouchStart.bind(this);
+        this._onTouchMove = this._onTouchMove.bind(this);
+        this._onTouchEnd = this._onTouchEnd.bind(this);
+    }


Lots of very similar code to popover ; not sure if there's a DRY opportunity here.

cdrini · 2026-09-03T17:27:47Z

        right: document.body.style.right,
        width: document.body.style.width,
        overflow: document.body.style.overflow,
+        paddingRight: document.body.style.paddingRight,


Should we record all the padding since we support a placement option?

cdrini · 2026-09-03T17:27:55Z

+    // (space-consuming) scrollbars, reclaiming its width would shift the page.
+    // Measure it now and hold that width as padding so nothing moves. Overlay
+    // scrollbars (default on macOS/iOS) measure 0, so this is a no-op there.
+    const scrollbarWidth = window.innerWidth - document.documentElement.clientWidth;


Looooove the attention to detail!

cdrini · 2026-09-03T17:57:25Z

+    }
+
+    /** @returns {HTMLElement} */
+    get _scrim() {


I wonder if we can use ::backdrop instead of a new element? I see we're using this to one-to-one control the opacity I think; could we use a css variable to perhaps do that?

Co-authored-by: Drini Cami <[email protected]>

The API tables on /developers/design print the JSDoc type verbatim, so {String} rendered a useless "String" while the real values sat in prose that nothing checks. Converting the closed sets means editors autocomplete them, tsc --checkJs catches typos, and the table lists them for free. Follows cdrini's review suggestion on ol-drawer's placement. Popover and tooltip placement stay {String} — they're a {side}-{align} grammar, not a short list worth enumerating. Also writes the rule down in docs/ai/web-components.md so new components keep doing it.

The focus trap preventDefault()s every Tab and moves focus itself, which also cancels the browser's native scroll-into-view. In a drawer taller than its own scroller, Tab could land on a control below the fold — focus invisible to the user, a WCAG 2.4.11 failure. focus({preventScroll:true}) stays, because letting focus() scroll would walk every ancestor and push the panel itself off-screen; scrollIntoView with block:'nearest' moves only the panel's scroller, and only when the target isn't already visible. Consumers with a sticky header or footer can inset the resting position through the new --ol-drawer-scroll-padding custom property, since the panel is shadow DOM and scroll-padding is otherwise out of reach.

lokesh · 2026-09-03T18:45:18Z

@cdrini Great feedback as always.

Fixed: When tabbing through items in ol-drawer the viewport now scrolls to keep focused items in view.
Updated our AI docs so that when generating JS docblocks they show the values in the definition: @prop {'start' | 'end'} placement - Which edge

The wiring lived as an inline <script> in nav_head.html, so it shipped on every page render, escaped lint and bundling, and had to stay ES5-flavoured. It now lives in js/hamburger-drawer.js, dynamically imported from index.js only when the trigger and drawer are both present.

.hamburger-trigger reset the button's background and border but never set a color, and a <button> doesn't inherit one — the UA's ButtonText wins, and iOS Safari resolves that to its system blue, which the currentColor sprite picks up. Pin it to --color-text, the same token the Browse trigger uses. The magnifier and barcode move to --color-text too, so the three header icons read as a set. The magnifier had #8a7f71 baked into search-icon.svg and could not take a token at all, so it becomes a sprite icon; the file stays for the legacy search box in searchbox.css.

accesslint

Found 1 issue across 1 rule.

Reviewed by AccessLint, which checks every pull request for accessibility issues.

accesslint · 2026-09-03T19:08:50Z

 </header>
+
+<ol-drawer id="hamburger-drawer" label="$_('Menu')">
+  <ul class="drawer-menu hamburger-drawer-menu">


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

<ul> and <ol> must only contain <li>, <script>, <template>, or <style> 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, apply styles to <li> elements directly and remove the wrapper (e.g., change <ul>``<div>item</div>``</ul> to <ul>``<li>item</li>``</ul>).

Git rename detection carries master's hamburger-drawer wiring (#12326) onto the renamed main.js. Strip the webpackChunkName magic comment from the merged-in import: under Vite, chunks are named after their imported file, so the chunk is emitted as hamburger-drawer.<hash>.js naturally.

lokesh and others added 5 commits April 8, 2026 21:58

feat: add OlDrawer Lit component and update header bar navigation

f66a1d1

style: improve readability of hamburgerLinks list formatting

9bf1616

refactor: hardcode drawer menu links for readability

81d5324

Replace the loop-based hamburgerLinks rendering with hardcoded markup. Remove unused loginLinks, contributeLinks, and resourceLinks arrays.

[pre-commit.ci] auto fixes from pre-commit.com hooks

a8cbd49

for more information, see https://pre-commit.ci

mekarpeles requested a review from Copilot April 9, 2026 06:05

Copilot started reviewing on behalf of mekarpeles April 9, 2026 06:06 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

mekarpeles added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Apr 9, 2026

github-actions Bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Apr 9, 2026

lokesh and others added 4 commits April 8, 2026 23:54

Update openlibrary/templates/lib/nav_head.html

f65ad93

Co-authored-by: Copilot <[email protected]>

Update openlibrary/components/lit/OlDrawer.js

4b786f8

Co-authored-by: Copilot <[email protected]>

Merge branch 'master' into drawer-component

e454b0a

[pre-commit.ci] auto fixes from pre-commit.com hooks

fcc949d

for more information, see https://pre-commit.ci

mekarpeles assigned cdrini Apr 13, 2026

lokesh mentioned this pull request Apr 13, 2026

Systematize z-index usage across the site #12363

Open

14 tasks

lokesh and others added 2 commits April 14, 2026 15:32

Merge remote-tracking branch 'upstream/master' into drawer-component

1875e81

# Conflicts: # openlibrary/plugins/openlibrary/js/index.js

[pre-commit.ci] auto fixes from pre-commit.com hooks

148a20b

for more information, see https://pre-commit.ci

lokesh added 6 commits July 6, 2026 12:53

Add press-scale feedback to drawer menu items

5007205

Match the <ol-button> press affordance: menu links and the logout button scale to 0.97 on :active with an 0.08s transform transition. Hover color changes stay instant per docs/ai/design.md.

Merge remote-tracking branch 'upstream/master' into drawer-component

242b923

Merge remote-tracking branch 'upstream/master' into drawer-component

60ebc48

# Conflicts: # openlibrary/i18n/messages.pot # openlibrary/templates/lib/header_dropdown.html # openlibrary/templates/lib/nav_head.html # static/css/components/header-bar.css

accesslint Bot reviewed Aug 21, 2026

View reviewed changes

pre-commit-ci Bot and others added 14 commits August 21, 2026 22:37

[pre-commit.ci] auto fixes from pre-commit.com hooks

abadcf3

for more information, see https://pre-commit.ci

Merge remote-tracking branch 'upstream/master' into drawer-component

0e11134

Merge remote-tracking branch 'upstream/master' into drawer-component

8f05bd3

# Conflicts: # openlibrary/i18n/messages.pot # openlibrary/templates/lib/browse_popover.html

Merge remote-tracking branch 'upstream/master' into drawer-component

7a91f82

Merge remote-tracking branch 'upstream/master' into drawer-component

63f6cb9

Merge remote-tracking branch 'upstream/master' into drawer-component

a9603dc

Merge remote-tracking branch 'upstream/master' into drawer-component

0ec971a

Merge remote-tracking branch 'upstream/master' into drawer-component

23476fe

# Conflicts: # static/css/ol-components.css

Fix drawer registry row after upstream dropped Component.description

51af52f

The merge kept the drawer's description string as a third positional arg, which landed on `group` and collided with the keyword.

accesslint Bot reviewed Sep 3, 2026

View reviewed changes

cdrini approved these changes Sep 3, 2026

View reviewed changes

lokesh and others added 4 commits September 3, 2026 11:24

Merge remote-tracking branch 'upstream/master' into drawer-component

a1dee1a

Update openlibrary/components/lit/OlDrawer.js

5945154

Co-authored-by: Drini Cami <[email protected]>

lokesh added 3 commits September 3, 2026 11:52

Merge remote-tracking branch 'upstream/master' into drawer-component

8d591ca

accesslint Bot reviewed Sep 3, 2026

View reviewed changes

lokesh merged commit 3c709c5 into internetarchive:master Sep 3, 2026
5 checks passed

Uh oh!

Uh oh!

Conversation

lokesh commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Technical

Testing

Screenshot

Stakeholders

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

mekarpeles commented Apr 9, 2026 • edited by lokesh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Possible improvements for this PR

Uh oh!

mekarpeles commented Apr 13, 2026

Uh oh!

accesslint Bot left a comment

Choose a reason for hiding this comment

Uh oh!

accesslint Bot Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

accesslint Bot left a comment

Choose a reason for hiding this comment

Uh oh!

accesslint Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

cdrini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cdrini May 22, 2026

Choose a reason for hiding this comment

Uh oh!

cdrini Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

cdrini Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cdrini Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lokesh commented Sep 3, 2026

Uh oh!

accesslint Bot left a comment

Choose a reason for hiding this comment

Uh oh!

accesslint Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lokesh commented Apr 9, 2026 •

edited

Loading

mekarpeles commented Apr 9, 2026 •

edited by lokesh

Loading