Skip to content

Search result bookcover image space reservation - #13129

Merged
lokesh merged 10 commits into
internetarchive:masterfrom
lbartron:12825/fix/search-cover-image-space
Sep 15, 2026
Merged

lokesh merged 10 commits into
internetarchive:masterfrom
lbartron:12825/fix/search-cover-image-space

Conversation

@lbartron

@lbartron lbartron commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

Closes #12825

Reserves layout space for cover images in list-view search results to prevent cumulative layout shift. I originally suggested a flat aspect ratio of 180 / 270. This PR uses the actual per-book cover_width and cover_height fields that are now available in Solr thanks to PR #12916.

The <img> gets width/height attributes so the browser reserves the right box before the cover loads. When dimensions aren't available (likely for many books, since the Solr fields are recent), or when the cover shown isn't the one those dimensions describe, it falls back to 180×270 (2:3). Results with no cover use the placeholder's own 84×115.

Technical

  • openlibrary/plugins/worksearch/schemes/works.py: request cover_width / cover_height from Solr via default_fetched_fields, which also carries them into the editions.fl subquery.
  • openlibrary/plugins/worksearch/code.py: add both fields to get_doc()'s allow-list so they reach the template.
  • openlibrary/macros/SearchResultsWork.html: set width/height on the cover <img>. Solr dimensions are only used when the displayed URL was built from the doc's cover_i. For a work, get_cover_url() prefers the availability edition's cover or the IA cover over cover_i, so the indexed dimensions would describe a different image.
  • static/css/components/search-result-item.css: add height: auto; object-fit: contain to .searchResultItem .bookcover img so the attributes reserve space and the real ratio wins once the image loads. The existing width: 175px / max-height: 300px constraints are unchanged. No aspect-ratio is declared, since it would override the image's real ratio.

Testing

  • test_cover_dimensions_fetched_for_works_and_editions checks the fields are in default_fetched_fields and in editions.fl.
  • test_get_doc covers the new fields passing through get_doc().
  • Manual CLS verification through Lighthouse is still to do, once the local Solr has cover dimensions to test against.

Screenshot

To follow with the manual verification.

Stakeholders

@lokesh

@openlibrary-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR, @lbartron!

🤖 Copilot has been assigned for an initial review.

@lokesh is assigned to this PR and currently has:

  • 4 open PR(s) of equal or higher priority to review first

Possible improvements for this PR

  • CI is failing — pre-commit.ci couldn't complete its check because this branch has merge conflicts with master. Please rebase: see the git guide.
PR triage checklist (maintainers / Richy)
  • 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. PAM provides status visibility, performs basic project management functions, and gives actionable feedback so contributors aren't left waiting.

@openlibrary-bot openlibrary-bot added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jul 7, 2026

Copilot AI left a comment

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.

Pull request overview

Reserves layout space for cover images in list-view search results to reduce cumulative layout shift (CLS) by using per-book cover dimensions (cover_width/cover_height) from Solr, with a 2/3 fallback ratio when dimensions are missing.

Changes:

  • Fetch cover_width/cover_height in the worksearch Solr scheme defaults.
  • Pass cover_width/cover_height through get_doc() so templates can access them.
  • Apply aspect-ratio in CSS and (when available) set --cw/--ch from the selected edition in the search result template.

Reviewed changes

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

File Description
static/css/components/search-result-item.css Switches cover rendering to aspect-ratio (with CSS var fallback) and height: auto for stable layout.
openlibrary/plugins/worksearch/schemes/works.py Requests cover_width/cover_height from Solr as available fields and default fetched fields.
openlibrary/plugins/worksearch/code.py Exposes cover_width/cover_height on the doc object returned to templates.
openlibrary/macros/SearchResultsWork.html Sets per-result CSS variables (--cw/--ch) from the selected edition when available.

