[chores:fix] Preserve indoor overlay URL fragment on popup close #546

Added configurable fragment preservation support for
bookmarkable actions so indoor map overlays retain
their URL fragment when only the popup nodeId is removed.

Fixes #546

---------

Co-authored-by: Federico Capoano <f.capoano@openwisp.io>

Author: dee077

README.md

@@ -566,6 +566,7 @@ NetJSON format used internally is based on [networkgraph](http://netjson.org/rfc
     id: string,
     zoomOnRestore: boolean,
     zoomLevel: number,
+    preserveFragment: boolean,
   }
   ```
 
@@ -587,7 +588,8 @@ NetJSON format used internally is based on [networkgraph](http://netjson.org/rfc
 
   For links, the URL fragment uses the format `source~target` as the `nodeId`. Opening such a URL restores the initial map or graph view and triggers the corresponding link click event.
 
-  If you need to manually remove the URL fragment, you can call the built-in utility method: `netjsongraphInstance.utils.removeUrlFragment(bookmarkableActions.id);` where you pass the value of your `bookmarkableActions.id` configuration.
+  If you need to manually remove the URL fragment, you can call the built-in utility method: `netjsongraphInstance.utils.removeUrlFragment(bookmarkableActions.id);` where you pass the value of your `bookmarkableActions.id` configuration. You can also remove only a specific parameter from the fragment, for example:
+  `netjsongraphInstance.utils.removeUrlFragment(bookmarkableActions.id, "nodeId");`. This removes only the `nodeId` parameter from the URL fragment for that specific map while preserving the remaining fragment state. To keep the fragment itself after removing `nodeId`, make sure to set `bookmarkableActions.preserveFragment = true` otherwise, if no additional parameters remain after removing `nodeId`, the entire fragment will be cleaned up automatically.
 
 - `onInit`
 

public/example_templates/netjsonmap-indoormap-overlay.html

@@ -314,6 +314,7 @@
               enabled: true,
               id: "indoorMap",
               zoomOnRestore: false,
+              preserveFragment: true,
             },
             prepareData: (data) => {
               data.nodes.forEach((n) => {

src/js/netjsongraph.config.js

@@ -301,6 +301,7 @@ const NetJSONGraphDefaultConfig = {
     id: null,
     zoomOnRestore: true,
     zoomLevel: null,
+    preserveFragment: false,
   },
 
   /**

src/js/netjsongraph.gui.js

@@ -325,8 +325,8 @@ class NetJSONGraphGUI {
       console.error("Node location not available. Cannot load popup.");
       return;
     }
-    const bookmarkableActionId =
-      self.config.bookmarkableActions && self.config.bookmarkableActions.id;
+    const {bookmarkableActions: {id: bookmarkableActionId, preserveFragment} = {}} =
+      self.config;
     const popupRequest = {};
     self.leaflet.currentPopupRequest = popupRequest;
     // Track whether tooltip/labels were already hidden by an open popup, so
@@ -354,7 +354,7 @@ class NetJSONGraphGUI {
         if (self.leaflet.currentPopupRequest !== popupRequest) {
           return;
         }
-        self.utils.removeUrlFragment(bookmarkableActionId, "nodeId");
+        self.utils.removeUrlFragment(bookmarkableActionId, "nodeId", preserveFragment);
         self.leaflet.currentPopupRequest = null;
         // If we tore down a previous popup before content generation failed,
         // its remove handler was bypassed — restore tooltip/labels manually so
@@ -397,7 +397,11 @@ class NetJSONGraphGUI {
         const fragments = self.utils.parseUrlFragments();
         const currentFragment = fragments[bookmarkableActionId];
         if (currentFragment && currentFragment.get("nodeId") === popupNodeId) {
-          self.utils.removeUrlFragment(bookmarkableActionId, "nodeId");
+          self.utils.removeUrlFragment(
+            bookmarkableActionId,
+            "nodeId",
+            preserveFragment,
+          );
         }
       }
       self.leaflet.currentPopup = null;

src/js/netjsongraph.util.js

@@ -1270,6 +1270,16 @@ class NetJSONGraphUtil {
       .map((urlParams) => urlParams.toString())
       .join(";");
 
+    // Remove dangling "#" when no fragments remain.
+    if (!newHash) {
+      window.history.pushState(
+        state,
+        "",
+        window.location.pathname + window.location.search,
+      );
+      return;
+    }
+
     // We store the selected node's data to the browser's history state.
     // This allows the node's information to be retrieved instantly on a back/forward
     // button click without needing to re-parse the entire nodes list.
@@ -1335,19 +1345,22 @@ class NetJSONGraphUtil {
    * @param {string} id        The bookmarkable action id (e.g. "geoMap").
    * @param {string} [paramName] If provided, only this query-param is removed
    *   from the fragment. If omitted, the whole fragment for the id is dropped.
+   * @param {boolean} [preserveFragment] If true, keep a bare id-only fragment
+   *   after removing paramName.
    */
-  removeUrlFragment(id, paramName = null) {
+  removeUrlFragment(id, paramName = null, preserveFragment = false) {
     const fragments = this.parseUrlFragments();
     if (!fragments[id]) {
       return;
     }
     if (paramName) {
       fragments[id].delete(paramName);
-      // Drop the whole entry if only the bare action id is left — a fragment
-      // like "#id=geoMap" with no other params is a useless stub that
-      // parseUrlFragments would still pick up on subsequent visits.
+      // Remove the entire fragment if only the bare action id remains,
+      // unless preserveFragment is enabled. Some consumers (e.g. indoor
+      // overlays) use the fragment itself as meaningful UI state even
+      // without additional params like nodeId.
       const remainingKeys = Array.from(fragments[id].keys()).filter((k) => k !== "id");
-      if (remainingKeys.length === 0) {
+      if (!preserveFragment && remainingKeys.length === 0) {
         delete fragments[id];
       }
     } else {

test/netjsongraph.dom.test.js

@@ -737,7 +737,11 @@ describe("Test GUI loadNodePopup with async and tooltip handling", () => {
     testGraph.utils.removeUrlFragment = jest.fn();
     const node = {id: "node-1", location: {lat: 10, lng: 20}};
     await testGraph.gui.loadNodePopup(node);
-    expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith("id", "nodeId");
+    expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith(
+      "id",
+      "nodeId",
+      false,
+    );
     expect(testGraph.echarts.setOption).not.toHaveBeenCalled();
     expect(testGraph.utils.updateLabelVisibility).not.toHaveBeenCalled();
   });
@@ -778,7 +782,11 @@ describe("Test GUI loadNodePopup with async and tooltip handling", () => {
     await testGraph.gui.loadNodePopup(node);
     expect(testGraph.utils.setTooltipVisibility).toHaveBeenCalledWith(testGraph, true);
     expect(testGraph.utils.updateLabelVisibility).toHaveBeenCalledWith(testGraph, true);
-    expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith("id", "nodeId");
+    expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith(
+      "id",
+      "nodeId",
+      false,
+    );
   });
 
   test("loadNodePopup catches synchronous custom content errors", async () => {
@@ -807,7 +815,11 @@ describe("Test GUI loadNodePopup with async and tooltip handling", () => {
     testGraph.utils.removeUrlFragment = jest.fn();
     const node = {id: "node-1", location: {lat: 10, lng: 20}};
     await testGraph.gui.loadNodePopup(node);
-    expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith("id", "nodeId");
+    expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith(
+      "id",
+      "nodeId",
+      false,
+    );
     expect(testGraph.echarts.setOption).not.toHaveBeenCalled();
   });
 
@@ -832,7 +844,43 @@ describe("Test GUI loadNodePopup with async and tooltip handling", () => {
     mockPopup.handlers.remove();
     expect(testGraph.utils.setTooltipVisibility).toHaveBeenCalledWith(testGraph, true);
     expect(testGraph.utils.updateLabelVisibility).toHaveBeenCalledWith(testGraph, true);
-    expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith("id", "nodeId");
+    expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith(
+      "id",
+      "nodeId",
+      false,
+    );
+  });
+
+  test("loadNodePopup forwards preserveFragment on popup remove", async () => {
+    testGraph.setConfig({
+      bookmarkableActions: {
+        enabled: true,
+        id: "id",
+        preserveFragment: true,
+      },
+    });
+    testGraph.echarts = {
+      setOption: jest.fn(),
+    };
+    testGraph.leaflet = {
+      currentPopup: null,
+      once: jest.fn(),
+      off: jest.fn(),
+    };
+    testGraph.utils.updateLabelVisibility = jest.fn();
+    testGraph.utils.setTooltipVisibility = jest.fn();
+    testGraph.utils.parseUrlFragments = jest.fn(() => ({
+      id: new URLSearchParams("id=id&nodeId=node-1"),
+    }));
+    testGraph.utils.removeUrlFragment = jest.fn();
+    const node = {id: "node-1", location: {lat: 10, lng: 20}};
+    await testGraph.gui.loadNodePopup(node);
+    mockPopup.handlers.remove();
+    expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith(
+      "id",
+      "nodeId",
+      true,
+    );
   });
 
   test("loadNodePopup ignores stale async content without clearing newer URL fragment", async () => {
@@ -1032,7 +1080,11 @@ describe("Test GUI loadNodePopup with async and tooltip handling", () => {
     const node = {id: "node-1", location: {lat: 10, lng: 20}};
     await testGraph.gui.loadNodePopup(node);
     expect(mockPopup.remove).toHaveBeenCalled();
-    expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith("id", "nodeId");
+    expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith(
+      "id",
+      "nodeId",
+      false,
+    );
     expect(consoleErrorSpy).toHaveBeenCalledWith(
       "Failed to run popup onOpen callback:",
       onOpenError,

test/netjsongraph.util.test.js

@@ -732,6 +732,23 @@ describe("Test removeUrlFragment with paramName argument", () => {
     // After deletion, only `id` would remain → entire entry should be gone.
     expect(util.updateUrlFragments).toHaveBeenCalledWith({}, {id: "geoMap"});
   });
+
+  test("removeUrlFragment keeps an id-only fragment when preserveFragment is true", () => {
+    const util = new NetJSONGraphUtil();
+    const params = new URLSearchParams();
+    params.set("id", "geoMap");
+    params.set("nodeId", "node-1");
+    util.parseUrlFragments = jest.fn(() => ({
+      geoMap: params,
+    }));
+    util.updateUrlFragments = jest.fn();
+    util.removeUrlFragment.call(util, "geoMap", "nodeId", true);
+    expect(util.updateUrlFragments).toHaveBeenCalledWith(
+      {geoMap: params},
+      {id: "geoMap"},
+    );
+    expect(params.toString()).toBe("id=geoMap");
+  });
 });
 
 describe("Test updateLabelVisibility utility method", () => {