fix: judge order dependence by the area in force, close the stuck drag

Review CODE-REVIEW-220-r1.

H1: markersNeedingPlacement decided who depends on the order by reading
marker.area alone, while resolveExplicitMarkerPlacement reads
`marker.area || <area of the HA device>`. The ordinary marker — bind an HA
device, store neither field — is anchored by the registry and never depended
on the order, yet it was being written a space it never asked for. Dormant
today, and the day that HA area changes it moves the marker to whatever space
used to be first. The resolver now asks for the area actually in force.

M1: a mouse released past the panel left the gesture stuck, swallowing the
next click. Pointer capture is the usual answer and is now taken, but it is
not a guarantee — the browser grants it only for a live pointer. The window
listener is what actually closes the gesture.

M2: the fifth mutant from the spec is registered, plus a sixth for the stuck
drag above.

Writing the smoke for M1 turned up why the first attempt passed against
broken code: synthetic PointerEvents default to composed:false and never
leave the shadow root, so nothing outside the panel could ever hear them.
Real pointer events are composed; the smoke now says so.

Issue: #220
User-Visible: no
This commit is contained in:
Codex
2026-08-21 00:50:15 +03:00
parent 90a00670a7
commit a8aeecc32c
8 changed files with 143 additions and 17 deletions
+37 -3
View File
@@ -1268,6 +1268,20 @@ class HouseplanCard extends LitElement {
spaceCount: this._model.length,
fixedFloor: this._hasFixedFloor,
})) return;
// A mouse released past the edge of the panel fires neither pointerup nor
// pointercancel on any tab, and the gesture would stay stuck mid-drag —
// taking the next click with it, since _tabClick swallows clicks that
// follow a drag (review CODE-REVIEW-220-r1, M1).
//
// Capture is the usual answer and this file uses it everywhere, but it is
// not a guarantee: the browser grants it only for a live pointer, so a
// gesture that starts any other way keeps no capture at all. The window
// listener below is what actually closes the gesture; capture merely keeps
// the moves flowing to the tab while the button is held.
capturePointer(event);
this._tabDragRelease = (release: PointerEvent) => this._tabPointerUp(release);
window.addEventListener('pointerup', this._tabDragRelease);
window.addEventListener('pointercancel', this._tabDragRelease);
this._tabDrag = {
id, pointerId: event.pointerId, x: event.clientX, y: event.clientY,
moved: false, overId: id,
@@ -1287,11 +1301,21 @@ class HouseplanCard extends LitElement {
private _tabPointerUp(event: PointerEvent): void {
const drag = this._tabDrag;
this._tabDrag = null;
if (!drag || drag.pointerId !== event.pointerId || !drag.moved) return;
if (drag && drag.pointerId !== event.pointerId) return;
this._endTabDrag();
if (!drag || !drag.moved) return;
this._commitTabOrder(drag.id, drag.overId);
}
/** Drop the gesture and its window listeners, wherever the release happened. */
private _endTabDrag(): void {
this._tabDrag = null;
if (!this._tabDragRelease) return;
window.removeEventListener('pointerup', this._tabDragRelease);
window.removeEventListener('pointercancel', this._tabDragRelease);
this._tabDragRelease = null;
}
/** A click that followed a real drag must not also switch the space. */
private _tabClick(id: string): void {
if (this._tabDrag?.moved) return;
@@ -1313,12 +1337,19 @@ class HouseplanCard extends LitElement {
const ids = this._model.map((space) => space.id);
const order = reorderSpaceIds(ids, movedId, targetId);
if (order === ids) return;
// The area in force, not merely the one stored on the marker: a marker that
// binds an HA device inherits its area from the registry, and such a marker
// never depended on the order (review CODE-REVIEW-220-r1, H1).
const areaById = new Map(
this._devices.map((device) => [String(device.id), String(device.area || '')]),
);
const pinned = markersNeedingPlacement(
cfg.markers || [],
Object.fromEntries(
Object.entries(this._areaToSpace).map(([area, value]) => [area, value.space]),
),
ids[0] || '',
(markerId) => areaById.get(markerId) || '',
);
if (pinned.length) {
const byId = new Map(pinned.map((entry) => [entry.id, entry.space]));
@@ -1930,6 +1961,9 @@ class HouseplanCard extends LitElement {
id: string; pointerId: number; x: number; y: number; moved: boolean; overId: string;
} | null = null;
/** Window-level release handler while a tab is held; see _tabPointerDown. */
private _tabDragRelease: ((event: PointerEvent) => void) | null = null;
/** The positional-`floor` warning is worth saying once, not on every drop. */
private _tabOrderWarned = false;
private _lastTap = 0;
@@ -15860,7 +15894,7 @@ class HouseplanCard extends LitElement {
@pointerdown=${(e: PointerEvent) => this._tabPointerDown(e, s.id)}
@pointermove=${(e: PointerEvent) => this._tabPointerMove(e, s.id)}
@pointerup=${(e: PointerEvent) => this._tabPointerUp(e)}
@pointercancel=${() => { this._tabDrag = null; }}
@pointercancel=${() => this._endTabDrag()}
@click=${() => this._tabClick(s.id)}
>
${s.title}${this._norm && this._canEdit
+14 -2
View File
@@ -92,16 +92,27 @@ export interface PlacementMarker {
* Markers whose space is decided by the "first space" fallback, and where that
* fallback currently lands.
*
* Such a marker has neither an explicit `space` nor an `area` that names a
* Such a marker has neither an explicit `space` nor an area that names a
* space. Today it renders in whichever space happens to sit first; after a
* reorder that would be a different one — the marker would move on its own,
* which is the one thing a reorder may never do. Writing the answer it has
* right now makes the placement explicit and independent of order for good.
*
* **The area is not only the marker's own field.** `resolveExplicitMarkerPlacement`
* (`devices.ts`) reads `marker.area || <area of the HA device or entity>`, so a
* marker that simply binds an existing HA device — the ordinary case, saved
* without `area` or `space` — is anchored by the registry and never depended on
* the order at all. Judging by `marker.area` alone would classify it as
* order-dependent and write it a `space` it never asked for: a field that is
* dormant today and moves the marker the day its HA area changes. Hence
* `effectiveArea`, which answers with the area actually in force (review
* CODE-REVIEW-220-r1, H1).
*/
export function markersNeedingPlacement(
markers: readonly PlacementMarker[],
areaToSpace: Readonly<Record<string, string>>,
firstSpaceId: string,
effectiveArea: (markerId: string) => string = () => '',
): { id: string; space: string }[] {
if (!firstSpaceId) return [];
const out: { id: string; space: string }[] = [];
@@ -111,7 +122,8 @@ export function markersNeedingPlacement(
if (!id) continue;
const explicit = typeof marker.space === 'string' ? marker.space : '';
if (explicit) continue;
const area = typeof marker.area === 'string' ? marker.area : '';
const own = typeof marker.area === 'string' ? marker.area : '';
const area = own || effectiveArea(id) || '';
if (area && areaToSpace[area]) continue;
out.push({ id, space: firstSpaceId });
}