Skip to content

Commit 71dbec1

Browse files
committed
Backport improvements that were created during creation of gcds-map:
- Implement layer "registry" for child (Leaflet) layers in BaseLayerElement, HTMLFeatureElement, and HTMLTileElement to optimize layer management and cleanup. - Update HTMLMapCaptionElement to ensure only the first caption manages the aria-label. - Refactor rendering logic in renderStyles.js to improve handling of SVG and container elements. - Adjust event handling in HTMLWebMapElement and HTMLLinkElement for better performance and reliability. - Update test cases to ensure proper navigation and loading states.
1 parent 292134d commit 71dbec1

18 files changed

Lines changed: 219 additions & 93 deletions

src/layer.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,9 @@ export class BaseLayerElement extends HTMLElement {
215215
lc.removeLayer(l);
216216
}
217217
// remove properties of layer involved in whenReady() logic
218+
if (this._layerRegistry) {
219+
this._layerRegistry.clear();
220+
}
218221
delete this._layer;
219222
delete this._layerControl;
220223
delete this._layerControlHTML;
@@ -228,6 +231,18 @@ export class BaseLayerElement extends HTMLElement {
228231
/* jshint ignore:start */
229232
this.#hasConnected = true;
230233
/* jshint ignore:end */
234+
// _layerRegistry tracks child MapFeatureLayer and MapTileLayer instances
235+
// by their calculated DOM position (z-index). Adjacent <map-feature> or
236+
// <map-tile> elements share a single Leaflet layer; the first in a sequence
237+
// creates it and registers here with count=1. Subsequent siblings look up
238+
// the entry and increment count. On disconnect, count is decremented;
239+
// when it reaches 0 the entry is deleted and the Leaflet layer is removed.
240+
// This replaces fragile DOM sibling traversal (getPrevious()) with O(1)
241+
// lookup and proper reference counting for cleanup.
242+
// map-link.js uses the same pattern for its templated child layers.
243+
if (!this._layerRegistry) {
244+
this._layerRegistry = new Map();
245+
}
231246
this._createLayerControlHTML = createLayerControlHTML.bind(this);
232247
const doConnected = this._onAdd.bind(this);
233248
const doRemove = this._onRemove.bind(this);

src/map-caption.js

Lines changed: 34 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -13,37 +13,47 @@ export class HTMLMapCaptionElement extends HTMLElement {
1313
this.parentElement.nodeName === 'MAPML-VIEWER' ||
1414
this.parentElement.nodeName === 'MAP'
1515
) {
16-
// calls MutationObserver; needed to observe changes to content between <map-caption> tags and update to aria-label
17-
let mapcaption =
18-
this.parentElement.querySelector('map-caption').textContent;
16+
setTimeout(() => {
17+
// Only the first map-caption child should manage the aria-label
18+
const firstMapCaption = this.parentElement.querySelector('map-caption');
19+
const isFirstCaption = firstMapCaption === this;
1920

20-
this.observer = new MutationObserver(() => {
21-
let mapcaptionupdate =
21+
if (!isFirstCaption) {
22+
return;
23+
}
24+
25+
// calls MutationObserver; needed to observe changes to content between <map-caption> tags and update to aria-label
26+
let mapcaption =
2227
this.parentElement.querySelector('map-caption').textContent;
2328

24-
if (mapcaptionupdate !== mapcaption) {
25-
this.parentElement.setAttribute(
26-
'aria-label',
27-
this.parentElement.querySelector('map-caption').textContent
28-
);
29-
}
30-
});
29+
this.observer = new MutationObserver(() => {
30+
let mapcaptionupdate =
31+
this.parentElement.querySelector('map-caption').textContent;
3132

32-
this.observer.observe(this, {
33-
characterData: true,
34-
subtree: true,
35-
attributes: true,
36-
childList: true
37-
});
33+
if (mapcaptionupdate !== mapcaption) {
34+
this.parentElement.setAttribute(
35+
'aria-label',
36+
this.parentElement.querySelector('map-caption').textContent
37+
);
38+
}
39+
});
3840

39-
// don't change aria-label if one already exists from user (checks when element is first created)
40-
if (!this.parentElement.hasAttribute('aria-label')) {
41-
const ariaLabel = this.textContent;
42-
this.parentElement.setAttribute('aria-label', ariaLabel);
43-
}
41+
this.observer.observe(this, {
42+
characterData: true,
43+
subtree: true,
44+
attributes: true,
45+
childList: true
46+
});
47+
48+
// don't change aria-label if one already exists from user (checks when element is first created)
49+
if (!this.parentElement.hasAttribute('aria-label')) {
50+
const ariaLabel = this.textContent;
51+
this.parentElement.setAttribute('aria-label', ariaLabel);
52+
}
53+
}, 0);
4454
}
4555
}
4656
disconnectedCallback() {
47-
this.observer.disconnect();
57+
if (this.observer) this.observer.disconnect();
4858
}
4959
}

