Skip to content

Commit

Permalink
Fix issue #935 (#954)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>
  • Loading branch information
prushforth and AliyanH authored Apr 10, 2024
1 parent 263d14a commit d2f7bec
Show file tree
Hide file tree
Showing 9 changed files with 341 additions and 27 deletions.
59 changes: 37 additions & 22 deletions src/map-extent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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();
Expand All @@ -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);
});
Expand All @@ -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
});
Expand Down Expand Up @@ -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);
Expand All @@ -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');
Expand Down
47 changes: 45 additions & 2 deletions src/mapml/control/LayerControl.js
Original file line number Diff line number Diff line change
Expand Up @@ -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...
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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;
Expand Down
80 changes: 80 additions & 0 deletions test/e2e/core/drag.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
Binary file added test/e2e/elements/map-extent/Img_Sample.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
92 changes: 92 additions & 0 deletions test/e2e/elements/map-extent/map-extent-checked-ordering.test.js
Original file line number Diff line number Diff line change
@@ -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
});
});
Loading

0 comments on commit d2f7bec

Please sign in to comment.