Comment on lines 690 to +694
first_edition=doc.get("first_edition", None),
subtitle=doc.get("subtitle", None),
cover_edition_key=doc.get("cover_edition_key", None),
cover_width=doc.get("cover_width", None),
cover_height=doc.get("cover_height", None),
Comment on lines +67 to +74
$ cover = get_cover_url(selected_ed) or "/static/images/icons/avatar_book-sm.png"
$ cover_w = selected_ed.get('cover_width')
$ cover_h = selected_ed.get('cover_height')
<a href="$work_edition_url">
<img
itemprop="image"
src="$cover"
$:cond(cover_w and cover_h, 'style="--cw:%s;--ch:%s;"' % (cover_w, cover_h))
Comment on lines 153 to 159
.searchResultItem .bookcover img {
border-radius: var(--border-radius-thumbnail);
max-width: 100%;
max-height: 300px;
width: 175px;
height: auto;
aspect-ratio: var(--cw, 2) / var(--ch, 3);
}
@github-actions github-actions Bot added the Needs: Response Issues which require feedback from lead label Jul 7, 2026
@github-actions

Copy link
Copy Markdown

Hello! This pull request has been marked as stale because it has been waiting for submitter input for 7 days. If you're still working on this, please add a comment to keep it open. Otherwise, a maintainer will have to determine how to proceed including reassigning or opening the issue for others to work on after communicating with you.
For guidance on contributing, please see our Contributing guide.

@github-actions github-actions Bot added the Stale label Jul 15, 2026
@lokesh

lokesh commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this work!

Three things to check on:
1. Drop the all_fields change
There are two different lists in the search scheme that sound similar. The default_fetched_fields is the 'what Solr should send back to us' and that is the one this PR needs to update so the cover dimensions arrive in the template. all_fields is what the user is allowed to search on. e.g. cover_width:[600 TO *] in these search input

2. Add object-fit: contain
If the loaded image isn't shaped 2:3, for example a square, I suspect it will get distorted to fit our aspect ratio. With object-fit: contain the image scales down to fit inside our 2:3 aspect ratio box while keeping its own real shape.

** 3. The cover-source mismatch**
A scenario we need to account for: the dimensions in Solr measure one specific image, the cover identified by cover_i. But the function that picks the src for the doesn't always return that image. It could hand back a different edition's cover or an Archive.org scan instead. That's the theory, in practice, this barely happens. And if use object-fit: contain, the image should still render fine and without distortion.

Overall direction makes sense to me. If you can test and gather some screenshots, we'll push this through.

@lokesh lokesh added Theme: Design Issues related to UI design, branding, etc. [managed] Theme: Public APIs Issues related to APIs accessible to external parties. [managed] Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] and removed Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] Needs: Response Issues which require feedback from lead Stale labels Jul 17, 2026
@github-actions

Copy link
Copy Markdown

Hello! This pull request has been marked as stale because it has been waiting for submitter input for 7 days. If you're still working on this, please add a comment to keep it open. Otherwise, a maintainer will have to determine how to proceed including reassigning or opening the issue for others to work on after communicating with you.
For guidance on contributing, please see our Contributing guide.

@github-actions github-actions Bot added Stale Needs: Response Issues which require feedback from lead and removed Stale labels Jul 25, 2026
@github-actions github-actions Bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jul 30, 2026
@lokesh lokesh removed the Needs: Response Issues which require feedback from lead label Jul 30, 2026
@lokesh

lokesh commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Pushed a couple of commits to this branch — merged master and reworked the space reservation. Summary of what was wrong and what changed:

test_get_doc was failing. It asserts full-dict equality, so the two new keys in get_doc() broke it. Added cover_width/cover_height to both the input doc and the expected storage.

Branch was stale. get_doc() here predated cover_i being added on master. Merged master in.

The CSS aspect-ratio approach distorted covers. aspect-ratio: var(--cw) / var(--ch) force-fits the image to whatever Solr reports, so a stale or wrong dimension stretches the actual cover — and the no-cover placeholder (84×115) got squashed to 2:3 on every coverless result. Removing max-height: 300px also let tall covers blow out row height.

Swapped the CSS custom properties for plain width/height attributes on the <img>. Browsers derive an intrinsic ratio from those to reserve space before load, which is what kills the CLS, but the real image ratio takes over once it loads — so bad Solr data can't distort anything. Falls back to 180×270 when dimensions are missing. Restored max-height: 300px and added object-fit: contain so tall covers letterbox instead of squash.

