feat: Consolidate sidebar buy options into unified Buy button dropdown - #13113

PR #13113 — Rebase + verification (Ada, on behalf of Mek)

Rebased this branch onto current master (it was ~6 weeks / 272 commits behind) and then ran a full verification pass in Docker + Playwright. Thanks for this — the BEM naming, the async-partial handling and the removal of the old .buy-options-table styles are all clean, and the dropdown genuinely works. Findings below, with the evidence for each.

Rebase

9d3363e2f → 441dd7096, force-pushed with --force-with-lease. GitHub's mergeable field said MERGEABLE, but a real rebase produced 25 conflict hunks — all in the generated openlibrary/i18n/messages.pot, and all #: source-reference churn from master drift. I took master's side and regenerated with scripts/i18n-messages extract rather than hand-merging line refs, in a separate clearly-labelled commit so it's easy to tell from your work. Nothing required guessing at your intent.

Two commits dropped, both verified lossless: the two [pre-commit.ci] commits contained only .pot regeneration (empty against current master), and chore(ci): increase page-admin.css bundlesize maxSize to 28KB is already upstream via 257e08c77.

CORRECTNESS

🔴 BLOCKING — every edition page 500s. openlibrary/macros/databarWork.html:8:

$ buy_is_primary = editions_page and not _has_digital and bool(worldcat_url)

worldcat_url doesn't exist in this template's scope — the macro is passed worldcat_links (plural, a pre-rendered blob). Repo-wide, worldcat_url exists only as the Edition.get_worldcat_url() method in plugins/upstream/models.py:178. This line is top-level in the macro, so it runs on every render:

GET /books/OL1M → HTTP 500
NameError: name 'worldcat_url' is not defined
  File "/openlibrary/openlibrary/macros/databarWork.html", line 12, in __template__

Isolated to this single line: neutralizing just that expression makes /books/OL1M, /books/OL4M and /books/OL24218089M all return 200.

Suggested fix — delete lines 5-8 entirely (4 lines, zero rendered-output change). buy_is_primary is never read anywhere — no CSS, template, JS or Python reference. _has_digital is read only on line 8. availability is independently re-assigned at line 79 before its only other use at line 80. So the whole block is dead code whose only effect is the crash. I'd suggest not trying to implement the intended blue/grey primary-button behaviour here — see the scope note at the bottom.

🔴 BLOCKING — the WorldCat link is removed from book pages. The PR description says this "consolidates the sidebar WorldCat/library locate links and purchase/affiliate options into the unified Buy split-popover button", but WorldCat isn't in the new popover — it only contains $:affiliate_links.

So worldcat_link is still computed and still passed into the macro, but is received and silently discarded. Confirmed in the browser, not just by grep — a Playwright probe of a rendered book page reports sidebarHasWorldcat: false, with the only remaining worldcat.org href being the one in the identifiers table, not the sidebar CTA.

🔴 BLOCKING — javascript_tests now fails on bundlesize (it passed before the rebase):

✖ static/build/css/page-book.css   17.03KB > 17KB
✖ static/build/css/page-edit.css   27.1KB  > 27KB
✖ static/build/css/page-plain.css  27.18KB > 27KB

Root cause: components/read-panel.css and components/work.css are imported by both page-book.css and legacy.css — and legacy.css is imported by page-form, page-edit, page-admin, page-user, page-list-edit and page-plain. So the new buy-dropdown CSS is duplicated into 7 bundles, 6 of which never render a buy button. Master's own CSS growth over the last 6 weeks consumed the remaining headroom, which is why this only went red on rebase.

docs/ai/css.md ("Bundle Size Limits") sanctions two fixes — remove unused styles, or move the styles into a <name>--js.css entrypoint loaded via JS, which has a higher threshold. Raising the caps isn't among them, so scoping these rules out of the legacy.css path looks like the right call rather than another cap bump.

