Merge pull request #1586 from umap-project/fix-browser-inbbox

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

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/modules/browser.js

@@ -0,0 +1,159 @@
+// Uses `L._`` from Leaflet.i18n which we cannot import as a module yet
+import { DomUtil, DomEvent } from '../../vendors/leaflet/leaflet-src.esm.js'
+
+export default class Browser {
+  constructor(map) {
+    this.map = map
+    this.map.on('moveend', this.onMoveEnd, this)
+    this.options = {
+      filter: '',
+      inBbox: false,
+    }
+  }
+
+  addFeature(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 = DomUtil.create('li', `${feature.getClassName()} feature`),
+      zoom_to = DomUtil.create('i', 'feature-zoom_to', feature_li),
+      edit = DomUtil.create('i', 'show-on-edit feature-edit', feature_li),
+      del = DomUtil.create('i', 'show-on-edit feature-delete', feature_li),
+      colorBox = DomUtil.create('i', 'feature-color', feature_li),
+      title = DomUtil.create('span', 'feature-title', feature_li),
+      symbol = feature._getIconUrl
+        ? L.U.Icon.prototype.formatUrl(feature._getIconUrl(), feature)
+        : null
+    zoom_to.title = L._('Bring feature to center')
+    edit.title = L._('Edit this feature')
+    del.title = L._('Delete this feature')
+    title.textContent = feature.getDisplayName() || '—'
+    const bgcolor = feature.getDynamicOption('color')
+    colorBox.style.backgroundColor = bgcolor
+    if (symbol && symbol !== this.map.options.default_iconUrl) {
+      const icon = L.U.Icon.makeIconElement(symbol, colorBox)
+      L.U.Icon.setIconContrast(icon, colorBox, symbol, bgcolor)
+    }
+    DomEvent.on(
+      zoom_to,
+      'click',
+      function (e) {
+        e.callback = L.bind(this.view, this)
+        this.zoomTo(e)
+      },
+      feature
+    )
+    DomEvent.on(
+      title,
+      'click',
+      function (e) {
+        e.callback = L.bind(this.view, this)
+        this.zoomTo(e)
+      },
+      feature
+    )
+    DomEvent.on(edit, 'click', feature.edit, feature)
+    DomEvent.on(del, 'click', feature.confirmDelete, feature)
+    // HOTFIX. Remove when this is released:
+    // https://github.com/Leaflet/Leaflet/pull/9052
+    DomEvent.disableClickPropagation(feature_li)
+    parent.appendChild(feature_li)
+  }
+
+  datalayerId(datalayer) {
+    return `browse_data_datalayer_${L.stamp(datalayer)}`
+  }
+
+  onDataLayerChanged(e) {
+    this.updateDatalayer(e.target)
+  }
+
+  addDataLayer(datalayer, parent) {
+    const container = DomUtil.create('div', datalayer.getHidableClass(), parent),
+      headline = DomUtil.create('h5', '', container),
+      counter = DomUtil.create('span', 'datalayer-counter', headline)
+    container.id = this.datalayerId(datalayer)
+    datalayer.renderToolbox(headline)
+    DomUtil.add('span', '', headline, datalayer.options.name)
+    const ul = DomUtil.create('ul', '', container)
+    this.updateDatalayer(datalayer)
+    datalayer.on('datachanged', this.onDataLayerChanged, this)
+    this.map.ui.once('panel:closed', () => {
+      datalayer.off('datachanged', this.onDataLayerChanged, this)
+    })
+  }
+
+  updateDatalayer(datalayer) {
+    // Compute once, but use it for each feature later.
+    this.bounds = this.map.getBounds()
+    const parent = DomUtil.get(this.datalayerId(datalayer))
+    // Panel is not open
+    if (!parent) return
+    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))
+
+    let total = datalayer.count(),
+      current = container.querySelectorAll('li').length,
+      count = total == current ? total : `${current}/${total}`
+    counter.textContent = count
+    counter.title = L._('Features in this layer: {count}', { count: count })
+  }
+
+  onFormChange() {
+    this.map.eachBrowsableDataLayer((datalayer) => {
+      datalayer.resetLayer(true)
+      this.updateDatalayer(datalayer)
+    })
+  }
+
+  onMoveEnd() {
+    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() {
+    // Get once but use it for each feature later
+    this.filterKeys = this.map.getFilterKeys()
+    const container = DomUtil.create('div', 'umap-browse-data')
+    // HOTFIX. Remove when this is released:
+    // https://github.com/Leaflet/Leaflet/pull/9052
+    DomEvent.disableClickPropagation(container)
+
+    const title = DomUtil.add(
+      'h3',
+      'umap-browse-title',
+      container,
+      this.map.options.name
+    )
+
+    const formContainer = DomUtil.create('div', '', container)
+    const dataContainer = DomUtil.create('div', 'umap-browse-features', container)
+
+    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: () => this.onFormChange(),
+    })
+    formContainer.appendChild(builder.build())
+
+    this.map.ui.openPanel({
+      data: { html: container },
+      actions: [this.map._aboutLink()],
+    })
+
+    this.map.eachBrowsableDataLayer((datalayer) => {
+      this.addDataLayer(datalayer, dataContainer)
+    })
+  }
+}