src/map-feature.js

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,14 @@ export class HTMLFeatureElement extends HTMLElement {
221221
)
222222
return;
223223
this._observer.disconnect();
224+
// Decrement layer registry count
225+
const entry = this._parentEl?._layerRegistry?.get(this.position);
226+
if (entry) {
227+
entry.count--;
228+
if (entry.count === 0) {
229+
this._parentEl._layerRegistry.delete(this.position);
230+
}
231+
}
224232
if (this._featureLayer) {
225233
this.removeFeature(this._featureLayer);
226234
// If this was the last feature in the layer, clean up the layer
@@ -290,16 +298,7 @@ export class HTMLFeatureElement extends HTMLElement {
290298
this._setUpEvents();
291299
}
292300
isFirst() {
293-
// Get the previous element sibling
294-
const prevSibling = this.previousElementSibling;
295-
296-
// If there's no previous sibling, return true
297-
if (!prevSibling) {
298-
return true;
299-
}
300-
301-
// Compare the node names (tag names) - return true if they're different
302-
return this.nodeName !== prevSibling.nodeName;
301+
return !this._parentEl._layerRegistry.has(this.position);
303302
}
304303
getPrevious() {
305304
// Check if this is the first element of a sequence
@@ -370,13 +369,23 @@ export class HTMLFeatureElement extends HTMLElement {
370369
})
371370
});
372371

372+
// Register in parent's layer registry
373+
parentElement._layerRegistry.set(this.position, {
374+
layer: this._featureLayer,
375+
count: 1
376+
});
377+
373378
this.addFeature(this._featureLayer);
374379

