Skip to content

fix(hillshade): revert per-tile blend, use isolation: isolate on #map (#171)#172

Merged
jasoneplumb merged 2 commits intomainlinefrom
fix/hillshade-blend-revert
Apr 26, 2026
Merged

fix(hillshade): revert per-tile blend, use isolation: isolate on #map (#171)#172
jasoneplumb merged 2 commits intomainlinefrom
fix/hillshade-blend-revert

Conversation

@jasoneplumb
Copy link
Copy Markdown
Owner

Summary

  • PR fix(hillshade): apply mix-blend-mode per tile to fix max-zoom dropouts (#168) #169 moved `mix-blend-mode: multiply` from `.hillshade-blend` (Leaflet layer container) to `.hillshade-blend img.leaflet-tile` (individual tile images). My stacking-context reasoning was backwards.
  • Applying the blend AT the tile image places it INSIDE the transformed `.leaflet-tile-container`'s stacking context. That context contains only hillshade tiles — there is nothing to multiply against — so the blend renders source pixels opaque and the base map is completely hidden.
  • The user reported v0.31.0-beta showing "the hillshade is all that is visible when it's enabled".

Fix

  1. Revert the selector to `.hillshade-blend` (the layer container, peer of the base layer's container under `.leaflet-tile-pane` — has the base map as sibling backdrop).
  2. Add `isolation: isolate` to `#map` so the map is a clean stacking-context root — addresses the actual root cause of the original "sometimes not applied" issue from fix(hillshade): mix-blend-mode sometimes not applied at closest zoom on topo layer #168 by preventing ancestor compositing layers from isolating the blend.

Closes #171.

Why this is the right level

`isolation: isolate` on the map root creates a self-contained stacking context. Inside it, every Leaflet pane and layer composites against the map's backdrop, so `mix-blend-mode` on `.hillshade-blend` has a stable, predictable backdrop (the base layer below it in the tile pane).

Test plan

  • `npm run type-check` clean
  • `npm run lint` clean
  • Manual: enable Hillshade with the Topographic basemap — slopes shaded, flat-area base-map color preserved
  • Manual: zoom to max — blend stays consistent without dropouts
  • Manual: switch basemaps with Hillshade enabled — blend re-applies cleanly

🤖 Generated with Claude Code

… instead

PR #169 moved mix-blend-mode from .hillshade-blend (the layer
container) to .hillshade-blend img.leaflet-tile (individual tile
images). I had the stacking-context reasoning backwards: applying
the blend AT the tile image places it INSIDE the transformed
.leaflet-tile-container's stacking context. That context contains
only the hillshade tiles — nothing to multiply against — so the
blend has no backdrop and renders source pixels opaque, hiding the
base map entirely.

Correct placement is at .hillshade-blend, which is a peer of the
base-layer container under .leaflet-tile-pane. There the multiply
blend has the base map as the sibling backdrop.

To address the original "sometimes not applied" symptom from #168,
add isolation: isolate to #map. This makes the map a clean stacking-
context root so the blend can reliably composite against base-layer
tiles without ancestor compositing layers isolating it from the
intended backdrop.

Closes #171
Re-opens #168 conceptually but the new fix targets the actual root
cause.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jasoneplumb jasoneplumb added bug Something isn't working P1 Critical — fix immediately review-requested Triggers Claude code review and removed review-requested Triggers Claude code review labels Apr 26, 2026
@claude
Copy link
Copy Markdown

claude Bot commented Apr 26, 2026

Code Review

Overview

This is a focused, correct CSS fix: reverts mix-blend-mode from the per-tile img.leaflet-tile selector back to the .hillshade-blend container level, and adds isolation: isolate to #map to solve the underlying stacking-context problem. The technical reasoning in the PR description is sound.


What's working well

  • Correct root-cause fix. Moving the blend to the layer container (peer of the base-layer container inside .leaflet-tile-pane) gives multiply a proper backdrop. The per-tile approach was blending against an empty stacking context — nothing to multiply against.
  • isolation: isolate on #map addresses the original bug from fix(hillshade): mix-blend-mode sometimes not applied at closest zoom on topo layer #168, not just the regression. This is the right long-term fix.
  • Small, surgical diff with zero risk of collateral breakage.

Issues

Comment style violates CLAUDE.md conventions

CLAUDE.md says:

"never write multi-paragraph docstrings or multi-line comment blocks — one short line max"
"Don't reference the current task, fix, or callers … since those belong in the PR description and rot as the codebase evolves."

Both new comments are multi-line blocks, and the second one explicitly references "PR #169", which will be meaningless context once this merges and time passes. The PR description already carries this history — the code comments should be trimmed to just the non-obvious WHY.

Suggested replacements:

/* isolation: isolate keeps mix-blend-mode contained to the map's own stacking context */
#map { position: absolute; top: 0; bottom: 0; right: 0; left: 0; isolation: isolate; }

/* blend at container level so .hillshade-blend composites against the base-layer sibling below it */
.hillshade-blend { mix-blend-mode: multiply; }

That's one short line each — captures the non-obvious WHY without the implementation history.

Manual test plan is incomplete

Three of the five manual test steps remain unchecked. These cover the core scenarios (hillshade visible without blocking basemap, no dropouts at max zoom, basemap switching). The fix should be manually verified before merge, especially since the previous regression (#171) was a visual failure that escaped without a browser test.


Minor / Non-blocking

  • Performance of isolation: isolate: This promotes #map to a compositing layer, which is a tiny overhead. It's the right trade-off here since the map is already GPU-composited via Leaflet's translate3d tiles, but worth knowing if profiling ever flags paint costs.
  • No type-check or lint impact (CSS-only change) — confirmed by the PR's test checklist.

Summary

The fix is technically correct and well-motivated. Two things before merging:

  1. Trim the comments to single lines and remove the PR reference per CLAUDE.md conventions.
  2. Complete the manual test checklist — hillshade + basemap switching is exactly the scenario that caught the last regression.

🤖 Generated with Claude Code

Single-line comments capturing only the non-obvious WHY; no PR refs
that rot as the codebase evolves.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jasoneplumb jasoneplumb merged commit a610b9b into mainline Apr 26, 2026
2 checks passed
@jasoneplumb jasoneplumb deleted the fix/hillshade-blend-revert branch April 26, 2026 05:42
@claude
Copy link
Copy Markdown

claude Bot commented Apr 26, 2026

Code Review — PR #172

Overview

A minimal, targeted CSS-only fix (4 additions / 3 deletions) for the hillshade blend regression introduced in #169. Reverts mix-blend-mode from per-tile images back to the layer container, and adds isolation: isolate to #map to address the underlying stacking-context root cause from #168.


Correctness ✅

The CSS stacking-context reasoning is sound:

  • Per-tile blend (broken): mix-blend-mode on .hillshade-blend img.leaflet-tile composites each tile against its parent .leaflet-tile-container. Leaflet uses transform: translate3d(…) on tile containers, which promotes them to their own compositing layer — a stacking context that contains only hillshade tiles. With no base-map pixels as backdrop, multiply renders the tiles opaque and hides everything beneath. This explains the reported "hillshade is all that is visible" symptom exactly.
  • Container-level blend (correct): .hillshade-blend sits as a sibling of the base-layer container inside .leaflet-tile-pane. mix-blend-mode on the container composites against the base-layer container below it — the intended backdrop.
  • isolation: isolate on #map: Prevents an ancestor compositing layer (e.g. a browser-promoted element outside #map) from absorbing the stacking context and disrupting the blend. This is the right place to create the boundary.

Code Quality ✅

  • Change is minimal and scoped precisely to the problem.
  • Both comments explain the why (non-obvious CSS compositing behavior) — consistent with the project's guidance to only comment when the reason would surprise a reader.
  • No TypeScript, logic, or test changes needed; this is purely presentational.

Minor Observations

  1. Comment verbosity (style note, non-blocking): The blend comment /* Blend at the layer container so multiply composites against the base-layer sibling below. */ is helpful for this tricky area, but it does partially describe what as well as why. The current wording is fine — just noting it for awareness against the project's "no what-comments" guideline. The isolation: isolate comment is spot-on.

  2. Browser compatibility: isolation: isolate is broadly supported (Chrome 41+, Firefox 36+, Safari 8+). No concerns for a PWA targeting modern mobile browsers.

  3. isolation: isolate z-index side effect: Creating a new stacking context on #map means z-index values on elements inside #map are no longer compared against elements outside it. Given #map fills the entire viewport with position: absolute, this is almost certainly a non-issue — but worth a quick sanity check that any overlays (modals, consent dialogs) rendered inside #map still stack correctly against each other.


Test Plan

The manual test checklist in the PR description is appropriate for a visual CSS change — automated unit tests can't cover blend-mode rendering. Suggested additions to the checklist:

  • Hillshade disabled — base map renders normally, no visual regressions
  • Consent modal / any overlay visible with Hillshade enabled — z-index stacking unaffected by isolation: isolate

Summary

This is a clean, well-reasoned fix. The PR description's stacking-context explanation is excellent — it makes the intent auditable in the future. Ready to merge once the manual rendering checks pass.

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working P1 Critical — fix immediately review-requested Triggers Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: revert PR #169 — hillshade renders opaque, base map invisible

1 participant