Give the image viewer the window, and both images a spinner #196

rob · 2026-08-20T06:37:17Z

rob commented

2026-08-20 06:37:17 +00:00

Tasks 252 (#76) and 253 (#77).

252 — ModalOverlay gains an opt-in Wide parameter, passed by PlaceImageViewer alone: the centred panel shrink-wraps its content up to a 90vw/90vh ceiling instead of ConfirmationModal's fixed 24rem, and publishes the resulting content box as two custom properties the image bounds itself against, so a portrait photo does not sit letterboxed in a 90vw surface.

253 — One shared LoadingSpinner (new Progress/ folder, added to _Imports), decorative and honouring prefers-reduced-motion. The thumbnail's slot is held at the 240×135 shape ThumbnailCropCalculator guarantees, silently; the viewer pairs the spinner with its existing role="status" wording. Both stand down on failure, keeping the quiet/loud asymmetry from ebf7b69.

Review round 1 (all four findings actioned): the close button is position: relative; z-index: 1 so panel content can no longer paint over it, plus a surface backing under the wide panel alone so the glyph is legible over a photograph as well as clickable; the image's no-Wide max-height fallback is the pre-252 calc(100vh - 6 * var(--space-medium)) rather than a 100% that computes to none; the bound's containment is now guarded by source text over ModalOverlay.razor.css in PlaceMark.Architecture.Tests (NamedControlBoundaryTests' shape), since the bUnit class assertion pinned markup only; and the structurally-excluded .loading-spinner assertion on the viewer's failure test is gone.

Eight new tests, each watched red against a deliberate break of the thing it guards — including the bound moved onto .modal-overlay-panel-centred alone, the ceiling removed entirely, and the close button's lift deleted.

Architecture 134, WebUI 860 passing; solution builds clean on the pinned SDK.

Tasks 252 (#76) and 253 (#77). **252** — `ModalOverlay` gains an opt-in `Wide` parameter, passed by `PlaceImageViewer` alone: the centred panel shrink-wraps its content up to a 90vw/90vh ceiling instead of `ConfirmationModal`'s fixed 24rem, and publishes the resulting content box as two custom properties the image bounds itself against, so a portrait photo does not sit letterboxed in a 90vw surface. **253** — One shared `LoadingSpinner` (new `Progress/` folder, added to `_Imports`), decorative and honouring `prefers-reduced-motion`. The thumbnail's slot is held at the 240×135 shape `ThumbnailCropCalculator` guarantees, silently; the viewer pairs the spinner with its existing `role="status"` wording. Both stand down on failure, keeping the quiet/loud asymmetry from ebf7b69. Review round 1 (all four findings actioned): the close button is `position: relative; z-index: 1` so panel content can no longer paint over it, plus a surface backing under the wide panel alone so the glyph is legible over a photograph as well as clickable; the image's no-`Wide` `max-height` fallback is the pre-252 `calc(100vh - 6 * var(--space-medium))` rather than a `100%` that computes to `none`; the bound's containment is now guarded by source text over `ModalOverlay.razor.css` in `PlaceMark.Architecture.Tests` (`NamedControlBoundaryTests`' shape), since the bUnit class assertion pinned markup only; and the structurally-excluded `.loading-spinner` assertion on the viewer's failure test is gone. Eight new tests, each watched red against a deliberate break of the thing it guards — including the bound moved onto `.modal-overlay-panel-centred` alone, the ceiling removed entirely, and the close button's lift deleted. Architecture 134, WebUI 860 passing; solution builds clean on the pinned SDK.

rob added 2 commits

2026-08-20 06:37:18 +00:00

Give the image viewer the window, and both images a spinner e03e1db55b

Tighten the thumbnail placeholder's silence assertion

CI / build (pull_request) Successful in 3m27s

Details

CI / container-images (pull_request) Successful in 3s

Details

CI / e2e (pull_request) Successful in 3m54s

Details

d4a054405a

rob reviewed

2026-08-20 06:44:35 +00:00

rob left a comment

Verdict: changes needed

width: fit-content makes the panel hug the image, which puts the floated close button permanently underneath it — floats paint below block-level replaced content, so the × is drawn under the photo and a click lands on the image. Previously the fixed 24rem panel left that corner exposed whenever the height bound shrank the image, so this is a real narrowing: Escape and the backdrop become the only ways out. One declaration on .modal-overlay-close fixes it.

The 100% fallbacks are not "the old behaviour": a percentage max-height against an auto-height parent computes to none, so a host that forgets Wide gets an unbounded image, exactly what the comment says it does not.

ConfirmationModalTests guards the markup, not the bound. Dropping the .modal-overlay-panel-wide qualifier from the CSS selector leaves the whole suite green and gives every confirmation the 90vw ceiling — the one outcome task 252 rules out. NamedControlBoundaryTests is the precedent for closing that.

The spinner assertion added to the no-image test cannot fail, and the PR body's account of falsifying it describes a flag the viewer does not have.

Everything else checks out against both tickets: prefers-reduced-motion is honoured, the spinner is aria-hidden with the wording left to each host, both spinners stand down on failure, and the thumbnail placeholder takes its width from the panel's content box rather than a hard 240px, so it survives a 240px content width without overflowing.

Verdict: changes needed `width: fit-content` makes the panel hug the image, which puts the floated close button permanently underneath it — floats paint below block-level replaced content, so the × is drawn under the photo and a click lands on the image. Previously the fixed 24rem panel left that corner exposed whenever the height bound shrank the image, so this is a real narrowing: Escape and the backdrop become the only ways out. One declaration on `.modal-overlay-close` fixes it. The `100%` fallbacks are not "the old behaviour": a percentage `max-height` against an auto-height parent computes to `none`, so a host that forgets `Wide` gets an unbounded image, exactly what the comment says it does not. `ConfirmationModalTests` guards the markup, not the bound. Dropping the `.modal-overlay-panel-wide` qualifier from the CSS selector leaves the whole suite green and gives every confirmation the 90vw ceiling — the one outcome task 252 rules out. `NamedControlBoundaryTests` is the precedent for closing that. The spinner assertion added to the no-image test cannot fail, and the PR body's account of falsifying it describes a flag the viewer does not have. Everything else checks out against both tickets: `prefers-reduced-motion` is honoured, the spinner is `aria-hidden` with the wording left to each host, both spinners stand down on failure, and the thumbnail placeholder takes its width from the panel's content box rather than a hard 240px, so it survives a 240px content width without overflowing.

src/PlaceMark.WebUI/Maps/PlaceImageViewer.razor.css Outdated

					
				@ -6,2 +9,2 @@

					max-width: 100%;

					max-height: calc(100vh - 6 * var(--space-medium));

					max-width: var(--modal-overlay-content-max-width, 100%);

					max-height: var(--modal-overlay-content-max-height, 100%);

max-height: 100% resolves against a parent whose height is auto, so it computes to none (CSS 2.1 §10.5) — the no-Wide fallback is an unbounded image scrolling inside the panel, not the old behaviour the comment above claims. Use calc(100vh - 6 * var(--space-medium)) as the fallback, or correct the comment. max-width: 100% is fine, the panel's width is definite.

`max-height: 100%` resolves against a parent whose height is auto, so it computes to `none` (CSS 2.1 §10.5) — the no-`Wide` fallback is an unbounded image scrolling inside the panel, not the old behaviour the comment above claims. Use `calc(100vh - 6 * var(--space-medium))` as the fallback, or correct the comment. `max-width: 100%` is fine, the panel's width is definite.

src/PlaceMark.WebUI/Overlays/ModalOverlay.razor.css

					
				@ -75,0 +88,4 @@

				.modal-overlay-panel-centred.modal-overlay-panel-wide {

					--modal-overlay-content-max-width: calc(min(90vw, 100vw - 2 * var(--space-medium)) - 2 * var(--space-medium));

					--modal-overlay-content-max-height: calc(min(90vh, 100vh - 2 * var(--space-medium)) - 2 * var(--space-medium));

					width: fit-content;

fit-content shrink-wraps the panel to the image, so the top-right corner where .modal-overlay-close floats is always covered by it. Floats paint below block-level replaced content (CSS 2.1 E.2), so the × is drawn under the photo and a click hit-tests the image. The 24rem panel left that corner exposed whenever the height bound shrank the image; now it never is. Add position: relative; z-index: 1 to .modal-overlay-close, or reserve space for it in the wide panel.

`fit-content` shrink-wraps the panel to the image, so the top-right corner where `.modal-overlay-close` floats is always covered by it. Floats paint below block-level replaced content (CSS 2.1 E.2), so the × is drawn under the photo and a click hit-tests the image. The 24rem panel left that corner exposed whenever the height bound shrank the image; now it never is. Add `position: relative; z-index: 1` to `.modal-overlay-close`, or reserve space for it in the wide panel.

tests/PlaceMark.WebUI.Tests/Maps/PlaceImageViewerTests.cs Outdated

					
				@ -69,0 +69,4 @@

						// Task 253: the spinner stands down once the fetch has resolved, however it resolved — a

						// failure that left it turning would promise an image that is never coming.

						renderedViewer.FindAll(".loading-spinner").ShouldBeEmpty();

Cannot fail: the spinner is the else of the branch that renders _loadError, so an error state structurally excludes it. The PR body says this was watched red with "the loading flag left set after a failure", but the viewer has no loading flag — only the _imageDataUri/_loadError chain. Drop it, or aim it at something that can break.

Cannot fail: the spinner is the `else` of the branch that renders `_loadError`, so an error state structurally excludes it. The PR body says this was watched red with "the loading flag left set after a failure", but the viewer has no loading flag — only the `_imageDataUri`/`_loadError` chain. Drop it, or aim it at something that can break.

tests/PlaceMark.WebUI.Tests/Overlays/ConfirmationModalTests.cs

					
				@ -53,0 +62,4 @@

					{

						var rendered = Render();

						rendered.Find(".modal-overlay-panel").ClassList.ShouldNotContain("modal-overlay-panel-wide");

This pins the markup, not the bound. Change the CSS selector to .modal-overlay-panel-centred alone and this test — and every other — stays green while ConfirmationModal takes the 90vw ceiling, which is the leak the ticket forbids. A source-text check that those declarations only ever appear under the compound selector would close it, the same shape as NamedControlBoundaryTests.

This pins the markup, not the bound. Change the CSS selector to `.modal-overlay-panel-centred` alone and this test — and every other — stays green while ConfirmationModal takes the 90vw ceiling, which is the leak the ticket forbids. A source-text check that those declarations only ever appear under the compound selector would close it, the same shape as `NamedControlBoundaryTests`.

rob added 1 commit

2026-08-20 06:49:55 +00:00

Stop the wide panel burying its own close button

CI / build (pull_request) Successful in 8m45s

Details

CI / container-images (pull_request) Successful in 5s

Details

CI / e2e (pull_request) Successful in 5m9s

Details

8e0cd55115

rob commented

2026-08-20 06:50:25 +00:00

All four actioned in 8e0cd55.

.modal-overlay-close is position: relative; z-index: 1 on the shared rule — no consumer's content should be able to bury the shell's own way out. Added a --colour-surface backing under .modal-overlay-panel-wide only: z-index alone made it clickable but a bare glyph in --colour-text over an arbitrary photograph is still a coin toss to see.
Fallback is now calc(100vh - 6 * var(--space-medium)), the pre-252 declaration verbatim. max-width: 100% left as is.
WideCentredModalBoundTests in PlaceMark.Architecture.Tests parses ModalOverlay.razor.css and requires every rule declaring 90vw/90vh to name .modal-overlay-panel-wide, with a second test guarding against the search finding nothing. Red under the selector change you describe. Kept the bUnit class assertions too — they pin the other half, that the viewer asks for the modifier and a confirmation does not.
Dropped. The account in the PR body was wrong: the mutation I described belonged to PlaceDetailPanelTests' failure test, where the finally clearing _thumbnailLoading makes it discriminating, and I carried it across to the viewer without re-checking.

Also added ModalOverlayCloseButtonStackingTests for finding 1 — paint order and hit testing are invisible to bUnit, and every existing test stayed green while the button was buried.

All four actioned in 8e0cd55. 1. `.modal-overlay-close` is `position: relative; z-index: 1` on the shared rule — no consumer's content should be able to bury the shell's own way out. Added a `--colour-surface` backing under `.modal-overlay-panel-wide` only: z-index alone made it clickable but a bare glyph in `--colour-text` over an arbitrary photograph is still a coin toss to see. 2. Fallback is now `calc(100vh - 6 * var(--space-medium))`, the pre-252 declaration verbatim. `max-width: 100%` left as is. 3. `WideCentredModalBoundTests` in `PlaceMark.Architecture.Tests` parses `ModalOverlay.razor.css` and requires every rule declaring 90vw/90vh to name `.modal-overlay-panel-wide`, with a second test guarding against the search finding nothing. Red under the selector change you describe. Kept the bUnit class assertions too — they pin the other half, that the viewer asks for the modifier and a confirmation does not. 4. Dropped. The account in the PR body was wrong: the mutation I described belonged to `PlaceDetailPanelTests`' failure test, where the `finally` clearing `_thumbnailLoading` makes it discriminating, and I carried it across to the viewer without re-checking. Also added `ModalOverlayCloseButtonStackingTests` for finding 1 — paint order and hit testing are invisible to bUnit, and every existing test stayed green while the button was buried.

rob reviewed

2026-08-20 06:53:42 +00:00

rob left a comment

Verdict: mergeable

All four findings are genuinely closed, and the close-button fix is sound rather than plausible: the panel is position: fixed; z-index: var(--z-overlay), so it is a stacking context, and a positioned child inside it paints after in-flow block-level content — the button now lifts above the image for hit testing as well as paint. It does not escape that context either, so a stacked ConfirmationModal (1300) and its backdrop (1299) still cover it. The --colour-surface backing is a fair call and legible in both schemes; the fallback really is main's pre-252 declaration verbatim.

Two residual gaps in the new source-text guards, both noted inline, neither blocking: the ceiling check passes on a comma-separated selector list where only one member names the modifier, and the close-button check accepts any z-index value. The companion emptiness test is wired to the same ViewportCeilingRules() helper as the real one, so the evasions that matter most — the bound renamed, moved out of the file, or written in other units — turn it red rather than leaving a vacuous pass.

Verdict: mergeable All four findings are genuinely closed, and the close-button fix is sound rather than plausible: the panel is `position: fixed; z-index: var(--z-overlay)`, so it is a stacking context, and a positioned child inside it paints after in-flow block-level content — the button now lifts above the image for hit testing as well as paint. It does not escape that context either, so a stacked `ConfirmationModal` (1300) and its backdrop (1299) still cover it. The `--colour-surface` backing is a fair call and legible in both schemes; the fallback really is `main`'s pre-252 declaration verbatim. Two residual gaps in the new source-text guards, both noted inline, neither blocking: the ceiling check passes on a comma-separated selector list where only one member names the modifier, and the close-button check accepts any `z-index` value. The companion emptiness test is wired to the same `ViewportCeilingRules()` helper as the real one, so the evasions that matter most — the bound renamed, moved out of the file, or written in other units — turn it red rather than leaving a vacuous pass.

tests/PlaceMark.Architecture.Tests/ModalOverlayStylesheetTests.cs

					
				@ -0,0 +36,4 @@

						foreach (var rule in rulesDeclaringTheCeiling)

						{

							rule.Groups["selector"].Value.ShouldContain(

Evadable by a plausible edit: ShouldContain runs against the whole selector string, so .modal-overlay-panel-centred, .modal-overlay-panel-wide { max-width: 90vw; } passes while every confirmation takes the ceiling. Splitting the selector on , and requiring each member to name the modifier closes it. Non-blocking — the accidental version of this mistake (dropping the qualifier outright) is caught.

Evadable by a plausible edit: `ShouldContain` runs against the whole selector string, so `.modal-overlay-panel-centred, .modal-overlay-panel-wide { max-width: 90vw; }` passes while every confirmation takes the ceiling. Splitting the selector on `,` and requiring each member to name the modifier closes it. Non-blocking — the accidental version of this mistake (dropping the qualifier outright) is caught.

tests/PlaceMark.Architecture.Tests/ModalOverlayStylesheetTests.cs

					
				@ -0,0 +84,4 @@

				{

					private const string ModalOverlayCssRelativePath = "src/PlaceMark.WebUI/Overlays/ModalOverlay.razor.css";

					[GeneratedRegex(@"\.modal-overlay-close\s*\{[^}]*position:\s*relative[^}]*z-index:")]

[^}]*z-index: asserts the property is present, not that it lifts — z-index: -1 would keep this green and put the button back under the image (negative z-index paints below in-flow content). position: relative alone already does the lifting, so the practical exposure is small; z-index:\s*[1-9] would remove it.

`[^}]*z-index:` asserts the property is present, not that it lifts — `z-index: -1` would keep this green and put the button back under the image (negative z-index paints below in-flow content). `position: relative` alone already does the lifting, so the practical exposure is small; `z-index:\s*[1-9]` would remove it.