Fix upload/new-folder buttons not actually hiding when logged out

.gpx-panel__action's own "display: flex" outranks the browser's
default "[hidden] { display: none }" -- same specificity, but an
author stylesheet rule always beats a user-agent one -- so setting
.hidden = true on these (GpxPanelControl#mount) never had any visual
effect. The per-file row icons (delete/download) were unaffected by
this since they're conditionally never added to the DOM at all rather
than hidden after the fact, a different mechanism that doesn't hit
this bug.

Same class of bug as the earlier Distance/Area overlap fix. Swept the
rest of the app's [hidden]-manipulated elements for the same pattern
(a display override with no [hidden] counterpart) -- this was the
only other one.

Verified in a real browser: computed display is now "none", not just
the hidden attribute being set with no visual effect.

docs/architecture.adoc

@@ -338,34 +338,37 @@ app, and not yet opened. Badging the common case would just be noise,
 so the file tree's icon (`PENDING_ICON`, `public/js/icons.js`) marks
 that exception instead of the (near-universal) ready state.
 +
-Once a `.gpx`/`.fit` is cached, the tree shows it under its converted
+The tree always shows a `.gpx`/`.fit` file under its real, as-uploaded
-name instead — `"ride.gpx"` reads as `"ride.gpx.geojson"`, matching
+name, even once cached — it briefly grew a `.geojson` suffix there
-the real cache file sitting next to it on disk — since the GeoJSON,
+(matching the real cache file's name on disk) back when the name
-not the original, is what's actually rendered on the map from that
+doubled as a GeoJSON-only download link and the suffix explained what
-point on. The original is never deleted or re-parsed once cached, just
+clicking it would get you; now that downloading is the download
-not re-shown as itself, so both forms need a way out of the app as an
+button's job (below), the suffix was just confusing; the file itself
-actual file: the file name itself (`GET /api/gpx/file/<path>?download=geojson`)
+is still named `"ride.gpx"`. The original is never deleted or
-is a real link, not a `<span>` — clicking it downloads the GeoJSON
+re-parsed once cached, just not re-shown as itself once the GeoJSON
-rather than also toggling the row's checkbox, since a native
+becomes what's actually rendered on the map, so both forms need a way
-`<a>`/`<button>` nested inside a `<label>` claims its own click
+out of the app as an actual file: one download button, next to
-instead of the label forwarding it to the associated control — and a
+delete, opens a small menu ("Original file" / "GeoJSON",
-second button next to delete (`?download=original`) downloads the
+`_downloadButton`/`_downloadMenuItem`). Only one menu open at a time
-untouched, as-uploaded file, since the name alone only covers half of
+(`_toggleDownloadMenu`/`_closeDownloadMenu`), closed by a
-what's now downloadable and isn't an obvious click target regardless.
+document-level click listener the same way an outside click closes a
-A directly-uploaded `.geojson` keeps its own name and both buttons
+`sidebar-flyout` dropdown, just scoped to this one control rather than
-download the same (identical) content — redundant there, not wrong.
+threaded through `onlyOneSidePanelOpen`. A directly-uploaded `.geojson`
-Both kinds of download require login, same as upload/move/delete/
+keeps its own name and both menu items download the same (identical)
-create-folder — `GET /api/gpx/file` itself stays open to anyone
+content — redundant there, not wrong. Both downloads (`GET
-(viewing a file on the map isn't a write action), just not
+/api/gpx/file/<path>?download=original`/`?download=geojson`) require
-`?download=...` on it. Since both links are real `<a href>`s (so
+login, same as upload/move/delete/create-folder, so the whole button —
-right-click/middle-click still do something sensible), a logged-out
+not just its menu items — is left out of the DOM when logged out
-click can't just be a plain navigation the way delete/upload already
+rather than shown and failing on click (see "Authentication" below);
-aren't — `_download` (`gpx.js`) fetches instead, checks for a 401 the
+`GET /api/gpx/file` itself stays open to anyone otherwise (viewing a
-same way `redirectIfUnauthorized` already does for every other write
+file on the map isn't a write action), just not `?download=...` on it.
-action, and only then turns the response into a blob and clicks a
+Each menu item is a real `<a href>` (so right-click/middle-click still
-throwaway `<a download>` to trigger the actual save — a plain
+do something sensible), but a plain navigation would land a logged-out
-navigation would otherwise land on a raw JSON error page instead of
+click on a raw JSON error page instead of being sent to log in —
-being sent to log in. `Gpx` parses the GPX with REXML: `<wpt>`
+`_download` (`gpx.js`) fetches instead, checks for a 401 the same way
+`redirectIfUnauthorized` already does for every other write action,
+and only then turns the response into a blob and clicks a throwaway
+`<a download>` to trigger the actual save. `Gpx` parses the GPX with REXML: `<wpt>`
 waypoints become `Point` features (with `name`, `desc` and `time`
 carried through as properties), each `<trk>` becomes one
 `MultiLineString` feature (one line per `<trkseg>`), and each `<rte>`

public/css/style.css

@@ -277,6 +277,14 @@ html, body {
   cursor: pointer;
 }
 
+/* This class's own "display: flex" above otherwise wins over the browser
+   default "[hidden] { display: none }" — same specificity, but an author
+   rule always outranks a user-agent one — so GpxPanelControl#mount hiding
+   these (logged out: no upload/new-folder) had no visual effect at all. */
+.gpx-panel__action[hidden] {
+  display: none;
+}
+
 .gpx-panel__action svg {
   width: 18px;
   height: 18px;
@@ -379,11 +387,20 @@ html, body {
   height: 14px;
 }
 
+.gpx-tree__download-wrapper {
+  position: relative;
+  flex-shrink: 0;
+  display: flex;
+}
+
 .gpx-tree__download {
   flex-shrink: 0;
   display: flex;
+  border: none;
+  background: none;
   color: #999;
   padding: 0 4px;
+  cursor: pointer;
 }
 
 .gpx-tree__download svg {
@@ -395,6 +412,38 @@ html, body {
   color: #333;
 }
 
+.gpx-tree__download-menu {
+  position: absolute;
+  top: 100%;
+  right: 0;
+  z-index: 20;
+  min-width: max-content;
+  background: #fff;
+  border: 1px solid #ddd;
+  border-radius: 6px;
+  box-shadow: 0 2px 8px rgba(0, 0, 0, 0.15);
+  padding: 4px;
+  font-size: 13px;
+}
+
+.gpx-tree__download-menu[hidden] {
+  display: none;
+}
+
+.gpx-tree__download-menu-item {
+  display: block;
+  padding: 6px 10px;
+  border-radius: 4px;
+  color: inherit;
+  text-decoration: none;
+  white-space: nowrap;
+  cursor: pointer;
+}
+
+.gpx-tree__download-menu-item:hover {
+  background: #f0f0f0;
+}
+
 .gpx-tree__delete {
   flex-shrink: 0;
   border: none;

public/js/gpx.js

@@ -53,6 +53,7 @@ export class GpxPanelControl {
     this._visible = new Set(); // file paths currently shown on the map
     this._interactiveLayers = new Set(); // point layer ids with click/hover wired up
     this._pointsByPath = new Map(); // path -> its waypoints FeatureCollection, for the category legend
+    this._openDownloadMenu = null; // the one gpx-tree__download-menu currently open, if any
 
     // setStyle() (switching the Online/Offline base map) tears down all
     // custom sources/layers/images — re-register the category glyphs before
@@ -130,6 +131,14 @@ export class GpxPanelControl {
 
     onlyOneSidePanelOpen(this._panel, () => this._icon.classList.remove("active"));
 
+    // Closes whichever per-file download menu is open on any click that
+    // isn't the toggle button that opened it (that one calls
+    // stopPropagation so this never fights it) — same "outside click
+    // closes it" behavior sidebar-flyout dropdowns already have, just not
+    // wired through onlyOneSidePanelOpen since these aren't full side
+    // panels.
+    document.addEventListener("click", () => this._closeDownloadMenu());
+
     this._setupToolbarActions();
     this._setupRootDropTarget();
     this._setupMapDropTarget();
@@ -287,37 +296,16 @@ export class GpxPanelControl {
     checkbox.type = "checkbox";
     checkbox.checked = this._visible.has(file.path);
     checkbox.addEventListener("change", () => this._toggleFile(file.path, checkbox.checked, checkbox));
-    // Once a .gpx/.fit is cached, its converted GeoJSON is the thing that's
+    // Just the real, as-uploaded name — it used to grow a ".geojson" suffix
-    // actually shown on the map from here on (see GpxStore#geojson) — the
+    // once cached (matching the real "<path>.geojson" cache file on disk),
-    // name reflects that (matching the real "<path>.geojson" cache file on
+    // back when the name doubled as a GeoJSON-only download link and the
-    // disk) rather than still reading as a plain, un-converted .gpx/.fit.
+    // suffix explained what clicking it would get you. Now that downloading
-    // A direct .geojson upload already ends in .geojson, so it's left
+    // is the download button's job (see below), that suffix was just
-    // alone.
+    // confusing: the file is still named "ride.gpx", not "ride.gpx.geojson".
-    const displayName = file.cached && !file.name.toLowerCase().endsWith(".geojson") ? `${file.name}.geojson` : file.name;
     const loggedIn = isLoggedIn();
-    // Logged in: an <a>, not a <span> — clicking it downloads that GeoJSON,
+    const name = document.createElement("span");
-    // the only way to get it out of the app, since the original .gpx/.fit
-    // is never deleted but also never re-shown once converted. An <a>
-    // nested in a <label> doesn't also toggle the checkbox on click (unlike
-    // a plain element), so this doesn't fight the row's own
-    // toggle-on-map behavior. href kept pointing at the real endpoint (so
-    // hover/right-click/middle-click still do something sensible) but the
-    // actual click goes through _download, which redirects to /login on a
-    // 401 rather than navigating to a raw JSON error page (this endpoint
-    // requires login the same as upload/move/delete, see app.rb).
-    // Logged out: plain text — nothing here would actually work, so it
-    // isn't shown as if it would (same reasoning as the buttons below).
-    const name = document.createElement(loggedIn ? "a" : "span");
     name.className = "gpx-tree__file-name";
-    name.textContent = displayName;
+    name.textContent = file.name;
-    if (loggedIn) {
-      name.href = `${gpxFileUrl(file.path)}?download=geojson`;
-      name.title = `Download ${displayName}`;
-      name.addEventListener("click", (event) => {
-        event.preventDefault();
-        this._download(name.href, displayName);
-      });
-    }
     label.appendChild(checkbox);
     (file.kinds || []).forEach((kind) => label.appendChild(this._kindIcon(kind)));
     if (!file.cached) label.appendChild(this._pendingIcon());
@@ -357,24 +345,75 @@ export class GpxPanelControl {
     return icon;
   }
 
-  // The file's name already downloads its GeoJSON (?download=geojson,
+  // One button, one obvious place for both download kinds — clicking it
-  // matching what's displayed) — this covers the other half: the
+  // opens a small menu (Original / GeoJSON) instead of splitting them
-  // untouched, as-uploaded file, which otherwise has no way out of the
+  // across two differently-shaped click targets (the file name used to be
-  // app once it stops being shown as itself. For a native .geojson
+  // a GeoJSON-only download link; easy to miss it was a link at all, and
-  // upload the two downloads are identical (nothing to convert), so this
+  // it only ever covered half the story). Costs a click for the common
-  // isn't hidden for those — just redundant, not wrong.
+  // case of downloading several files in a row, in exchange for both
+  // formats actually being reachable from one, discoverable place.
   _downloadButton(file) {
-    const link = document.createElement("a");
+    const wrapper = document.createElement("span");
-    link.className = "gpx-tree__download";
+    wrapper.className = "gpx-tree__download-wrapper";
-    link.href = `${gpxFileUrl(file.path)}?download=original`;
+
-    link.title = "Download original file";
+    const button = document.createElement("button");
-    link.setAttribute("aria-label", "Download original file");
+    button.type = "button";
-    link.innerHTML = DOWNLOAD_ICON;
+    button.className = "gpx-tree__download";
-    link.addEventListener("click", (event) => {
+    button.title = "Download";
-      event.preventDefault();
+    button.setAttribute("aria-label", "Download");
-      this._download(link.href, file.name);
+    button.innerHTML = DOWNLOAD_ICON;
+
+    const menu = document.createElement("div");
+    menu.className = "gpx-tree__download-menu";
+    menu.hidden = true;
+    const geojsonName = file.cached && !file.name.toLowerCase().endsWith(".geojson") ? `${file.name}.geojson` : file.name;
+    menu.appendChild(this._downloadMenuItem("Original file", `${gpxFileUrl(file.path)}?download=original`, file.name));
+    menu.appendChild(this._downloadMenuItem("GeoJSON", `${gpxFileUrl(file.path)}?download=geojson`, geojsonName));
+
+    // stopPropagation so this click doesn't immediately reach the
+    // document-level listener (mount()) that closes whichever menu is
+    // open on an outside click — otherwise it would close right back up
+    // the instant it opened.
+    button.addEventListener("click", (event) => {
+      event.stopPropagation();
+      this._toggleDownloadMenu(menu);
     });
-    return link;
+
+    wrapper.appendChild(button);
+    wrapper.appendChild(menu);
+    return wrapper;
+  }
+
+  _downloadMenuItem(label, url, filename) {
+    const item = document.createElement("a");
+    item.className = "gpx-tree__download-menu-item";
+    item.href = url;
+    item.textContent = label;
+    item.addEventListener("click", (event) => {
+      event.preventDefault();
+      this._closeDownloadMenu();
+      this._download(url, filename);
+    });
+    return item;
+  }
+
+  // Only one menu open at a time — opening a second one closes whichever
+  // was already open, same as onlyOneSidePanelOpen does for the sidebar's
+  // own panels (panels.js), just scoped to this one control instead of
+  // shared across the page.
+  _toggleDownloadMenu(menu) {
+    if (this._openDownloadMenu === menu) {
+      this._closeDownloadMenu();
+      return;
+    }
+    this._closeDownloadMenu();
+    menu.hidden = false;
+    this._openDownloadMenu = menu;
+  }
+
+  _closeDownloadMenu() {
+    if (this._openDownloadMenu) this._openDownloadMenu.hidden = true;
+    this._openDownloadMenu = null;
   }
 
   // fetch-based rather than a plain navigation so a 401 (not logged in —
@@ -720,19 +759,29 @@ export class GpxPanelControl {
     const files = [...fileList].filter((file) => /\.(gpx|fit|geojson|zip)$/i.test(file.name));
     if (files.length === 0) return;
 
-    this._setStatus(files.length === 1 ? "Uploading…" : `Uploading ${files.length} files…`);
     let tree = null;
     let failed = 0;
 
-    for (const file of files) {
+    for (const [index, file] of files.entries()) {
+      this._setStatus(files.length === 1 ? "Uploading…" : `Uploading ${index + 1} of ${files.length}…`);
       const formData = new FormData();
       formData.append("file", file);
       formData.append("folder", folder);
       try {
-        const response = await fetch(UPLOAD_URL, { method: "POST", body: formData });
+        // fetch() has no cross-browser upload progress event, so this one
-        if (redirectIfUnauthorized(response)) return;
+        // upload uses XMLHttpRequest instead — its upload.progress firing
-        if (!response.ok) throw new Error("upload failed");
+        // with loaded === total is the only client-observable signal that
-        ({ tree } = await response.json());
+        // the server has (or is about to start) converting rather than
+        // still receiving bytes, worth surfacing now that a several-
+        // thousand-waypoint POI export's conversion can itself take
+        // several more seconds on top of the transfer (GpxStore#save).
+        const xhr = await uploadFile(formData, () => this._setStatus("Converting…"));
+        if (xhr.status === 401) {
+          window.location.href = "/login";
+          return;
+        }
+        if (xhr.status < 200 || xhr.status >= 300) throw new Error("upload failed");
+        ({ tree } = JSON.parse(xhr.responseText));
       } catch (err) {
         failed += 1;
       }
@@ -814,6 +863,24 @@ function redirectIfUnauthorized(response) {
   return true;
 }
 
+// XMLHttpRequest rather than fetch() — only so onUploadComplete (the
+// upload.progress event) is available at all; every other write action in
+// this file stays on fetch()/redirectIfUnauthorized. Resolves with the xhr
+// itself (not just responseText) so the caller can still read .status the
+// same way a fetch Response would let it.
+function uploadFile(formData, onUploadComplete) {
+  return new Promise((resolve, reject) => {
+    const xhr = new XMLHttpRequest();
+    xhr.open("POST", UPLOAD_URL);
+    xhr.upload.addEventListener("progress", (event) => {
+      if (event.lengthComputable && event.loaded === event.total) onUploadComplete();
+    });
+    xhr.addEventListener("load", () => resolve(xhr));
+    xhr.addEventListener("error", () => reject(new Error("network error")));
+    xhr.send(formData);
+  });
+}
+
 // Rasterizes a hand-authored SVG icon to ImageData for map.addImage(), at a
 // fixed square size (the source SVGs have no explicit width/height, just a
 // viewBox, so one is injected — otherwise an <img> falls back to the CSS