375380
// add MapFeatureLayer to appropriate parent layer
376381
parentLayer.addLayer(this._featureLayer);
377382
} else {
378-
// get the previous feature's layer
379-
this._featureLayer = this.getPrevious()?._featureLayer;
383+
// Look up the existing layer from the registry
384+
const entry = this._parentEl._layerRegistry.get(this.position);
385+
this._featureLayer = entry?.layer;
386+
if (entry) {
387+
entry.count++;
388+
}
380389
if (this._featureLayer) {
381390
this.addFeature(this._featureLayer);
382391
}

src/map-link.js

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,10 @@ export class HTMLLinkElement extends HTMLElement {
271271
(this.parentExtent && this.parentExtent.hasAttribute('data-moving'))
272272
)
273273
return;
274+
// Layer registry for child map-feature/map-tile layers — see layer.js
275+
if (!this._layerRegistry) {
276+
this._layerRegistry = new Map();
277+
}
274278
switch (this.rel.toLowerCase()) {
275279
// for some cases, require a dependency check
276280
case 'tile':
@@ -321,6 +325,9 @@ export class HTMLLinkElement extends HTMLElement {
321325
// what else?
322326
}
323327
disconnectedCallback() {
328+
if (this._layerRegistry) {
329+
this._layerRegistry.clear();
330+
}
324331
switch (this.rel.toLowerCase()) {
325332
case 'stylesheet':
326333
if (this._stylesheetHost) {
@@ -453,12 +460,17 @@ export class HTMLLinkElement extends HTMLElement {
453460
this.link.setAttribute('href', new URL(this.href, this.getBase()).href);
454461
copyAttributes(this, this.link);
455462

456-
if (this._stylesheetHost._layer) {
463+
// IMPORTANT: Only render if the correct container exists for THIS element's parent.
464+
// Don't fall back to _extentLayer if this is a layer child - wait for _layer instead.
465+
const isLayerChild = this._stylesheetHost.tagName === 'MAP-LAYER';
466+
const isExtentChild = this._stylesheetHost.tagName === 'MAP-EXTENT';
467+
468+
if (isLayerChild && this._stylesheetHost._layer) {
457469
this._stylesheetHost._layer.renderStyles(this);
470+
} else if (isExtentChild && this._stylesheetHost._extentLayer) {
471+
this._stylesheetHost._extentLayer.renderStyles(this);
458472
} else if (this._stylesheetHost._templatedLayer) {
459473
this._stylesheetHost._templatedLayer.renderStyles(this);
460-
} else if (this._stylesheetHost._extentLayer) {
461-
this._stylesheetHost._extentLayer.renderStyles(this);
462474
}
463475
}
464476

src/map-style.js

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,17 @@ export class HTMLStyleElement extends HTMLElement {
7070
this.styleElement.mapStyle = this;
7171
this.styleElement.textContent = this.textContent;
7272
copyAttributes(this, this.styleElement);
73-
if (this._stylesheetHost._layer) {
73+
// IMPORTANT: Only render if the correct container exists for THIS element's parent.
74+
// Don't fall back to _extentLayer if this is a layer child - wait for _layer instead.
75+
const isLayerChild = this._stylesheetHost.tagName === 'MAP-LAYER';
76+
const isExtentChild = this._stylesheetHost.tagName === 'MAP-EXTENT';
77+
78+
if (isLayerChild && this._stylesheetHost._layer) {
7479
this._stylesheetHost._layer.renderStyles(this);
80+
} else if (isExtentChild && this._stylesheetHost._extentLayer) {
81+
this._stylesheetHost._extentLayer.renderStyles(this);
7582
} else if (this._stylesheetHost._templatedLayer) {
7683
this._stylesheetHost._templatedLayer.renderStyles(this);
77-
} else if (this._stylesheetHost._extentLayer) {
78-
this._stylesheetHost._extentLayer.renderStyles(this);
7984
}
8085

8186
function copyAttributes(source, target) {

src/map-tile.js

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,14 @@ export class HTMLTileElement extends HTMLElement {
135135
}
136136

137137
disconnectedCallback() {
138+
// Decrement layer registry count
139+
const entry = this._parentEl?._layerRegistry?.get(this.position);
140+
if (entry) {
141+
entry.count--;
142+
if (entry.count === 0) {
143+
this._parentEl._layerRegistry.delete(this.position);
144+
}
145+
}
138146
// If this is a map-tile connected to a tile layer, remove it from the layer
139147
if (this._tileLayer) {
140148
this._tileLayer.removeMapTile(this);
@@ -148,16 +156,7 @@ export class HTMLTileElement extends HTMLElement {
148156
}
149157
}
150158
isFirst() {
151-
// Get the previous element sibling
152-
const prevSibling = this.previousElementSibling;
153-
154-
// If there's no previous sibling, return true
155-
if (!prevSibling) {
156-
return true;
157-
}
158-
159-
// Compare the node names (tag names) - return true if they're different
160-
return this.nodeName !== prevSibling.nodeName;
159+
return !this._parentEl._layerRegistry.has(this.position);
161160
}
162161
getPrevious() {
163162
// Check if this is the first element of a sequence
@@ -249,6 +248,12 @@ export class HTMLTileElement extends HTMLElement {
249248
});
250249
this._tileLayer.addMapTile(this);
251250

251+
// Register in parent's layer registry
252+
parentElement._layerRegistry.set(this.position, {
253+
layer: this._tileLayer,
254+
count: 1
255+
});
256+
252257
// add MapTileLayer to TemplatedFeaturesOrTilesLayer of the MapLink
253258
if (parentElement._templatedLayer?.addLayer) {
254259
parentElement._templatedLayer.addLayer(this._tileLayer);
@@ -257,8 +262,12 @@ export class HTMLTileElement extends HTMLElement {
257262
parentElement._layer.addLayer(this._tileLayer);
258263
}
259264
} else {
260-
// get the previous tile's layer
261-
this._tileLayer = this.getPrevious()?._tileLayer;
265+
// Look up the existing layer from the registry
266+
const entry = parentElement._layerRegistry.get(this.position);
267+
this._tileLayer = entry?.layer;
268+
if (entry) {
269+
entry.count++;
270+
}
262271
if (this._tileLayer) {
263272
this._tileLayer.addMapTile(this);
264273
}

src/mapml-viewer.js

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -420,6 +420,16 @@ export class HTMLMapmlViewerElement extends HTMLElement {
420420
layersReady.push(reAttach.whenReady());
421421
}
422422
return Promise.allSettled(layersReady).then(() => {
423+
// For single-layer case (e.g. link traversal), synchronously set
424+
// zoom constraints so that layer.zoomTo() uses correct bounds
425+
const layers = this.querySelectorAll('map-layer,layer-');
426+
if (layers.length === 1) {
427+
const layer = layers[0];
428+
if (layer.extent) {
429+
this._map.setMinZoom(layer.extent.zoom.minZoom);
430+
this._map.setMaxZoom(layer.extent.zoom.maxZoom);
431+
}
432+
}
423433
// use the saved map location to ensure it is correct after
424434
// changing the map CRS. Specifically affects projection
425435
// upgrades, e.g. https://maps4html.org/experiments/custom-projections/BNG/
@@ -660,13 +670,19 @@ export class HTMLMapmlViewerElement extends HTMLElement {
660670
_removeEvents() {
661671
if (this._map) {
662672
this._map.off();
663-
this.removeEventListener('drop', this._dropHandler, false);
664-
this.removeEventListener('dragover', this._dragoverHandler, false);
673+
if (this._boundDropHandler) {
674+
this.removeEventListener('drop', this._boundDropHandler, false);
675+
}
676+
if (this._boundDragoverHandler) {
677+
this.removeEventListener('dragover', this._boundDragoverHandler, false);
678+
}
665679
}
666680
}
667681
_setUpEvents() {
668-
this.addEventListener('drop', this._dropHandler, false);
669-
this.addEventListener('dragover', this._dragoverHandler, false);
682+
this._boundDropHandler = this._dropHandler.bind(this);
683+
this._boundDragoverHandler = this._dragoverHandler.bind(this);
684+
this.addEventListener('drop', this._boundDropHandler, false);
685+
this.addEventListener('dragover', this._boundDragoverHandler, false);
670686
this.addEventListener(
671687
'change',
672688
function (e) {

src/mapml.css

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,11 @@ and: https://developer.mozilla.org/en-US/docs/Web/CSS/:fullscreen */
447447
text-shadow: 1px 1px 1px #000, 1px 1px 1px #000;
448448
}
449449

450+
/* Ensure white background persists in fullscreen */
451+
:host(.mapml-fullscreen-on) {
452+
background-color: white;
453+
}
454+
450455
/*
451456
* User interaction.
452457
*/

src/mapml/elementSupport/layers/renderStyles.js

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,19 @@ export function renderStyles(mapStyleOrLink) {
1010

1111
// If there are no rendered styles or links yet, insert before any content
1212
if (renderedSiblingStyles.length === 0) {
13-
return this._container.lastChild &&
14-
(this._container.lastChild.nodeName.toUpperCase() === 'SVG' ||
15-
this._container.lastChild.classList.contains(
16-
'mapml-vector-container'
17-
))
18-
? { position: 'beforebegin', node: this._container.lastChild }
19-
: this._container.lastChild
20-
? { position: 'afterend', node: this._container.lastChild }
21-
: { position: 'afterbegin', node: this._container };
13+
const lastChild = this._container.lastChild;
14+
if (!lastChild) {
15+
return { position: 'afterbegin', node: this._container };
16+
}
17+
18+
const isSVG = lastChild.nodeName === 'SVG';
19+
const isContainer =
20+
lastChild.classList?.contains('mapml-vector-container') ||
21+
lastChild.classList?.contains('mapml-extentlayer-container');
22+
23+
return isSVG || isContainer
24+
? { position: 'beforebegin', node: lastChild }
25+
: { position: 'afterend', node: lastChild };
2226
}
2327

2428
// Peek into the light DOM context for comparison

src/mapml/layers/MapFeatureLayer.js

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -410,12 +410,18 @@ export var MapFeatureLayer = FeatureGroup.extend({
410410
!geometry._map
411411
) {
412412
this.addRendering(geometry);
413-
// update the layerbounds
414-
let placeholder =
415-
geometry.defaultOptions.group.parentNode.querySelector(
416-
`span[id="${geometry._leaflet_id}"]`
417-
);
418-
placeholder.replaceWith(geometry.defaultOptions.group);
413+
// Only try to find placeholder if the geometry group has a parentNode
414+
// If parentNode is null, the geometry is being handled by reRender()
415+
// which will attach it to the DOM after _validateRendering completes
416+
if (geometry.defaultOptions.group.parentNode) {
417+
let placeholder =
418+
geometry.defaultOptions.group.parentNode.querySelector(
419+
`span[id="${geometry._leaflet_id}"]`
420+
);
421+
if (placeholder) {
422+
placeholder.replaceWith(geometry.defaultOptions.group);
423+
}
424+
}
419425
}
420426
}
421427
}

0 commit comments

Comments
 (0)