Skip to content

Fix search control bug #1050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
prushforth wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix search control bug #1050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 4 additions & 12 deletions src/mapml-viewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,13 +502,10 @@ export class HTMLMapmlViewerElement extends HTMLElement {
})
.addTo(this._map);
}
if (
!this._searchButton &&
this._controlsList &&
this._controlsList.contains('search') &&
totalSize + 49 <= mapSize
) {
totalSize += 49;
if (!this._searchButton) {
// Note: search is opt-in (default hidden) so it occupies no
// vertical space until enabled by controlslist="search"; do not
// charge it against the mapSize budget here.
this._searchButton = searchButton({ mapEl: this }).addTo(this._map);
}
if (!this._reloadButton && totalSize + 49 <= mapSize) {
Expand Down Expand Up @@ -577,11 +574,6 @@ export class HTMLMapmlViewerElement extends HTMLElement {
this._setControlsVisibility('geolocation', false);
break;
case 'search':
if (!this._searchButton) {
this._searchButton = searchButton({ mapEl: this }).addTo(
this._map
);
}
this._setControlsVisibility('search', false);
break;
case 'noscale':
Expand Down
16 changes: 4 additions & 12 deletions src/web-map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,13 +544,10 @@ export class HTMLWebMapElement extends HTMLMapElement {
})
.addTo(this._map);
}
if (
!this._searchButton &&
this._controlsList &&
this._controlsList.contains('search') &&
totalSize + 49 <= mapSize
) {
totalSize += 49;
if (!this._searchButton) {
// Note: search is opt-in (default hidden) so it occupies no
// vertical space until enabled by controlslist="search"; do not
// charge it against the mapSize budget here.
this._searchButton = searchButton({ mapEl: this }).addTo(this._map);
}
if (!this._reloadButton && totalSize + 49 <= mapSize) {
Expand Down Expand Up @@ -619,11 +616,6 @@ export class HTMLWebMapElement extends HTMLMapElement {
this._setControlsVisibility('geolocation', false);
break;
case 'search':
if (!this._searchButton) {
this._searchButton = searchButton({ mapEl: this }).addTo(
this._map
);
}
this._setControlsVisibility('search', false);
break;
case 'noscale':
Expand Down
20 changes: 15 additions & 5 deletions test/e2e/api/domApi-mapml-viewer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,12 @@ test.describe('mapml-viewer DOM API Tests', () => {
'.leaflet-top.leaflet-left',
(div) => div.childElementCount
);
expect(leftControlCount).toBe(2);
// 3 controls in DOM: zoom, search (always created up-front for
// correct DOM ordering but hidden because controlslist does not
// contain "search"), and reload. Fullscreen is squeezed out by the
// mapSize budget at default size. The search control is opt-in and
// hidden by default, so visually only zoom + reload are shown.
expect(leftControlCount).toBe(3);

let zoomHidden = await page.$eval(
'.leaflet-top.leaflet-left > .leaflet-control-zoom',
Expand Down Expand Up @@ -808,11 +813,16 @@ test.describe('mapml-viewer DOM API Tests', () => {
document.querySelector('mapml-viewer')
);

// search control should not exist by default (opt-in, lazily created)
let searchEl = await page.$(
'.leaflet-top.leaflet-left > .mapml-search-control'
// search control is now always created up-front and hidden by
// default (opt-in via controlslist="search"). This ensures it
// ends up in its correct DOM slot (between zoom and reload) when
// controlslist is set at runtime, instead of being appended after
// fullscreen.
let searchHiddenInitial = await page.$eval(
'.leaflet-top.leaflet-left > .mapml-search-control',
(div) => div.hidden
);
expect(searchEl).toBeNull();
expect(searchHiddenInitial).toEqual(true);

// enable search via controlslist attribute
await page.evaluate(
Expand Down
20 changes: 15 additions & 5 deletions test/e2e/api/domApi-web-map.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,12 @@ test.describe('web-map DOM API Tests', () => {
'.leaflet-top.leaflet-left',
(div) => div.childElementCount
);
expect(leftControlCount).toBe(2);
// 3 controls in DOM: zoom, search (always created up-front for
// correct DOM ordering but hidden because controlslist does not
// contain "search"), and reload. Fullscreen is squeezed out by the
// mapSize budget at default size. The search control is opt-in and
// hidden by default, so visually only zoom + reload are shown.
expect(leftControlCount).toBe(3);

let zoomHidden = await page.$eval(
'.leaflet-top.leaflet-left > .leaflet-control-zoom',
Expand Down Expand Up @@ -787,11 +792,16 @@ test.describe('web-map DOM API Tests', () => {
document.querySelector('map')
);

// search control should not exist by default (opt-in, lazily created)
let searchEl = await page.$(
'.leaflet-top.leaflet-left > .mapml-search-control'
// search control is now always created up-front and hidden by
// default (opt-in via controlslist="search"). This ensures it
// ends up in its correct DOM slot (between zoom and reload) when
// controlslist is set at runtime, instead of being appended after
// fullscreen.
let searchHiddenInitial = await page.$eval(
'.leaflet-top.leaflet-left > .mapml-search-control',
(div) => div.hidden
);
expect(searchEl).toBeNull();
expect(searchHiddenInitial).toEqual(true);

// enable search via controlslist attribute
await page.evaluate(
Expand Down
59 changes: 59 additions & 0 deletions test/e2e/mapml-viewer/mapml-viewer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,65 @@ test.describe('Playwright mapml-viewer Element Tests', () => {
);
expect(contains).toEqual(true);

// clean up
await page.$eval('body > mapml-viewer', (viewer) =>
viewer.removeAttribute('controlslist')
);
});
test('search control inserted between zoom and reload when controlslist="search" is added at runtime', async () => {
// Regression test: previously the search button was created
// lazily when controlslist="search" was observed, which meant
// Leaflet's Control.addTo() appended its container at the END of
// the topleft control corner (after fullscreen) instead of in
// its correct slot between zoom and reload. The fix creates the
// search button up-front in _createControls() and merely toggles
// its visibility via controlslist, mirroring the reload control
// pattern.

// Make sure we start without controlslist.
await page.$eval('body > mapml-viewer', (viewer) =>
viewer.removeAttribute('controlslist')
);
await page.waitForTimeout(200);

// Add controlslist="search" at runtime.
await page.$eval('body > mapml-viewer', (viewer) =>
viewer.setAttribute('controlslist', 'search')
);
await page.waitForTimeout(200);

// Read the DOM order of the topleft control corner and find the
// indexes of zoom, search and reload. The search button must
// appear AFTER zoom and BEFORE reload.
const order = await page.$eval('body > mapml-viewer', (viewer) => {
const corner = viewer.shadowRoot.querySelector(
'.leaflet-control-container > .leaflet-top.leaflet-left'
);
if (!corner) return [];
const result = [];
for (let i = 0; i < corner.children.length; i++) {
result.push(corner.children[i].className);
}
return result;
});

const zoomIndex = order.findIndex((cn) =>
cn.includes('leaflet-control-zoom')
);
const searchIndex = order.findIndex((cn) =>
cn.includes('mapml-search-control')
);
const reloadIndex = order.findIndex((cn) =>
cn.includes('mapml-reload-button')
);

expect(zoomIndex).toBeGreaterThanOrEqual(0);
expect(searchIndex).toBeGreaterThanOrEqual(0);
expect(reloadIndex).toBeGreaterThanOrEqual(0);

expect(searchIndex).toBeGreaterThan(zoomIndex);
expect(searchIndex).toBeLessThan(reloadIndex);

// clean up
await page.$eval('body > mapml-viewer', (viewer) =>
viewer.removeAttribute('controlslist')
Expand Down
62 changes: 62 additions & 0 deletions test/e2e/web-map/map.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,68 @@ test.describe('Playwright web-map Element Tests', () => {
);
expect(contains).toEqual(true);

// clean up
await page.$eval('body > map', (map) =>
map.removeAttribute('controlslist')
);
});
test('search control inserted between zoom and reload when controlslist="search" is added at runtime', async () => {
// Regression test: previously the search button was created
// lazily when controlslist="search" was observed, which meant
// Leaflet's Control.addTo() appended its container at the END of
// the topleft control corner (after fullscreen) instead of in
// its correct slot between zoom and reload. The fix creates the
// search button up-front in _createControls() and merely toggles
// its visibility via controlslist, mirroring the reload control
// pattern.

// Make sure we start without controlslist.
await page.$eval('body > map', (map) =>
map.removeAttribute('controlslist')
);
await page.waitForTimeout(200);

// Add controlslist="search" at runtime.
await page.$eval('body > map', (map) =>
map.setAttribute('controlslist', 'search')
);
await page.waitForTimeout(200);

// Read the DOM order of the topleft control corner and find the
// indexes of zoom, search and reload. The search button must
// appear AFTER zoom and BEFORE reload.
const order = await page.$eval('body > map', (map) => {
// The Leaflet control container lives inside web-map's shadow
// root; reach it via the live Leaflet map instance.
const leafletContainer = map._map.getContainer();
const corner = leafletContainer.querySelector(
'.leaflet-control-container > .leaflet-top.leaflet-left'
);
if (!corner) return [];
const result = [];
for (let i = 0; i < corner.children.length; i++) {
result.push(corner.children[i].className);
}
return result;
});

const zoomIndex = order.findIndex((cn) =>
cn.includes('leaflet-control-zoom')
);
const searchIndex = order.findIndex((cn) =>
cn.includes('mapml-search-control')
);
const reloadIndex = order.findIndex((cn) =>
cn.includes('mapml-reload-button')
);

expect(zoomIndex).toBeGreaterThanOrEqual(0);
expect(searchIndex).toBeGreaterThanOrEqual(0);
expect(reloadIndex).toBeGreaterThanOrEqual(0);

expect(searchIndex).toBeGreaterThan(zoomIndex);
expect(searchIndex).toBeLessThan(reloadIndex);

// clean up
await page.$eval('body > map', (map) =>
map.removeAttribute('controlslist')
Expand Down
Loading