Skip to content

Fix: Cap Portrait Video Wrapper Height - #7249

Merged
Gykes merged 1 commit into
stashapp:developfrom
quietsignal44:fix/portrait-video-click-blocking
Oct 1, 2026
Merged

Gykes merged 1 commit into
stashapp:developfrom
quietsignal44:fix/portrait-video-click-blocking

Conversation

@quietsignal44

Copy link
Copy Markdown
Contributor

Summary

Portrait/vertical videos set .video-wrapper height to 177.78vw, which on wide viewports extends far below the visible video area (e.g. 3413px on a 1920px-wide screen). This creates an invisible overlay that blocks clicks on scene tabs, rating buttons, and other UI elements positioned below or beside the player.

Root cause

In ui/v2.5/src/components/ScenePlayer/styles.scss (lines 21-23):

&.portrait .video-wrapper {
    height: 177.78vw;
}

The 177.78vw value (100vw × 16/9) is intended to make portrait videos fill the viewport width at the correct aspect ratio, but there is no upper bound. The wrapper's box extends past the visible video (which is clipped by overflow: hidden) and intercepts pointer events on elements below.

Fix

Add max-height: calc(100vh - 4rem) so the wrapper never exceeds the available viewport height:

&.portrait .video-wrapper {
    height: 177.78vw;
    max-height: calc(100vh - #{$menuHeight});
}

This preserves the existing behavior for viewports where 177.78vw fits, while capping the height on wider screens where it would otherwise overflow.

Related issues

🤖 Generated with Claude Code

https://claude.ai/code/session_01S4siNHnHXrvxFCU7uYaka3

Portrait/vertical videos set the .video-wrapper height to 177.78vw,
which on wide viewports extends far below the visible video area. This
creates an invisible overlay that blocks clicks on scene tabs, rating
buttons, and other UI elements below the player.

Add max-height constraint so the wrapper never exceeds the available
viewport space.

Fixes the issue described in stashapp#6276.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S4siNHnHXrvxFCU7uYaka3
@quietsignal44

Copy link
Copy Markdown
Contributor Author

@WithoutPants @Gykes — Would appreciate your eyes on this one. It's a one-line CSS fix for portrait/vertical videos creating an invisible overlay that blocks clicks on scene tabs and rating buttons below the player.

The root cause is the height: 177.78vw on .VideoPlayer.portrait .video-wrapper with no upper bound — on wider viewports the wrapper extends thousands of pixels past the visible video area. The fix adds max-height: calc(100vh - 4rem) to cap it.

Related to the closed #6276 (same symptoms, but the underlying CSS issue was never addressed).

@DogmaDragon DogmaDragon added the noncompliance Doesn't follow the template or other guidelines. label Sep 22, 2026
8ullyMaguire pushed a commit to 8ullyMaguire/stash that referenced this pull request Oct 1, 2026
Upstream PR stashapp#7249, 1+0 in one file. `.VideoPlayer.portrait .video-wrapper` was
sized `height: 177.78vw` — 100vw × 16/9, derived from viewport **width** with no
upper bound. The reporter's figure is 3413px on a 1920px-wide screen, and the
wrapper's box then extends far below the visible video, covering the scene tabs
and rating buttons. `overflow: hidden` is on the wrapper, so it clips its own
video — the wrapper is not clipped by the container.

The cap reuses the bound the container already applies
(`calc(100vh - #{$menuHeight})`), so the wrapper can never be taller than the
player it sits in.

**I could not reproduce the reported overflow from the stylesheet as written, and
the probe says so rather than implying otherwise.** Driving headless Chromium
over CDP with a faithful transcription of the SCSS (jsdom implements neither the
cascade, viewport units, nor flex overflow):

- `>= 1200px` the container gets a *definite* `height: 100vh` from its media
  query, so `flex-shrink` (default 1) pulls the item back inside — measured 989
  against a 1016 container at 1920×1080.
- `< 1200px` the container's height is `auto`, so shrink does not apply — but
  `overflow: hidden` on the wrapper makes the item's automatic minimum size
  (`min-height: auto`) resolve to **zero**, so it still shrinks. No overflow at
  800×600, 900×700, 1100×800, 1440×800 or 1920×1080.

The 3413px figure appears **only** with `flex-shrink: 0`, which the SCSS does not
set. It is `1.7778 × 1920` — arithmetic on the declared value, not a rendered
measurement, which is what a fixed multiplier makes easy to produce.

So the fix rests on what is established, which is enough: `max-height` only ever
**clamps down**, so it cannot grow the box and cannot regress a correctly sized
video (verified: capped ≤ uncapped in every state, and inert wherever flexbox
already fits the box), and it binds the moment anything prevents the shrink — a
taller box, a changed container, or a browser that resolves the minimum size
differently. That is the reporter's environment, and the case I cannot reproduce
here.

**Two claims I started with were wrong, and measurement killed both.**

1. *"`56.25vw` in the base rule has the same defect, smaller multiplier."* No. It
   is a flex item too and shrinks identically — 462 inside a 536 container at
   1920×600. Unbounded-vw-from-width is not by itself a bug; the missing
   ingredient is the missing shrink.
2. *"`overflow: hidden` on the wrapper proves the click blocker."* Not so — the
   first overlap check reported `overflow=0` with the wrapper at 3413px, because
   it measured a **flow sibling**, which stacks below the wrapper and can never
   overlap it. The elements the report names are fixed page chrome. Measuring the
   right thing showed 48px of genuine overlap; whether a *click* is eaten then
   depends on paint order, which I settled by reading the tree rather than
   guessing — `SceneLoader` renders `<ScenePage>` first and the
   `scene-player-container` **last** (`Scene.tsx:1045-1079`), and the tabs
   (`Scene.tsx:699`, with `RatingSystem` at 724) live inside `ScenePage`, so the
   wrapper is the later sibling and paints over them.

Kept `ui/v2.5/scripts/probe-portrait-cap.mjs` (19 checks) in the repo, alongside
the stashapp#7245 DOM probe. `Wall/styles.scss:6` uses the same
`height: 11.25vw` shape and is already capped at `max-height: 253px`, which is
the same defence — the pattern exists in this codebase, `ScenePlayer` just lacked
it. Landscape stays uncapped on purpose: measured inert, and the cap would be
dead code.

Gates: suite 2/2 clean, 38 packages ok; `-race` on internal/api x3 clean; tsc
clean; all seven frontend harnesses exit 0.
8ullyMaguire pushed a commit to 8ullyMaguire/stash that referenced this pull request Oct 1, 2026
PR partition re-derived FROM THE FILE and asserted disjoint: A=6, merged=19,
B1=12, B2=1, B3=32 = 70. Nothing missing, nothing extra, no merged PR listed
pending.

The first attempt asserted against a set I had invented for the CONFLICTING
group and failed. The doc's own B1 block was right all along; my constant was
wrong. Second attempt then read B1's PR numbers as 4011/4699/... because the
closing fence is a 16-backtick run, not three backticks, so a "```" match ran
past the block and into the next one. Both extraction and partition now read
the file, and the file is the authority.

stashapp#7249 needed no ledger change: no linked issue, so nothing moves from planned
to closed. 429 planned / 222 not planned / 24 closed / 675 total stands.

Recorded the finding rather than the win. The 3413px overflow does not
reproduce from the SCSS as written -- flex-shrink pulls the item back inside at
>=1200px, and below 1200px `overflow:hidden` drives the flex item's automatic
minimum size to zero, so it still shrinks. The 3413px needs flex-shrink:0,
which the stylesheet does not set, and 1.7778 x 1920 is arithmetic on the
declared value. The cap is still worth having: max-height only clamps down, so
it cannot regress a correctly sized video, and it binds as soon as anything
prevents the shrink.

Also recorded the two claims measurement killed, since both were plausible and
both were wrong: that the base 56.25vw rule shares the defect (it does not --
it shrinks identically), and that overflow:hidden proves the click blocker (the
first check reported zero overlap with the wrapper at 3413px, because it had
measured a flow sibling, which stacks below and can never overlap).

Gates: suite 2/2 clean, 38 ok; -race on internal/api x3; tsc clean; all seven
frontend harnesses exit 0; issue ledgers agree.

@Gykes Gykes 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.

Did a quick test with a vertical video and didn't see any obvious regressions.

@Gykes Gykes added bug Fix for a reproduced bug and removed noncompliance Doesn't follow the template or other guidelines. labels Oct 1, 2026
@Gykes Gykes added this to the Version 0.32.0 milestone Oct 1, 2026
@Gykes Gykes changed the title fix: cap portrait video wrapper height to prevent click-blocking overlay Fix: Cap Portrait Video Wrapper Height Oct 1, 2026
@Gykes
Gykes merged commit 3cadfe4 into stashapp:develop Oct 1, 2026
11 checks passed
@DogmaDragon DogmaDragon added the noncompliance Doesn't follow the template or other guidelines. label Oct 2, 2026
8ullyMaguire pushed a commit to 8ullyMaguire/stash that referenced this pull request Oct 3, 2026
Keeps the fork current with upstream. The drift was measured with docs/fork-drift.sh rather
than eyeballed -- and the first reading of `git rev-list --left-right --count` was misread,
which made 434 commits look like 434 commits of BEHIND when they are 434 AHEAD. This fork is a
soft fork carrying a large local programme (StashForge governance, the mesh, ed2k transfer), so
"ahead" being large is expected; what matters is the merge-base age.

Upstream work taken in: stashapp#7235 Safari layout, stashapp#7248 VR player colour, stashapp#7259 scraper search,
stashapp#7249 portrait video height, stashapp#7254 organized-button contrast, stashapp#7264 patchable create endpoints,
stashapp#7267 duration-match checkmark.

Merge, not rebase: the ledger cites commits by SHA and the local tags point at them, so a
rebase would rewrite every citation.

Three conflicts, all in files this fork had also touched.

## en-GB.json -- kept ours, spliced in upstream's one key

Upstream's only SEMANTIC change to this file is `package_manager.matched_via`. Every other
difference between the two sides is key ORDER, which git reports as a conflict because both
sides rewrote the same run of lines. So this is a keep-both, not a coin toss: take our side,
then add the one key -- via docs/add-3530-locale.py, the same splice tool, precisely because a
JSON round-trip cannot be byte-identical here.

Verified after: the file parses, all six media_info range keys and all four validation range
keys and both action range keys survive, and matched_via is present.

## Scenes/styles.scss -- took upstream's deletion

This fork set `&.organized { color: #ffffff }` (stash#7160); upstream deleted the whole
`&.organized` block instead (stash#7254). Both fix the same 1.35:1 contrast failure, and
deleting the rule lets the button inherit the default colour rather than restating it. Took
upstream's version and kept a comment recording WHY the rule is absent, since the reason is
invisible in the CSS -- which is what would otherwise invite a well-meaning "tidying" back in.

## utils/apple.ts -- kept ours

Upstream shipped the same ua-parser-js v2 fix (`"macOS"` where v1 said `"Mac OS"`, stash#7234),
terser. Ours takes one UAParser() parse instead of two and names the booleans, which matters
because the two reads were being taken from separate parser instances. Same behaviour, more
legible, and the reasoning is written down.

Verification after the merge:
  go build ./...                   exit 0
  go test -count=1 ./...           61 packages green
  go test -tags integration ./...  61 packages green
  pnpm run check                   1 error, the pre-existing Scene.tsx:756
  vite build                       exit 0
  ScenePlayer/styles.scss          upstream's max-height added, this fork's CSS intact
                                   (git diff HEAD on that file is empty)
8ullyMaguire pushed a commit to 8ullyMaguire/stash that referenced this pull request Oct 3, 2026
Every one of the 69 `planned` rows carried `upstream-marked (bug report|help wanted|bounty)` as
its ENTIRE reason -- which is the label the rows were IMPORTED with, not a decision about any of
them. Seven now have one.

## The measurement that made this cheap

docs/check_planned_upstream.py already reported that 8 of the 69 have a MERGED upstream PR
referencing them. A merged PR is evidence rather than opinion, so those went first.

docs/planned_pr_evidence.py (new) names each PR and -- the column that decides it -- whether its
merge commit is an ANCESTOR OF MAIN. Four of the eight are: their fix is in the tree, carried by
the upstream merge, so the rows close on a fact.

  stashapp#5002  PR stashapp#7018 (2026-06-25) in main   plugin panel collapse/filter/sort
  stashapp#5033  PR stashapp#6559 (2026-02-25) in main   the Tags Tagger
  stashapp#6526  PR stashapp#7249 (2026-10-01) in main   one line: max-height on .portrait .video-wrapper
  stashapp#3692  PR stashapp#5696 (2025-11-18) in main   lumberjack rotation + logfile_max_size
  stashapp#2122  PR stashapp#3619 (2023-05-25) in main   filter UI slice -- row is a discussion thread, not-planned
  stashapp#2049  PR stashapp#2073 (2022-02-03) in main   a submissions PROCESS thread, not a code ask
  stashapp#3065  PR stashapp#1190 NOT in main            scraper-side, vendored from stash-box: deferred

stashapp#2049 and stashapp#2122 are `not-planned` rather than `closed` on purpose: both are discussion/process
threads whose one shipped slice is already here, and calling the whole thread "closed" would claim
more than the evidence supports. stashapp#3065 is `deferred` rather than `not-planned` because its blocking
thing is real and named -- the scraper content lives upstream.

## A helper that answered the wrong question confidently

`in_history` first listed `git branch -r --contains <oid>` and looked for upstream/develop. On my
clone upstream had never been FETCHED, so that ref did not exist -- and every commit was reported
"not in this history", including a 2022 PR that has been in main for years. It now asks
`git merge-base --is-ancestor <oid> main`, which needs no remote refs at all. The 8 rows went from
"0 in main" to "6 in main" on that one line. A helper that is confidently wrong is worse than one
that refuses.

## The evidence gate caught four of my own bad commands

docs/disposition_planned.py (new) will not write a verdict whose evidence command prints nothing,
and it applies goal-check's own reason rules locally first. It rejected:

  - `grep -rn portrait ui/v2.5/src/components/Player/*` -- the path does not exist. The fix is in
    components/ScenePlayer/, and the evidence is now the one line of CSS itself.
  - `git log --grep='logo'` for stashapp#2049 -- matched "auth/login/LOGOUT" three times and printed 7
    unrelated commits. A verdict resting on that would have been noise wearing a citation.
  - `git log --grep='filter'` for stashapp#2122 -- matched my OWN commits from earlier today.
  - `ls internal/scraper/` -- no such directory; it is pkg/scraper/.

It also caught a bug in its own source: a `#` I left inside a Python string did not start a
comment, it swallowed the rest of the line, and the evidence command silently became prose. The
gate refusing to write is what surfaced it.

## Also here

docs/move_resolved_rows.py (new). check-issue-ledgers.py correctly refused four rows marked closed
while still sitting under a heading reading `Planned` -- the exact drift this roster suffered from
once, where the header said 90 planned and the table said 84. Rows move; the Resolved heading's
count is recomputed rather than left to rot. 4 rows moved, (43) -> (51).

## Verification

docs/recount_roster.py            675 = 62 planned, 97 deferred, 465 not-planned, 51 closed-or-done
docs/check-issue-ledgers.py       OK: header, table and log agree (39 closed, 39 log rows)
docs/goal-check.py                C2 69 -> 62; C1,C3,C4,C5,C6,C7,C8 all PASS
docs/disposition_planned.py --check   rehearses clean, writes nothing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Fix for a reproduced bug noncompliance Doesn't follow the template or other guidelines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants