fix: browser inBbox setting not persistent and features click not opening popup

Refactor the way it updates itself in the process.

umap/static/umap/base.css

@@ -679,6 +679,9 @@ input[type=hidden].blur + [type="button"] {
     overflow-x: hidden;
 }
 #umap-ui-container {
+    /* Added for playwright to consider the element as non visible  */
+    /* as being out of the visible viewport is not enough  */
+    visibility: hidden;
     position: absolute;
     bottom: 0;
     padding: 0 10px;
@@ -970,6 +973,7 @@ input:invalid {
     }
     .umap-ui #umap-ui-container {
         right: 0;
+        visibility: visible;
     }
 }
 @media all and (orientation:portrait) {
@@ -982,6 +986,7 @@ input:invalid {
     }
     .umap-ui #umap-ui-container {
         right: 0;
+        visibility: visible;
     }
     .umap-ui .leaflet-right {
         right: 0;

umap/static/umap/js/umap.browser.js

@@ -6,9 +6,13 @@ L.U.Browser = L.Class.extend({
 
   initialize: function (map) {
     this.map = map
+    this.map.on('moveend', this.onMoveEnd, this)
   },
 
-  addFeature: function (feature) {
+  addFeature: function (feature, parent) {
+    const filter = this.options.filter
+    if (filter && !feature.matchFilter(filter, this.filterKeys)) return
+    if (this.options.inBbox && !feature.isOnScreen(this.bounds)) return
     const feature_li = L.DomUtil.create('li', `${feature.getClassName()} feature`),
       zoom_to = L.DomUtil.create('i', 'feature-zoom_to', feature_li),
       edit = L.DomUtil.create('i', 'show-on-edit feature-edit', feature_li),
@@ -48,58 +52,76 @@ L.U.Browser = L.Class.extend({
     )
     L.DomEvent.on(edit, 'click', feature.edit, feature)
     L.DomEvent.on(del, 'click', feature.confirmDelete, feature)
-    return feature_li
+    // HOTFIX. Remove when this is released:
+    // https://github.com/Leaflet/Leaflet/pull/9052
+    L.DomEvent.disableClickPropagation(feature_li)
+    parent.appendChild(feature_li)
   },
 
-  addDatalayer: function (datalayer, dataContainer) {
-    const filterKeys = this.map.getFilterKeys()
-    const container = L.DomUtil.create(
-        'div',
-        datalayer.getHidableClass(),
-        dataContainer
-      ),
-      headline = L.DomUtil.create('h5', '', container)
-    container.id = `browse_data_datalayer_${datalayer.umap_id}`
+  datalayerId: function (datalayer) {
+    return `browse_data_datalayer_${L.stamp(datalayer)}`
+  },
+
+  onDataLayerChanged: function (e) {
+    this.updateDatalayer(e.target)
+  },
+
+  addDataLayer: function (datalayer, parent) {
+    const container = L.DomUtil.create('div', datalayer.getHidableClass(), parent),
+      headline = L.DomUtil.create('h5', '', container),
+      counter = L.DomUtil.create('span', 'datalayer-counter', headline)
+    container.id = this.datalayerId(datalayer)
     datalayer.renderToolbox(headline)
     L.DomUtil.add('span', '', headline, datalayer.options.name)
     const ul = L.DomUtil.create('ul', '', container)
-    L.DomUtil.classIf(container, 'off', !datalayer.isVisible())
-
-    const build = () => {
-      ul.innerHTML = ''
-      const bounds = this.map.getBounds()
-      datalayer.eachFeature((feature) => {
-        if (
-          this.options.filter &&
-          !feature.matchFilter(this.options.filter, filterKeys)
-        )
-          return
-        if (this.options.inBbox && !feature.isOnScreen(bounds)) return
-        ul.appendChild(this.addFeature(feature))
+    this.updateDatalayer(datalayer)
+    datalayer.on('datachanged', this.onDataLayerChanged, this)
+    this.map.ui.once('panel:closed', () => {
+      datalayer.off('datachanged', this.onDataLayerChanged, this)
     })
-    }
+  },
+
+  updateDatalayer: function (datalayer) {
+    // Compute once, but use it for each feature later.
+    this.bounds = this.map.getBounds()
+    const parent = L.DomUtil.get(this.datalayerId(datalayer))
+    // Panel is not open
+    if (!parent) return
+    L.DomUtil.classIf(parent, 'off', !datalayer.isVisible())
+    const container = parent.querySelector('ul'),
+      counter = parent.querySelector('.datalayer-counter')
+    container.innerHTML = ''
+    datalayer.eachFeature((feature) => this.addFeature(feature, container))
 
-    build()
     let total = datalayer.count(),
-      current = ul.querySelectorAll('li').length,
+      current = container.querySelectorAll('li').length,
       count = total == current ? total : `${current}/${total}`
-    const counter = L.DomUtil.add('span', 'datalayer-counter', headline, count)
+    counter.textContent = count
     counter.title = L._('Features in this layer: {count}', { count: count })
-    datalayer.on('datachanged', build)
-    datalayer.map.ui.once('panel:closed', () => {
-      datalayer.off('datachanged', build)
-      this.map.off('moveend', build)
-    })
-    datalayer.map.ui.once('panel:ready', () => {
-      datalayer.map.ui.once('panel:ready', () => {
-        datalayer.off('datachanged', build)
+  },
+
+  onFormChange: function () {
+    this.map.eachBrowsableDataLayer((datalayer) => {
+      datalayer.resetLayer(true)
+      this.updateDatalayer(datalayer)
     })
+  },
+
+  onMoveEnd: function () {
+    const isBrowserOpen = !!document.querySelector('.umap-browse-data')
+    if (!isBrowserOpen) return
+    const isListDynamic = this.options.inBbox
+    this.map.eachBrowsableDataLayer((datalayer) => {
+      if (!isListDynamic && !datalayer.hasDynamicData()) return
+      this.updateDatalayer(datalayer)
     })
   },
 
   open: function () {
+    // Get once but use it for each feature later
+    this.filterKeys = this.map.getFilterKeys()
     const container = L.DomUtil.create('div', 'umap-browse-data')
-    // HOTFIX. Remove when this is merged and released:
+    // HOTFIX. Remove when this is released:
     // https://github.com/Leaflet/Leaflet/pull/9052
     L.DomEvent.disableClickPropagation(container)
 
@@ -113,39 +135,23 @@ L.U.Browser = L.Class.extend({
     const formContainer = L.DomUtil.create('div', '', container)
     const dataContainer = L.DomUtil.create('div', 'umap-browse-features', container)
 
-    const rebuildHTML = () => {
-      dataContainer.innerHTML = ''
-      this.map.eachBrowsableDataLayer((datalayer) => {
-        this.addDatalayer(datalayer, dataContainer)
-      })
-    }
-    const redrawDataLayers = () => {
-      this.map.eachBrowsableDataLayer((datalayer) => {
-        datalayer.resetLayer(true)
-      })
-    }
     const fields = [
       ['options.filter', { handler: 'Input', placeholder: L._('Filter') }],
       ['options.inBbox', { handler: 'Switch', label: L._('Current map view') }],
     ]
     const builder = new L.U.FormBuilder(this, fields, {
       makeDirty: false,
-      callback: (e) => {
-        if (e.helper.field === 'options.inBbox') {
-          if (this.options.inBbox) this.map.on('moveend', rebuildHTML)
-          else this.map.off('moveend', rebuildHTML)
-        }
-        redrawDataLayers()
-        rebuildHTML()
-      },
+      callback: () => this.onFormChange(),
     })
     formContainer.appendChild(builder.build())
 
-    rebuildHTML()
-
     this.map.ui.openPanel({
       data: { html: container },
       actions: [this.map._aboutLink()],
     })
+
+    this.map.eachBrowsableDataLayer((datalayer) => {
+      this.addDataLayer(datalayer, dataContainer)
+    })
   },
 })

umap/static/umap/js/umap.layer.js

@@ -743,9 +743,13 @@ L.U.DataLayer = L.Evented.extend({
     return !((!isNaN(from) && zoom < from) || (!isNaN(to) && zoom > to))
   },
 
+  hasDynamicData: function () {
+    return !!(this.options.remoteData && this.options.remoteData.dynamic)
+  },
+
   fetchRemoteData: async function (force) {
     if (!this.isRemoteLayer()) return
-    if (!this.options.remoteData.dynamic && this.hasDataLoaded() && !force) return
+    if (!this.hasDynamicData() && this.hasDataLoaded() && !force) return
     if (!this.isVisible()) return
     let url = this.map.localizeUrl(this.options.remoteData.url)
     if (this.options.remoteData.proxy) {

umap/tests/integration/test_browser.py

@@ -153,6 +153,50 @@ def test_data_browser_bbox_limit_should_be_dynamic(live_server, page, bootstrap,
     expect(page.get_by_text("one line in new zeland")).to_be_hidden()
 
 
+def test_data_browser_bbox_filter_should_be_persistent(
+    live_server, page, bootstrap, map
+):
+    # Zoom on Europe
+    page.goto(f"{live_server.url}{map.get_absolute_url()}#6/51.000/2.000")
+    el = page.get_by_text("Current map view")
+    expect(el).to_be_visible()
+    el.click()
+    browser = page.locator("#umap-ui-container")
+    expect(browser.get_by_text("one point in france")).to_be_visible()
+    expect(browser.get_by_text("one line in new zeland")).to_be_hidden()
+    expect(browser.get_by_text("one polygon in greenland")).to_be_hidden()
+    # Close and reopen the browser to make sure this settings is persistent
+    close = browser.get_by_text("Close")
+    close.click()
+    expect(browser).to_be_hidden()
+    sleep(0.5)
+    expect(browser.get_by_text("one point in france")).to_be_hidden()
+    expect(browser.get_by_text("one line in new zeland")).to_be_hidden()
+    expect(browser.get_by_text("one polygon in greenland")).to_be_hidden()
+    page.get_by_title("See data layers").click()
+    page.get_by_role("button", name="Browse data").click()
+    expect(browser.get_by_text("one point in france")).to_be_visible()
+    expect(browser.get_by_text("one line in new zeland")).to_be_hidden()
+    expect(browser.get_by_text("one polygon in greenland")).to_be_hidden()
+
+
+def test_data_browser_bbox_filtered_is_clickable(live_server, page, bootstrap, map):
+    popup = page.locator(".leaflet-popup")
+    # Zoom on Europe
+    page.goto(f"{live_server.url}{map.get_absolute_url()}#6/51.000/2.000")
+    el = page.get_by_text("Current map view")
+    expect(el).to_be_visible()
+    el.click()
+    browser = page.locator("#umap-ui-container")
+    expect(browser.get_by_text("one point in france")).to_be_visible()
+    expect(browser.get_by_text("one line in new zeland")).to_be_hidden()
+    expect(browser.get_by_text("one polygon in greenland")).to_be_hidden()
+    expect(popup).to_be_hidden()
+    browser.get_by_text("one point in france").click()
+    expect(popup).to_be_visible()
+    expect(popup.get_by_text("one point in france")).to_be_visible()
+
+
 def test_data_browser_with_variable_in_name(live_server, page, bootstrap, map):
     # Include a variable
     map.settings["properties"]["labelKey"] = "{name} ({foo})"