Fix issue #935 (#954)

* Fix issue #935
* Add test / test data for map-extent issue. Closes #935
* Fix Layer reordering bug. closes #955
Update LayerControl._layers object which was being neglected and not being updated. Added a line to properly sort the layerControl based on the MapML Layer zIndex.
* Update call to sortFunction in copied / locally modified LayerControl.
_update function to use the options.sortFunction if present.  tbd:
if this should be offered via PR to upstream project.
* Add test for re-order bug #955

---------

Co-authored-by: AliyanH <aliyanulhaq@gmail.com>

Author: prushforth

src/map-extent.js

@@ -176,7 +176,13 @@ export class MapExtent extends HTMLElement {
                 let extentsRootFieldset =
                   this.parentLayer._propertiesGroupAnatomy;
                 let position = Array.from(
-                  this.parentNode.querySelectorAll('map-extent:not([hidden])')
+                  this.parentLayer.src
+                    ? this.parentLayer.shadowRoot.querySelectorAll(
+                        ':host > map-extent:not([hidden])'
+                      )
+                    : this.parentLayer.querySelectorAll(
+                        ':scope > map-extent:not([hidden])'
+                      )
                 ).indexOf(this);
                 if (newValue !== null) {
                   // remove from layer control (hide from user)
@@ -190,12 +196,18 @@ export class MapExtent extends HTMLElement {
                       this._layerControlHTML
                     );
                   } else if (position > 0) {
-                    this.parentNode
-                      .querySelectorAll('map-extent:not([hidden])')
-                      [position - 1]._layerControlHTML.insertAdjacentElement(
-                        'afterend',
-                        this._layerControlHTML
-                      );
+                    Array.from(
+                      this.parentLayer.src
+                        ? this.parentLayer.shadowRoot.querySelectorAll(
+                            ':host > map-extent:not([hidden])'
+                          )
+                        : this.parentLayer.querySelectorAll(
+                            ':scope > map-extent:not([hidden])'
+                          )
+                    )[position - 1]._layerControlHTML.insertAdjacentElement(
+                      'afterend',
+                      this._layerControlHTML
+                    );
                   }
                 }
                 this._validateLayerControlContainerHidden();
