Fix error that occurs when removing and reattaching map from document (#827)

prushforth · prushfor · AliyanH · web-flow · commit 6ded79f9df9a · 2023-04-20T10:12:40.000-04:00
* First pass at fixing #813, detach / re-attach map/mapml-viewer fails. * Add test for (script) deletes, adds back a map or mapml-viewer to DOM * make tests fail if errors get thrown * Revert a change to web-map-index.html to allow it to work in place --------- Co-authored-by: prushfor <prushfor@L-BSC-A146390.nrn.nrcan.gc.ca> Co-authored-by: AliyanH <aliyanulhaq@gmail.com>
diff --git a/index.html b/index.html
@@ -72,7 +72,7 @@
      </noscript> 
   </head>
   <body>
-    <mapml-viewer projection="CBMTILE" zoom="2" lat="45" lon="-90" controls controlslist="geolocation">
+    <mapml-viewer projection="CBMTILE" zoom="2" lat="45" lon="-90" controls controlslist="nofullscreen geolocation">
       <map-caption>A pleasing map of Canada</map-caption>
       <layer- label="CBMT" src="https://geogratis.gc.ca/mapml/en/cbmtile/cbmt/" checked></layer->
       <layer- label="Hat Guy" checked>
diff --git a/src/layer.js b/src/layer.js
@@ -210,7 +210,7 @@ export class MapLayer extends HTMLElement {
   }
   _validateDisabled() {
     setTimeout(() => {
-      let layer = this._layer, map = layer._map;
+      let layer = this._layer, map = layer?._map;
       if (map) {
         let count = 0, total=0, layerTypes = ["_staticTileLayer","_imageLayer","_mapmlvectors","_templatedLayer"];
         if(layer.validProjection){
diff --git a/src/mapml-viewer.js b/src/mapml-viewer.js
@@ -130,17 +130,14 @@ export class MapViewer extends HTMLElement {
   }
   connectedCallback() {
 
-    this._createShadowRoot();
+    this._initShadowRoot();
 
     this._controlsList = new DOMTokenList(
       this.getAttribute("controlslist"),
       this, "controlslist", 
       ["noreload","nofullscreen","nozoom","nolayer","noscale","geolocation"]
     );
 
-    // the dimension attributes win, if they're there. A map does not
-    // have an intrinsic size, unlike an image or video, and so must
-    // have a defined width and height.
     var s = window.getComputedStyle(this),
       wpx = s.width, hpx=s.height,
       w = this.hasAttribute("width") ? this.getAttribute("width") : parseInt(wpx.replace('px','')),
@@ -195,11 +192,14 @@ export class MapViewer extends HTMLElement {
       }, 0);
     }
   }
-  _createShadowRoot() {
+  _initShadowRoot() {
+    if (!this.shadowRoot) {
+      this.attachShadow({mode: 'open'});
+    }
     let tmpl = document.createElement('template');
     tmpl.innerHTML = `<link rel="stylesheet" href="${new URL("mapml.css", import.meta.url).href}">`; // jshint ignore:line
 
-    let shadowRoot = this.attachShadow({mode: 'open'});
+    let shadowRoot = this.shadowRoot;
     this._container = document.createElement('div');
 
     let output = "<output role='status' aria-live='polite' aria-atomic='true' class='mapml-screen-reader-output'></output>";
@@ -277,8 +277,11 @@ export class MapViewer extends HTMLElement {
     }
   }
   disconnectedCallback() {
-    //this._removeEvents();
+    while (this.shadowRoot.firstChild) {
+      this.shadowRoot.removeChild(this.shadowRoot.firstChild);
+    }      
     delete this._map;
+    this._deleteControls();
   }
   adoptedCallback() {
 //    console.log('Custom map element moved to new page.');
@@ -431,6 +434,15 @@ export class MapViewer extends HTMLElement {
     }
   }
 
+  // delete the map controls that are private properties of this custom element
+  _deleteControls() {
+    delete this._layerControl;
+    delete this._zoomControl;
+    delete this._reloadButton;
+    delete this._fullScreenControl;
+    delete this._geolocationButton;
+    delete this._scaleBar;
+  }
   // Sets the control's visibility AND all its childrens visibility,
   // for the control element based on the Boolean hide parameter
   _setControlsVisibility(control, hide) {
diff --git a/src/web-map.js b/src/web-map.js
@@ -134,17 +134,14 @@ export class WebMap extends HTMLMapElement {
   }
   connectedCallback() {
 
-    this._createShadowRoot();
+    this._initShadowRoot();
 
     this._controlsList = new DOMTokenList(
       this.getAttribute("controlslist"),
       this, "controlslist", 
       ["noreload","nofullscreen","nozoom","nolayer","noscale","geolocation"]
     );
     
-    // the dimension attributes win, if they're there. A map does not
-    // have an intrinsic size, unlike an image or video, and so must
-    // have a defined width and height.
     var s = window.getComputedStyle(this),
       wpx = s.width, hpx=s.height,
       w = this.hasAttribute("width") ? this.getAttribute("width") : parseInt(wpx.replace('px','')),
@@ -197,7 +194,7 @@ export class WebMap extends HTMLMapElement {
       }, 0);
     }
   }
-  _createShadowRoot() {
+  _initShadowRoot() {
     let tmpl = document.createElement('template');
     tmpl.innerHTML = `<link rel="stylesheet" href="${new URL("mapml.css", import.meta.url).href}">`; // jshint ignore:line
 
@@ -319,8 +316,13 @@ export class WebMap extends HTMLMapElement {
     }
   }
   disconnectedCallback() {
-    //this._removeEvents();
+    let rootDiv = this.querySelector('.mapml-web-map');
+    while (rootDiv.shadowRoot.firstChild) {
+      rootDiv.shadowRoot.removeChild(rootDiv.shadowRoot.firstChild);
+    }
+    rootDiv.remove();
     delete this._map;
+    this._deleteControls();
   }
   adoptedCallback() {
 //    console.log('Custom map element moved to new page.');
@@ -473,6 +475,15 @@ export class WebMap extends HTMLMapElement {
     }
   }
 
+  // delete the map controls that are private properties of this custom element
+  _deleteControls() {
+    delete this._layerControl;
+    delete this._zoomControl;
+    delete this._reloadButton;
+    delete this._fullScreenControl;
+    delete this._geolocationButton;
+    delete this._scaleBar;
+  }
   // Sets the control's visibility AND all its childrens visibility,
   // for the control element based on the Boolean hide parameter
   _setControlsVisibility(control, hide) {
diff --git a/test/e2e/api/domApi-mapml-viewer.test.js b/test/e2e/api/domApi-mapml-viewer.test.js
@@ -19,7 +19,7 @@ test.describe("mapml-viewer DOM API Tests", () => {
     let errorLogs = [];
     page.on("pageerror", (err) => {
       errorLogs.push(err.message);
-    })
+    });
     const viewerHandle = await page.evaluateHandle(()=> document.createElement("mapml-viewer"));
     const nn = await (await page.evaluateHandle(viewer => viewer.nodeName, viewerHandle)).jsonValue();
     expect(nn).toEqual('MAPML-VIEWER');
@@ -78,6 +78,37 @@ test.describe("mapml-viewer DOM API Tests", () => {
             (tileDiv) => tileDiv.firstChild.nodeName === "IMG");
     expect(layerVisible).toBe(true);
   });
+  
+  test("Remove mapml-viewer from DOM, add it back in", async () => {
+    // check for error messages in console
+    let errorLogs = [];
+    page.on("pageerror", (err) => {
+      errorLogs.push(err.message);
+    });
+    // locators avoid flaky tests, allegedly
+    const viewer = await page.locator('mapml-viewer');
+    await viewer.evaluate(()=>{
+    });
+    expect(await viewer.evaluate(() => {
+      let m = document.querySelector('mapml-viewer');
+      document.body.removeChild(m);
+      document.body.appendChild(m);
+      let l = m.querySelector('layer-');
+      return l.label;
+      // the label attribute is ignored if the mapml document has a map-title
+      // element, which is the case here.  Since the layer loads over the
+      // network, it means that the map is back to normal after being re-added
+      /// to the DOM, if the label reads as follows:
+    })).toEqual("Canada Base Map - Transportation (CBMT)");
+    // takes a couple of seconds for the tiles to load
+
+    // check for error messages in console
+    expect(errorLogs.length).toBe(0);
+    
+    await page.waitForLoadState('networkidle');
+    const layerTile = await page.locator("body > mapml-viewer .leaflet-tile-loaded:nth-child(1)");
+    expect(await layerTile.evaluate(tile=>tile.firstChild.nodeName === "IMG")).toBe(true);
+  });
 
   test("Toggle all mapml-viewer controls by adding or removing controls attribute", async () => {
     await page.evaluateHandle(() => document.querySelector('layer-').setAttribute("hidden",""));
@@ -226,7 +257,7 @@ test.describe("mapml-viewer DOM API Tests", () => {
     // remove map for next test
     await page.evaluateHandle(() => document.querySelector('mapml-viewer').remove());
   });
-
+  
   test.describe("controlslist test", () => {
 
     test("map created with controlslist", async () => {
@@ -495,7 +526,7 @@ test.describe("mapml-viewer DOM API Tests", () => {
         return viewer.defineCustomProjection(template);
       }, viewerHandle);
       expect(custProj).toEqual("basic");
-      await page.evaluate( (viewer) => {viewer.projection = "basic"}, viewerHandle);
+      await page.evaluate( (viewer) => {viewer.projection = "basic";}, viewerHandle);
 
       // Toggle Controls (T) contextmenu is disabled
       let toggleControlsBtn = await page.$eval("div > div.mapml-contextmenu > button:nth-child(10)",(btn) => btn.disabled);
diff --git a/test/e2e/api/domApi-web-map.test.js b/test/e2e/api/domApi-web-map.test.js
@@ -79,6 +79,36 @@ test.describe("web-map DOM API Tests", () => {
             (tileDiv) => tileDiv.firstChild.nodeName === "IMG");
     expect(layerVisible).toBe(true);
   });
+  test("Remove map from DOM, add it back in", async () => {
+    // check for error messages in console
+    let errorLogs = [];
+    page.on("pageerror", (err) => {
+      errorLogs.push(err.message);
+    });
+    // locators avoid flaky tests, allegedly
+    const viewer = await page.locator('map');
+    await viewer.evaluate(()=>{
+    });
+    expect(await viewer.evaluate(() => {
+      let m = document.querySelector('map');
+      document.body.removeChild(m);
+      document.body.appendChild(m);
+      let l = m.querySelector('layer-');
+      return l.label;
+      // the label attribute is ignored if the mapml document has a map-title
+      // element, which is the case here.  Since the layer loads over the
+      // network, it means that the map is back to normal after being re-added
+      /// to the DOM, if the label reads as follows:
+    })).toEqual("Canada Base Map - Transportation (CBMT)");
+    // takes a couple of seconds for the tiles to load
+
+    // check for error messages in console
+    expect(errorLogs.length).toBe(0);
+    
+    await page.waitForLoadState('networkidle');
+    const layerTile = await page.locator("body > map .leaflet-tile-loaded:nth-child(1)");
+    expect(await layerTile.evaluate(tile=>tile.firstChild.nodeName === "IMG")).toBe(true);
+  });
 
   test("Toggle all web map controls by adding or removing controls attribute", async () => {
     await page.evaluateHandle(() => document.querySelector('layer-').setAttribute("hidden",""));
diff --git a/web-map-index.html b/web-map-index.html
@@ -20,15 +20,17 @@
       (e.g. when scripting is disabled or when custom/built-in elements isn't
       supported in the browser). */
       map[is="web-map"]:defined {
-        /* Responsive map. */
-        /* max-width: 100%; */
-
-        /* Full viewport. */
-        /* width: 100%; */
-        /* height: 100%; */
-
-        /* Remove default (native-like) border. */
-        /* border: none; */
+      /* Responsive map. */ 
+          max-width: 100%;  
+          
+         /* Full viewport. */ 
+          width: 100%;  
+          height: 100%;  
+          
+         /* Remove default (native-like) border. */ 
+         border: none;
+         
+         vertical-align: middle;
       }
 
       /* Pre-style to avoid Layout Shift. */
@@ -71,7 +73,7 @@
     </noscript>
   </head>
   <body>
-    <map is="web-map" projection="CBMTILE" zoom="2" lat="45" lon="-90" height = "600" width = "600" controls>
+    <map is="web-map" projection="CBMTILE" zoom="2" lat="45" lon="-90" controls controlslist="nofullscreen geolocation">
       <layer- label="CBMT" src="https://geogratis.gc.ca/mapml/en/cbmtile/cbmt/" checked></layer->
     </map>
   </body>

Original file line number	Diff line number	Diff line change
`@@ -210,7 +210,7 @@ export class MapLayer extends HTMLElement {`
`210`	`210`	`}`
`211`	`211`	`_validateDisabled() {`
`212`	`212`	`setTimeout(() => {`
`213`		`- let layer = this._layer, map = layer._map;`
	`213`	`+ let layer = this._layer, map = layer?._map;`
`214`	`214`	`if (map) {`
`215`	`215`	`let count = 0, total=0, layerTypes = ["_staticTileLayer","_imageLayer","_mapmlvectors","_templatedLayer"];`
`216`	`216`	`if(layer.validProjection){`