Verified working (so you don't need to re-check these):

• The <span> → <div> change on the partial root is safe. js/index.js:608 selects by class (.affiliate-links-section), so it's tag-agnostic; and although js/affiliate-links.js:69-73 parses the partial by assigning it into a created <span>'s innerHTML, the <div> survives intact — verified shape after parse: rootTag: "div", .buy-options__list and .buy-options__disclaimer both present, options > 0, retry affordance 0, live partial fetch 200.
• No caller of macros.LoanStatus still passes post= — all four checked (SearchResultsWork.html:253, databarWork.html:39, books/custom_carousel_card.html:87, books/edition-sort.html:93). The signature change is safe.
• The dropdown opens with Enter and Space, Escape closes it and returns focus to the trigger, Tab walks the options in order, and every option link has target="_blank" + rel="noopener noreferrer".
• Mobile tray fills the viewport as intended (366px inner at a 390px viewport).

Non-blocking — orphaned CSS this PR leaves behind. .desktop-vendor and .mobile-vendor rules remain in static/css/components/read-panel.css with no markup anywhere in the repo now. .cta-section-title.world-cat-link is likewise dead, since its only markup is macros/WorldcatLink.html, whose output is the worldcat_link that's no longer rendered. (.cta-section / .btn-notice are still legitimately used by the nine book_providers/*_download_options.html templates — those should stay.)

Non-blocking — declared width isn't the rendered width. .buy-options computes to 240px but renders at 228px (panelRect: 228, innerComputedWidth: "240px", box-sizing: border-box). I didn't isolate the cause — possibly the popover panel's own collision/shift logic. Flagging with numbers rather than asserting a defect.

DESIGN SYSTEM

Checked rule-by-rule against docs/ai/design.md and docs/ai/css.md.

❌ Hover transitions (design.md "Hover state changes are instant"). The doc's "Bad" example is almost exactly this code:

.buy-options .buy-option { transition: background-color 0.2s; }
.buy-options .buy-option:hover { background-color: var(--lightest-grey); }

Transitions belong on :active, enter/exit and loading states — not hover colour changes.

❌ Deprecated colour aliases (design.md "Deprecated aliases": never use these in new code). This is largely bad luck — #13102 landed the semantic token layer after you wrote this. Every alias used has an exact-value semantic equivalent, so it's a zero-visual-change rename:

❌ Hover direction on a dark fill (design.md: light fills darken; saturated/dark fills lighten). .cta-btn--buy starts dark (--grey-464646 = --neutral-700) and hover darkens it further to --dark-grey (--neutral-800) — the opposite of the rule. The doc's prescribed tool is filter: brightness(1.1), which also carries the border and any inset highlight in one declaration.

❌ Hover not gated to hover-capable pointers (design.md "Gate hover styles"). Neither .buy-option:hover nor .cta-btn--buy:hover is wrapped in @media (hover: hover) and (pointer: fine), so on touch the hover state sticks after tap.

❌ Prices need font-variant-numeric: tabular-nums (design.md "Tabular numbers": explicitly names prices). .buy-option__price is right-aligned in a space-between flex row, so proportional digits make the price column visibly ragged across rows.

❌ Top margin (css.md: use only bottom margins for vertical spacing, never top margins). .buy-popover { margin-top: var(--spacing-stack-xs); }.

❌ Hardcoded control height. .cta-btn--buy { height: 37px; } with no box-sizing. static/css/tokens/control-heights.css exists precisely so single-line controls line up exactly — --control-height-large is 38px. A one-off 37px is the misalignment that file exists to prevent.

⚠️ Minor: the 768px media query has no comment naming --width-breakpoint-tablet (breakpoints.css asks for one, since custom properties can't be used in @media); no :active press feedback (transform: scale(0.97)); the arrow uses a PNG recoloured with filter: brightness(0) invert(1) plus magic background-size: 45px, while the cart in the same button is an inline SVG and the repo has an icon sprite system; and .buy-options .buy-options__disclaimer nests an element under its own block unnecessarily.

✅ N/A, checked: text-wrap: balance (this PR adds no headings — it removes one), and the font-size: 16px iOS floor (no text input in the dropdown). Stylelint passes.

ACCESSIBILITY

• Cart SVG isn't hidden from assistive tech. CART SVG ATTRS: {"ariaHidden":null,"focusable":null,"role":null}. It's decorative next to a real Buy label — add aria-hidden="true".
• List semantics lost. The old markup was <ul> / <li>; the new one is <div class="buy-option"> wrappers. Screen readers no longer announce "list, N items", which is a real navigational cue for a set of purchase options. Worth considering <ul role="list"> + <li>, which costs nothing visually with the current flex styling.
• First Tab lands on the disclaimer, not the first store. DOM order is correct (disclaimer AFTER list (expected)), and once moving, the options tab in the right order and wrap — the cycle just starts on the last element: ["small commission", "Better World Books", "Amazon", "Bookshop.org", "small commission"]. Minor, but it's the first thing a keyboard user hits.
• Not your bug — flagging so it isn't misattributed: axe reports a [serious] aria-hidden-focus violation on <span class="focus-sentinel" tabindex="0" aria-hidden="true"> when the popover is open. This reproduces identically on the unmodified header browse-popover, so it's a pre-existing OlPopover issue affecting every popover, not something this PR introduced. Same for the absent focus ring on options — a plain sidebar link computes identically (outlineStyle: "none", boxShadow: "none"), so that's site-wide, not yours.
• axe on the closed book page: no violations.
• Arrow-key navigation doesn't move between options — but OlPopover exposes aria-haspopup="dialog", and the APG dialog pattern doesn't require arrow keys. Not a defect.

TESTS

GAP — no tests accompany the change. The interesting part is why the crash above survived a green CI run: openlibrary/tests/test_templates.py::test_valid_template covers every .html under templates/ and macros/, but it only parses them (catching SyntaxError/TokenError) — it never renders one. $ x = ... worldcat_url parses perfectly; the undefined name only fails when the expression is evaluated. So there is currently no CI check anywhere that would catch a NameError in a macro. A single test that actually renders databarWork against an edition would have caught this, and would be worth more here than tests for the dropdown itself.

(Credit where due: test_noopener_noreferrer in that same file is why the target="_blank" links below are already safe — that one is enforced.) I wrote Playwright coverage locally to verify the dropdown (async population, keyboard operation, Escape/focus return, mobile width, WorldCat absence) — happy to hand it over if useful, but I haven't pushed it since these are your call.

SECURITY

No findings. All option links carry rel="noopener noreferrer" alongside target="_blank" (verified across every rendered link), and the | safe in AffiliateLinks.html.jinja is applied to macro-assembled output whose own interpolations are escaped.

OVERALL

FOUND ISSUES — 3 blocking, all fixable and none of them design problems with the approach itself:

1. worldcat_url NameError — every edition page 500s (delete the dead block at databarWork.html:5-8).
2. WorldCat affordance removed with no replacement, contrary to the PR description.
3. javascript_tests red on bundlesize across 3 CSS bundles.

Also needs a maintainer decision — scope. #12867 is Mek's own open PR (branch 12826/consolidate-sidebar-cta) implementing the full sidebar consolidation; the issue is #12826. This PR is a slice of that, which explains both the orphaned worldcat_url (it depended on the LoanStatus/view.html half that computes and passes it) and the never-read buy_is_primary (the unimplemented blue/grey state matrix). WorldCat was meant to move into the Borrow dropdown, which lives in that other half. Two consequences worth deciding on before merge:

• Either restore $:worldcat_links inside the Buy popover for this slice, or hold this until the ReadButton/LoanStatus half lands so WorldCat has its new home. As-is it's a user-facing regression.
• Closes #12867 points at a PR rather than an issue, and would overclaim a much larger scope. Part of #12826 is probably what's intended.

One last note: the sticky left column (work.css) is a separate UX change bundled in here — reasonable on its own, but worth calling out so it gets reviewed deliberately rather than as a side effect of the buy-button work.

Verified in Docker on the rebased branch: pre-commit and pre-push suites green, edition pages rendered, dropdown exercised in Playwright (Chromium, desktop 1280×800 and Pixel 5) with axe-core.

Pushed a fix for BLOCKER 1 (bf027cd7c) — Mek asked me to apply this one directly rather than leave it for you, so the branch is no longer 500ing.

Removed the dead block at databarWork.html:5-8 rather than repairing the worldcat_url expression, because the whole thing was unreachable code: buy_is_primary is never read anywhere in the repo, _has_digital was read only on the crashing line, and availability is independently re-assigned further down before its only other use. So rendered output is byte-for-byte unchanged — /books/OL1M, OL2M, OL4M and OL24218089M all return 200 now, verified on the exact pushed commit.

I deliberately did not try to implement the blue/grey primary-button behaviour that buy_is_primary was meant to feed. That state matrix depends on the LoanStatus/view.html half of #12867 computing and passing worldcat_url down, which isn't in this PR — building it here would have been guesswork about your intent. It belongs with the slice that carries ReadButton/LocateButton.

The other two blockers from my review above are untouched and still need decisions rather than code from me:

• WorldCat affordance — still removed with no replacement. Needs a call on whether to restore $:worldcat_links inside the Buy popover for this slice, or hold the PR until the ReadButton/LoanStatus half lands so it has its new home.
• bundlesize — still red on page-book / page-edit / page-plain. docs/ai/css.md sanctions removing unused styles or moving the rules into a <name>--js.css entrypoint; raising the caps isn't among its options, so this is likely a matter of scoping the new rules off the legacy.css path (which is what pulls them into six bundles that have no buy button).

Also worth knowing, and not your fault: this class of bug is invisible to CI. openlibrary/tests/test_templates.py::test_valid_template covers every macro but only parses them — it never renders one, so a NameError on an undefined name passes python_tests by construction. That's a gap on our side; I've flagged it separately.

Pushed two more fixes (e7c18ac9b) — the remaining blockers from my review above are now closed, and the dropdown is verified working end to end with Playwright rather than by inspection.

WorldCat is reachable again, inside the Buy popover

This restores what this PR's own description promised ("consolidates the sidebar WorldCat/library locate links and purchase/affiliate options into the unified Buy split-popover button") — previously both $:worldcat_links render sites were removed while worldcat_link was still computed and passed into the macro, so it was received and silently discarded.

WorldcatLink.html now emits a flat .buy-option row matching the affiliate store rows, instead of the old standalone .cta-section vendor card. It renders above the affiliate list, deliberately: the affiliate commission disclaimer should stay adjacent to the links it actually describes, and WorldCat earns no commission, so it shouldn't sit under that notice. WorldcatLink.html has exactly one caller, so restyling it affects nothing else.

bundlesize: 3 failing bundles → all 24 checks pass

components/read-panel.css is imported by both page-book.css and legacy.css, and legacy.css feeds six more bundles — so the buy-dropdown CSS was duplicated into 7 bundles, 6 of which never render a buy button.

Following docs/ai/css.md (raising caps isn't among its sanctioned options):

• moved the buy rules into components/buy-popover.css, imported by page-book.css only
• removed CSS this PR orphaned (.desktop-vendor, .mobile-vendor, .cta-section-title.world-cat-link)
• removed the transition on the hover background, which design.md forbids outright
• removed three long-dead rule sets to make room, as css.md prescribes — .sponsorship-card (markup deleted by 90d508e9f "remove sponsorship"), .check-availability, .check-other-editions (markup gone since Adding IA ebook preview button #2341/1592/feature/book preview #2365). All three verified to have zero non-CSS references repo-wide, and traced through git history rather than trusted to a grep.

Local bundlesize: page-book 16.92KB < 17KB, page-edit 26.72KB < 27KB, page-plain 26.8KB < 27KB — 24 passed, 0 failed.

Playwright verification

Chromium, desktop 1280×800 and Pixel 5. All passing:

• no 500s across 4 editions and 2 works
• Buy button renders with label and cart icon; aria-haspopup="dialog", aria-expanded toggles
• opens by click, Enter and Space
• Escape closes it and returns focus to the trigger
• Tab reaches every option including WorldCat
• WorldCat row carries a real worldcat.org/oclc/… href with rel="noopener noreferrer", and is present in the sidebar CTA (not just the identifiers table)
• mobile tray opens and shows WorldCat
• no uncaught JS errors

Still yours, and deliberately not touched

I left the design-system items from my review for you rather than rewriting your styling — except the transition removal above, which I took because design.md forbids it and it also bought the bundlesize headroom. Two accessibility items are worth doing and are small:

• cart SVG needs aria-hidden="true" — it's decorative next to a real "Buy" label, and is currently exposed to assistive tech.
• keyboard focus doesn't mirror the hover highlight. Confirmed in the browser: hovering a row gives rgb(245, 243, 235), focusing it by keyboard gives rgba(0, 0, 0, 0). Since design.md also asks for hover to be gated behind @media (hover: hover) and (pointer: fine), both are fixed together by making the rule :hover, :focus-visible and wrapping the hover half in that query.

Two things I confirmed are not yours, so they don't need action here: the [serious] axe aria-hidden-focus violation on the popover's focus sentinels, and focus being stranded when Escape is pressed mid-open-animation — both reproduce identically on the unmodified header browse-popover, so they're pre-existing OlPopover behaviour affecting every popover on the site.