Ring the selected pin's base in its own group colour #199
Loading…
Reference in a new issue
No description provided.
Delete branch "selected-pin-ring"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Task 256 (#80).
The ring is a CSS pseudo-element on the selected marker's own icon element, not a Leaflet layer of its own.
map.jsrecords which marker is selected and puts a class plus a--placemark-selection-ring-colourcustom property on that icon;leaflet-theme.cssdraws the circle at(12, 41)— the pin's anchor — behind the pin (z-index: -1) and out of hit-testing (pointer-events: none). Riding on the icon element is what makes the ring follow the pin for free: it exists exactly when and where the pin does, so a cluster collapsing it takes the ring with it, and a wholesalesetMarkersrebuild puts it back through the marker's ownaddhandler without .NET re-sending anything.Contrast comes from two
box-shadowedges (outer white, inner dark) rather than a theme token, because the colour is whatever hex the group holds (ADR-0117) and could vanish against either scheme's tiles on its own.LeafletMap.SelectedMarkerIdis a parameter of its own rather than a flag onMapMarker:Markersis compared withSequenceEqualand replaced wholesale, so a per-marker flag would rebuild every pin to move one ring.No ADR — this settles nothing that was open.
Tests: 8 new bUnit (
LeafletMapTests,HomeTests), 6 new Playwright (SelectedPinRingJourneyTests, both colour schemes). Each was watched to fail against a deliberate break and pass again after restoring — including one that did not: the first version of the E2E class passed withmap.js's marker-leveladdhandler deleted outright, because every test selected and then read, an order in whichsetSelectedMarker's own sweep suffices.SelectedMarker_ItsGroupRecolouredWhileStillSelected_KeepsTheRingInTheNewColourwas added to force the layer rebuild that half exists for, and it reddens.Clustering is not driven separately in E2E: it destroys and rebuilds the icon element by the same path a recolour does, which that test covers.
Verdict: changes needed
One required change: nothing in the suite discriminates the stacking-context premise the whole design rests on — see the inline comment on
ReadRingAsync.z-index: -1reads-1from the pseudo-element whether or not the icon establishes a context, so the assertion cannot fail for the thing it is there to guard. Two lines inReadRingAsynccloses it.Everything else below is a note, not a gate.
The mechanism itself checks out.
L.divIcon(so::beforerenders at all),iconSize: [25, 41],iconAnchor: [12, 41];left: 12px; bottom: 0+translate(-50%, 50%)puts the ring's centre on exactly (12, 41), andhtml { box-sizing: border-box }(app.css) makes that a 22px outer diameter with the stroke at radius 8–11, which is what the click test's "11px radius" assumes. Leaflet writes both atransformand a numericz-indexonto.leaflet-marker-icon, either of which forms the context, and paint order then puts a negative-z-index pseudo-element above the icon's own background but below its inline<svg>— behind the pin, not behind the tiles..leaflet-container'soverflow: hiddenis the only thing that clips, at the map edge, where the pin clips too.Mutation account, checked rather than taken on trust:
addhandler is caught bySelectedMarker_ItsGroupRecolouredWhileStillSelected_KeepsTheRingInTheNewColourand by nothing else. The other five E2E tests and all eight bUnit tests select-then-read and would stay green. The PR body says exactly this and it is right.setSelectedMarker's sweep, or the id it records, reddens every E2E test.SelectedMarker_ClickJustBelowTheRing_ReachesTheMapNotTheMarkerdoes discriminatepointer-events: none: Leaflet gives an interactive iconpointer-events: auto, the pseudo-element inherits it, the clicked point (anchor + 6px) falls inside the border-radius circle, and the resulting marker click re-selects and leaves the panel up. The page/host coordinate mixing is safe only because.app-shellisposition: fixed; inset: 0and the host fills it — worth knowing if that layout ever changes.Markerschange).SelectedMarkerIdas its own parameter is right:Markersis compared withSequenceEqualand replaced wholesale, andMapMarkeris a record, so a per-marker flag would rebuild every pin to move one ring.Unverified visually, flagged so Rob knows to eyeball it: the 22px diameter, the 3px stroke and the contrast of the two shadow edges have only ever been read as computed styles. Nothing asserts the ring's geometry at all — colour,
z-indexandpointer-eventsare the only properties any test reads — so "sits at the pin's base" is correct by arithmetic and unproven by observation.E2E does run in CI.
.forgejo/workflows/ci.ymlhas a dedicatede2ejob,runs-on: ubuntu-24.04,needs: build, settingPLACEMARK_RUN_E2E_TESTS: "true"and runningdotnet test tests/PlaceMark.E2E.Tests/.... There is no JS unit-test runner and nopackage.jsonanywhere in the repo —map.jsis only ever exercised through Playwright — which is probably what the parallel PR meant, but "no E2E runner" is false. Run #811 for this head is still in flight, so nothing here is confirmed green yet.PR #198: no functional interaction. Its current-location marker carries
placemark-current-location-icon, is added straight to the map rather than toplaceMarkMarkers, and isinteractive: false— so the sweep never visits it, the ring selector never matches it, and this suite's.placemark-marker-iconlocators do not pick it up. Textual conflicts only, inmap.js,LeafletMap.razor.cs,Home.razor,LeafletMapTests.csandHomeTests.cs(both add to the sameSetUpMapModulehelper).@ -74,0 +112,4 @@transform: translate(-50%, 50%);border: 3px solid var(--placemark-selection-ring-colour, currentColor);border-radius: 50%;box-shadow: 0 0 0 1px rgba(255, 255, 255, 0.9), inset 0 0 0 1px rgba(0, 0, 0, 0.35);The two edges are not equivalent, and the weaker one is the one a pale group colour depends on. The outer white is 1px at 0.9 alpha; the inner dark is 1px at 0.35 alpha, and being
insetit is clipped to the padding box, so it renders as a hairline inside the hole rather than bounding the stroke. A near-white ring on light tiles therefore reads on that hairline alone (~2.3:1). Consider a second outer edge —0 0 0 1px rgba(255,255,255,0.9), 0 0 0 2px rgba(0,0,0,0.35)— so both bounds sit outside the stroke regardless of colour. Judge it in a browser first; this is an eyeball call, not a computed one.@ -0,0 +87,4 @@// Two separate pins, not one cluster badge — the precondition everything below depends on, and// the thing a coordinate change in either journey step would silently take away.await Expect(page.Locator(".placemark-marker-icon")).ToHaveCountAsync(2);This is one screen-distance away from flaking.
AddPlaceAsyncplots at a fixed (51.5, -0.1);AddPlaceByClickingTheMapAsyncclicks a fixed (700, 300). Whether those two land more thanmaxClusterRadius: 80apart depends entirely on the default centre and zoom the home map opens at — change either and this collapses into one badge. It fails loudly rather than silently, which is why it is a note, but pinning the second point relative to the first pin's own bounding box would remove the coupling.@ -0,0 +265,4 @@const pinColour = getComputedStyle(probe).color;probe.remove();return [ring.borderTopColor, pinColour, ring.zIndex, ring.pointerEvents];The stacking-context premise is asserted nowhere. Every test reads only
getComputedStyle(pin, '::before').zIndex, which returns-1whether or not the icon element establishes a stacking context — so if Leaflet ever stopped writing a numericz-indexonto the marker icon, the ring would paint beneath the tile pane and all 14 tests here would stay green. That is the single load-bearing claim inleaflet-theme.css's own comment and the one thing nothing discriminates.Add
getComputedStyle(pin).zIndexto this array and assert it is not"auto"(ZIndexComputedStackingJourneyTestsis the precedent for reading a computed stacking property rather than a declared one).8d473a6f0ed3364114e3Verdict: mergeable
Re-reviewed at
d336411(previous verdict was against8d473a6, since force-pushed). Nothing to action.The gating finding is closed correctly.
ReadRingAsyncnow returnsgetComputedStyle(pin).zIndex— read from.placemark-marker-icon, which is the icon element the::beforeis generated on and therefore exactly the element that has to be the containing stacking context, not the pseudo-element.ShouldNotBe("auto")is the right strength rather than a lucky one: a positioned element with any numericz-indexestablishes a stacking context,0included, so the assertion admits every value that works and rejects only the one that does not. Both colour-scheme tests carry it throughAssertRingMatchesThePinAsync.The mutation is honest.
z-index: auto !importanton.leaflet-container .placemark-marker-icontouches one property on one class: it cannot reachcontent(soWaitForRingAsyncstill gets past and the test reaches the assertion rather than timing out early), nor the custom property, the border colour orpointer-events, and with a single marker on the map there is no marker-ordering side effect either. The reddening is attributable toPinZIndexand nothing else.One thing to have on the record, not to act on: Leaflet also writes a
transformonto the icon, which independently creates a stacking context, so the mutation proved the tripwire fires rather than reproducing an actual ring-under-the-tiles. That makes the assertion conservative — it would cry wolf before it would miss a regression, which is the safe direction. Its one unstated premise is that the icon staysposition: absolute(leaflet.css); if it ever weren't positioned a numericz-indexwould be inert, but the ring's geometry would visibly break first.Rebase is clean and nothing regressed with it. Merge base is
b3e1239; themap.jsandleaflet-theme.cssdiffs are byte-identical to the reviewed head (same blob hashes, same hunks), and every other file's additions and deletions are unchanged — onlySelectedPinRingJourneyTests.csgrew, by 12 lines. Every earlier mutation conclusion therefore still applies to code that did not move, independent of whether they were re-run. Two commits, subject-only messages, no AI attribution. CI #815 green on this head, and thee2ejob setsPLACEMARK_RUN_E2E_TESTSunconditionally, so the six Playwright tests genuinely ran rather than skipping.All three deliberate non-actions are reasonable. The inset hairline is a judgement that needs a screen, and widening it blind would be guessing; the coordinate coupling and the geometry gap are recorded where the next reader of those tests will find them.
Merge order: no reason this should not go first. It is the only one of the three that is green and already rebased on current
main. Whichever follows picks up the conflicts — #198 inHome.razor,LeafletMap.razor.csand theSetUpMapModulehelpers (itsmap.jshunks are appended at end of file, so those are clear), and #197 more awkwardly, since it deletes theZoomRequestwiring from the sameHome.razorcomment block and the sameOnParametersSetAsyncregion this addsSelectedMarkerIdto, and will want itscoverage-baseline.jsonfigure re-derived afterwards rather than carried over.