umap/static/umap/js/modules/global.js

@@ -1,9 +1,10 @@
 import * as L from '../../vendors/leaflet/leaflet-src.esm.js'
 import URLs from './urls.js'
+import Browser from './browser.js'
 import { Request, ServerRequest, RequestError, HTTPError, NOKError } from './request.js'
 // Import modules and export them to the global scope.
 // For the not yet module-compatible JS out there.
 
 // Copy the leaflet module, it's expected by leaflet plugins to be writeable.
 window.L = { ...L }
-window.umap = { URLs, Request, ServerRequest, RequestError, HTTPError, NOKError }
+window.umap = { URLs, Request, ServerRequest, RequestError, HTTPError, NOKError, Browser }

umap/static/umap/js/modules/request.js

@@ -1,5 +1,5 @@
 // Uses `L._`` from Leaflet.i18n which we cannot import as a module yet
-import { Evented, DomUtil } from '../../vendors/leaflet/leaflet-src.esm.js'
+import { DomUtil } from '../../vendors/leaflet/leaflet-src.esm.js'
 
 export class RequestError extends Error {}
 

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

@@ -1,151 +0,0 @@
-L.U.Browser = L.Class.extend({
-  options: {
-    filter: '',
-    inBbox: false,
-  },
-
-  initialize: function (map) {
-    this.map = map
-  },
-
-  addFeature: function (feature) {
-    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),
-      del = L.DomUtil.create('i', 'show-on-edit feature-delete', feature_li),
-      colorBox = L.DomUtil.create('i', 'feature-color', feature_li),
-      title = L.DomUtil.create('span', 'feature-title', feature_li),
-      symbol = feature._getIconUrl
-        ? L.U.Icon.prototype.formatUrl(feature._getIconUrl(), feature)
-        : null
-    zoom_to.title = L._('Bring feature to center')
-    edit.title = L._('Edit this feature')
-    del.title = L._('Delete this feature')
-    title.textContent = feature.getDisplayName() || '—'
-    const bgcolor = feature.getDynamicOption('color')
-    colorBox.style.backgroundColor = bgcolor
-    if (symbol && symbol !== this.map.options.default_iconUrl) {
-      const icon = L.U.Icon.makeIconElement(symbol, colorBox)
-      L.U.Icon.setIconContrast(icon, colorBox, symbol, bgcolor)
-    }
-    L.DomEvent.on(
-      zoom_to,
-      'click',
-      function (e) {
-        e.callback = L.bind(this.view, this)
-        this.zoomTo(e)
-      },
-      feature
-    )
-    L.DomEvent.on(
-      title,
-      'click',
-      function (e) {
-        e.callback = L.bind(this.view, this)
-        this.zoomTo(e)
-      },
-      feature
-    )
-    L.DomEvent.on(edit, 'click', feature.edit, feature)
-    L.DomEvent.on(del, 'click', feature.confirmDelete, feature)
-    return 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}`
-    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))
-      })
-    }
-
-    build()
-    let total = datalayer.count(),
-      current = ul.querySelectorAll('li').length,
-      count = total == current ? total : `${current}/${total}`
-    const counter = L.DomUtil.add('span', 'datalayer-counter', headline, 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)
-      })
-    })
-  },
-
-  open: function () {
-    const container = L.DomUtil.create('div', 'umap-browse-data')
-    // HOTFIX. Remove when this is merged and released:
-    // https://github.com/Leaflet/Leaflet/pull/9052
-    L.DomEvent.disableClickPropagation(container)
-
-    const title = L.DomUtil.add(
-      'h3',
-      'umap-browse-title',
-      container,
-      this.map.options.name
-    )
-
-    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()
-      },
-    })
-    formContainer.appendChild(builder.build())
-
-    rebuildHTML()
-
-    this.map.ui.openPanel({
-      data: { html: container },
-      actions: [this.map._aboutLink()],
-    })
-  },
-})

umap/static/umap/js/umap.js

@@ -423,7 +423,7 @@ L.U.Map.include({
     this._controls.permanentCredit = new L.U.PermanentCreditsControl(this)
     if (this.options.scrollWheelZoom) this.scrollWheelZoom.enable()
     else this.scrollWheelZoom.disable()
-    this.browser = new L.U.Browser(this)
+    this.browser = new window.umap.Browser(this)
     this.importer = new L.U.Importer(this)
     this.drop = new L.U.DropControl(this)
     this.share = new L.U.Share(this)

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/static/umap/test/index.html

@@ -44,7 +44,6 @@
     <script src="../js/umap.controls.js" defer></script>
     <script src="../js/umap.slideshow.js" defer></script>
     <script src="../js/umap.tableeditor.js" defer></script>
-    <script src="../js/umap.browser.js" defer></script>
     <script src="../js/umap.importer.js" defer></script>
     <script src="../js/umap.share.js" defer></script>
     <script src="../js/umap.js" defer></script>

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})"