@@ -220,16 +232,13 @@ export class MapExtent extends HTMLElement {
   async connectedCallback() {
     // this.parentNode.host returns the layer- element when parentNode is
     // the shadow root
-    this.parentLayer =
-      this.parentNode.nodeName.toUpperCase() === 'LAYER-'
-        ? this.parentNode
-        : this.parentNode.host;
+    this.parentLayer = this.getLayerEl();
     if (
       this.hasAttribute('data-moving') ||
       this.parentLayer.hasAttribute('data-moving')
     )
       return;
-    this.mapEl = this.parentLayer.closest('mapml-viewer,map[is=web-map]');
+    this.mapEl = this.getMapEl();
     await this.mapEl.whenProjectionDefined(this.units).catch(() => {
       throw new Error('Undefined projection:' + this.units);
     });
@@ -252,7 +261,9 @@ export class MapExtent extends HTMLElement {
       opacity: this.opacity,
       crs: M[this.units],
       extentZIndex: Array.from(
-        this.parentLayer.querySelectorAll('map-extent')
+        this.parentLayer.src
+          ? this.parentLayer.shadowRoot.querySelectorAll(':host > map-extent')
+          : this.parentLayer.querySelectorAll(':scope > map-extent')
       ).indexOf(this),
       extentEl: this
     });
@@ -418,9 +429,11 @@ export class MapExtent extends HTMLElement {
       // can be added to mapmllayer layerGroup no matter layer- is checked or not
       this._extentLayer.addTo(this.parentLayer._layer);
       this._extentLayer.setZIndex(
-        Array.from(this.parentLayer.querySelectorAll('map-extent')).indexOf(
-          this
-        )
+        Array.from(
+          this.parentLayer.src
+            ? this.parentLayer.shadowRoot.querySelectorAll(':host > map-extent')
+            : this.parentLayer.querySelectorAll(':scope > map-extent')
+        ).indexOf(this)
       );
     } else {
       this.parentLayer._layer?.removeLayer(this._extentLayer);
@@ -430,13 +443,15 @@ export class MapExtent extends HTMLElement {
   }
   _validateLayerControlContainerHidden() {
     let extentsFieldset = this.parentLayer._propertiesGroupAnatomy;
-    let nodeToSearch = this.parentLayer.src
-      ? this.parentLayer.shadowRoot
-      : this.parentLayer;
     if (!extentsFieldset) return;
-    if (
-      nodeToSearch.querySelectorAll('map-extent:not([hidden])').length === 0
-    ) {
+    const numberOfVisibleSublayers = (
+      this.parentLayer.src
+        ? this.parentLayer.shadowRoot.querySelectorAll(
+            ':host > map-extent:not([hidden])'
+          )
+        : this.parentLayer.querySelectorAll(':scope > map-extent:not([hidden])')
+    ).length;
+    if (numberOfVisibleSublayers === 0) {
       extentsFieldset.setAttribute('hidden', '');
     } else {
       extentsFieldset.removeAttribute('hidden');

src/mapml/control/LayerControl.js

@@ -98,9 +98,52 @@ export var LayerControl = L.Control.Layers.extend({
     }
   },
 
-  _withinZoomBounds: function (zoom, range) {
-    return range.min <= zoom && zoom <= range.max;
+  // imported from leaflet with slight modifications
+  // for layerControl ordering based on zIndex
+  _update: function () {
+    if (!this._container) {
+      return this;
+    }
+
+    L.DomUtil.empty(this._baseLayersList);
+    L.DomUtil.empty(this._overlaysList);
+
+    this._layerControlInputs = [];
+    var baseLayersPresent,
+      overlaysPresent,
+      i,
+      obj,
+      baseLayersCount = 0;
+
+    // <----------- MODIFICATION from the default _update method
+    // sort the layercontrol layers object based on the zIndex
+    // provided by MapMLLayer
+    if (this.options.sortLayers) {
+      this._layers.sort((a, b) =>
+        this.options.sortFunction(a.layer, b.layer, a.name, b.name)
+      );
+    }
+    // -------------------------------------------------->
+    for (i = 0; i < this._layers.length; i++) {
+      obj = this._layers[i];
+      this._addItem(obj);
+      overlaysPresent = overlaysPresent || obj.overlay;
+      baseLayersPresent = baseLayersPresent || !obj.overlay;
+      baseLayersCount += !obj.overlay ? 1 : 0;
+    }
+
+    // Hide base layers section if there's only one layer.
+    if (this.options.hideSingleBase) {
+      baseLayersPresent = baseLayersPresent && baseLayersCount > 1;
+      this._baseLayersList.style.display = baseLayersPresent ? '' : 'none';
+    }
+
+    this._separator.style.display =
+      overlaysPresent && baseLayersPresent ? '' : 'none';
+
+    return this;
   },
+
   _addItem: function (obj) {
     var layercontrols = obj.layer._layerEl._layerControlHTML;
     // the input is required by Leaflet...

src/mapml/elementSupport/extents/createLayerControlForExtent.js

@@ -31,8 +31,8 @@ export var createLayerControlExtentHTML = function () {
       extentSettings
     ),
     extentOpacitySummary = L.DomUtil.create('summary', '', opacityControl),
-    mapEl = this.parentLayer.parentNode,
-    layerEl = this.parentLayer,
+    mapEl = this.getMapEl(),
+    layerEl = this.getLayerEl(),
     opacity = L.DomUtil.create('input', '', opacityControl);
   extentSettings.hidden = true;
   extent.setAttribute('aria-grabbed', 'false');
@@ -227,7 +227,8 @@ export var createLayerControlExtentHTML = function () {
             let extentEl = c.querySelector('span').extent;
 
             extentEl.setAttribute('data-moving', '');
-            layerEl.insertAdjacentElement('beforeend', extentEl);
+            const node = layerEl.src ? layerEl.shadowRoot : layerEl;
+            node.append(extentEl);
             extentEl.removeAttribute('data-moving');
 
             extentEl.extentZIndex = zIndex;

test/e2e/core/drag.test.js

@@ -128,4 +128,84 @@ test.describe('UI Drag&Drop Test', () => {
     expect(layerIndex).toEqual('1');
     expect(controlText).toBe('Static MapML with tiles');
   });
+  test('Re-order checked bug (#955) test', async () => {
+    await page.waitForTimeout(500);
+    const layerControl = page.locator('.leaflet-top.leaflet-right');
+    await layerControl.hover();
+    const overlaysList = page.locator('.leaflet-control-layers-overlays');
+    const startingLowestLayer = overlaysList
+      .getByRole('group', { name: 'Canada Base Map - Transportation (CBMT)' })
+      .first();
+
+    // assert that CBMT is first of three layers
+    const cbmtIsFirstInLayerControl = await overlaysList.evaluate((l) => {
+      return (
+        l.firstElementChild.querySelector('.mapml-layer-item-name')
+          .textContent === 'Canada Base Map - Transportation (CBMT)'
+      );
+    });
+    expect(cbmtIsFirstInLayerControl).toBe(true);
+
+    const fromBox = await startingLowestLayer.boundingBox();
+    const fromPos = {
+      x: fromBox.x + fromBox.width / 2,
+      y: fromBox.y + fromBox.height / 3
+    };
+    let toPos = { x: fromPos.x, y: fromPos.y + fromBox.height * 1.1 };
+
+    // move (drag/drop) CBMT to the top of the layer stack
+    await page.mouse.move(fromPos.x, fromPos.y);
+    await page.mouse.down();
+    await page.mouse.move(toPos.x, toPos.y);
+    await page.mouse.up();
+    await page.mouse.move(toPos.x, toPos.y);
+    toPos = { x: toPos.x, y: toPos.y + fromBox.height * 1.1 };
+    await page.mouse.down();
+    await page.mouse.move(toPos.x, toPos.y);
+    await page.mouse.up();
+
+    let cbmtIsTopOfLayerControl = await overlaysList.evaluate((l) => {
+      return (
+        l.querySelectorAll(':scope > fieldset').length === 3 &&
+        l.lastElementChild.querySelector('.mapml-layer-item-name')
+          .textContent === 'Canada Base Map - Transportation (CBMT)'
+      );
+    });
+    // assert that CBMT is third of three layers
+    expect(cbmtIsTopOfLayerControl).toBe(true);
+
+    const cbmtCheckbox = overlaysList
+      .getByRole('checkbox', {
+        name: 'Canada Base Map - Transportation (CBMT)'
+      })
+      .first();
+    let cbmtIsChecked = await cbmtCheckbox.isChecked();
+    expect(cbmtIsChecked).toBe(true);
+    await page.pause();
+    await cbmtCheckbox.click();
+    cbmtIsChecked = await cbmtCheckbox.isChecked();
+    expect(cbmtIsChecked).toBe(false);
+    // cbmt layer should still be on top of layer control despite that it's unchecked
+    cbmtIsTopOfLayerControl = await overlaysList.evaluate((l) => {
+      return (
+        l.querySelectorAll(':scope > fieldset').length === 3 &&
+        l.lastElementChild.querySelector('.mapml-layer-item-name')
+          .textContent === 'Canada Base Map - Transportation (CBMT)'
+      );
+    });
+    // assert that CBMT is still third of three layers
+    expect(cbmtIsTopOfLayerControl).toBe(true);
+    await cbmtCheckbox.click();
+    cbmtIsChecked = await cbmtCheckbox.isChecked();
+    expect(cbmtIsChecked).toBe(true);
+    cbmtIsTopOfLayerControl = await overlaysList.evaluate((l) => {
+      return (
+        l.querySelectorAll(':scope > fieldset').length === 3 &&
+        l.lastElementChild.querySelector('.mapml-layer-item-name')
+          .textContent === 'Canada Base Map - Transportation (CBMT)'
+      );
+    });
+    // assert that CBMT is still third of three layers
+    expect(cbmtIsTopOfLayerControl).toBe(true);
+  });
 });

test/e2e/elements/map-extent/Img_Sample.png

@@ -0,0 +1,92 @@
+import { test, expect, chromium } from '@playwright/test';
+
+test.describe('map-extent checked order tests', () => {
+  let page;
+  let context;
+  test.beforeAll(async function () {
+    context = await chromium.launchPersistentContext('', { slowMo: 500 });
+    page =
+      context.pages().find((page) => page.url() === 'about:blank') ||
+      (await context.newPage());
+    await page.goto('map-extent-checked.html');
+  });
+  test('map-extent layer control order correct when cycling checked state', async () => {
+    // Fixed #935 https://github.com/Maps4HTML/Web-Map-Custom-Element/issues/935
+    /* 
+Go to this map map-extent-checked.html
+
+Open the layer control for the layer settings.
+
+Un-check the imagery layer <map-extent>
+Check the imagery layer <map-extent>
+
+What should happen:
+The imagery layer <map-extent> should draw underneath the states layer.
+
+What actually happens:
+The imagery layer <map-extent> draws on top of the states layer.
+     * */
+    const layerControl = page.locator('.leaflet-top.leaflet-right');
+    await layerControl.hover();
+    const layerSettings = layerControl.getByTitle('Layer Settings');
+    await layerSettings.click();
+    const imageryExtentCheckbox = layerControl.getByRole('checkbox', {
+      name: 'Extent One'
+    });
+    await imageryExtentCheckbox.click(); // turn it off
+    await imageryExtentCheckbox.click(); // turn it on
+    const ext1 = page.getByTestId('ext1');
+    let imageryZIndex = await ext1.evaluate((e) => {
+      return +e._extentLayer._container.style.zIndex;
+    });
+    expect(imageryZIndex).toEqual(0);
+    const ext2 = page.getByTestId('ext2');
+    let statesZIndex = await ext2.evaluate((e) => {
+      return +e._extentLayer._container.style.zIndex;
+    });
+    expect(statesZIndex).toEqual(1);
+    // re-order them via the layer control
+    const imageryFieldset = layerControl.getByRole('group', {
+      name: 'Extent One'
+    });
+    let controlBBox = await imageryFieldset.boundingBox();
+    let from = {
+        x: controlBBox.x + controlBBox.width / 2,
+        y: controlBBox.y + controlBBox.height / 2
+      },
+      to = { x: from.x, y: from.y + controlBBox.height * 1.1 };
+
+    await page.mouse.move(from.x, from.y);
+    await page.mouse.down();
+    await page.mouse.move(to.x, to.y);
+    await page.mouse.up();
+    imageryZIndex = await ext1.evaluate((e) => {
+      return +e._extentLayer._container.style.zIndex;
+    });
+    expect(imageryZIndex).toEqual(1);
+    statesZIndex = await ext2.evaluate((e) => {
+      return +e._extentLayer._container.style.zIndex;
+    });
+    expect(statesZIndex).toEqual(0);
+
+    await page.mouse.move(from.x, from.y);
+    await page.mouse.down();
+    await page.mouse.move(to.x, to.y);
+    await page.mouse.up();
+    await imageryExtentCheckbox.click(); // turn it off
+    await imageryExtentCheckbox.click(); // turn it on
+    imageryZIndex = await ext1.evaluate((e) => {
+      return +e._extentLayer._container.style.zIndex;
+    });
+    expect(imageryZIndex).toEqual(0);
+    statesZIndex = await ext2.evaluate((e) => {
+      return +e._extentLayer._container.style.zIndex;
+    });
+    expect(statesZIndex).toEqual(1);
+    // TO DO re-order them via the DOM (insertAdjacentHTML),
+    // ensure that
+    // a) render order/z-index is correct
+    // b) render order is reflected in layer control order as well
+    // see https://github.com/Maps4HTML/Web-Map-Custom-Element/issues/956
+  });
+});