Added a scheme test asserting the fields reach both the work fl and the editions subquery editions.fl — that derivation is the non-obvious part of the wiring, since the macro reads dimensions off the selected edition, not the work.

One thing I left alone: the all_fields addition. It isn't needed to fetch the fields (fields goes straight to fl without being checked against it) — all it does is expose cover_width: as public query syntax. Worth dropping unless you want that API surface.

lokesh added 2 commits July 29, 2026 23:31
They only need to be in default_fetched_fields to come back from Solr;
all_fields is the user-searchable set, and cover_width:[600 TO *] isn't
query syntax we want to expose.
The no-cover avatar is 84x115, not 2:3, so the 180x270 fallback reserved
262px and settled at 240px on load. Take the dimensions from the src we
actually chose, and let the placeholder win over stale Solr dimensions
when no cover URL resolves.
@lokesh

lokesh commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Following up on point 3 from my earlier comment — I dug into it and it's more structural than I first thought, so writing it down before it gets lost.

The dimensions and the src are decided in two places that don't talk to each other. The template asks the selected edition for cover_width/cover_height, and separately asks get_cover_url() for a URL. Nothing guarantees both answers describe the same image. get_cover_url() is a chain of five fallbacks and only the cover_i ones line up with what Solr measured; the availability branches can hand back a different edition's cover (/b/olid/…), or the Archive.org scan page cover, which is always 180×360, so 1:2 rather than 2:3. Those branches are live on every surface that calls add_availability — search, reading log, loan history, lists, trending — so it's not as rare as I implied.

Nothing looks broken when it happens, since object-fit: contain keeps the cover undistorted, but the reserved box was the wrong height and the row still nudges when the image lands. That's the same bug we're fixing, one layer up.

Patching a single branch won't hold — the other four can still drift. The durable version is to have the resolver answer both questions at once: a get_cover_url_with_dims() (or similar) returning (url, width, height), where each branch reports what it knows — Solr dimensions for the cover_i branches, a hardcoded 180×360 for the Archive.org page cover, nothing for the genuinely unknown one so the 2:3 guess applies. Then the two values can't diverge, because they come from one decision.

I'd keep that out of this PR — it touches book_providers.py and every caller of get_cover_url(). Follow-up issue, and this PR still wins on the common case.

Two smaller things on the current branch, both of which I'm taking care of so you don't have to: the all_fields addition is still in the diff (only default_fetched_fields is needed to fetch), and the no-cover placeholder is the one mismatch worth fixing here — avatar_book-sm.png is 84×115, not 2:3, so the 180×270 fallback reserves 262px and settles at 240px on load, on every coverless result. Fix is to take the dimensions from the src that actually got chosen rather than assuming a book shape.

Still want screenshots and a CLS measurement before this merges. One heads up there: local covers won't tell you anything useful, since the dev coverstore serves blank 1×1 placeholders that report a 1:1 ratio and mask the shift. Point covers at the production CDN first, and make sure your sample includes a coverless result and a lending/availability result so both of the above show up in the numbers.

@lokesh
lokesh force-pushed the 12825/fix/search-cover-image-space branch from 19aefc8 to 6146baa Compare July 30, 2026 07:05
get_cover_url prefers the availability edition or IA cover over the work's
cover_i, so Solr's cover_width/cover_height can describe a different image.
Fall back to 2:3 in that case.
…cover-image-space

# Conflicts:
#	openlibrary/plugins/worksearch/schemes/tests/test_works.py
@lokesh lokesh changed the title Draft for search result bookcover image space reservation Search result bookcover image space reservation Sep 15, 2026

@lokesh lokesh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Tested locally.

A few changes I made to make it more bulletproof:

  • Brought back the 300px height limit that had been removed, so very tall covers can't make rows huge.
  • Added object-fit: contain so covers shrink to fit instead of getting squashed.
  • Used Solr's numbers only when they describe the cover actually shown. Otherwise it falls back to a standard 2:3 shape.

@lokesh
lokesh merged commit 721c173 into internetarchive:master Sep 15, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Theme: Design Issues related to UI design, branding, etc. [managed] Theme: Public APIs Issues related to APIs accessible to external parties. [managed]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reserve cover image space to prevent CLS in list view search results

4 participants