From cd9203975d7ba15a567ac0d2a8bc9cba07aae330 Mon Sep 17 00:00:00 2001
From: Bryan Housel <bryan@mapbox.com>
Date: Fri, 4 Jan 2019 15:48:39 -0500
Subject: [PATCH] Use touch targets for notes, fix a few bugs with note
 dragging (closes #5213)

---
 css/20_map.css               |   9 +-
 css/55_cursors.css           |   5 +
 css/65_data.css              |  31 +------
 modules/behavior/drag.js     |   4 +-
 modules/behavior/hover.js    |   6 +-
 modules/modes/drag_note.js   |  37 +++++---
 modules/modes/select_note.js |   1 +
 modules/renderer/map.js      |   2 +-
 modules/svg/notes.js         | 174 +++++++++++++++++++++++------------
 modules/svg/touch.js         |   2 +-
 10 files changed, 163 insertions(+), 108 deletions(-)

diff --git a/css/20_map.css b/css/20_map.css
index 0200a8317..fc575d830 100644
--- a/css/20_map.css
+++ b/css/20_map.css
@@ -31,7 +31,9 @@
 
 
 /* No interactivity except what we specifically allow */
-.layer-osm * {
+.data-layer.osm *,
+.data-layer.notes *,
+.data-layer.keepRight * {
     pointer-events: none;
 }
 
@@ -42,6 +44,7 @@
 
 /* `.target` objects are interactive */
 /* They can be picked up, clicked, hovered, or things can connect to them */
+.note.target,
 .node.target,
 .turn .target {
     pointer-events: fill;
@@ -78,7 +81,7 @@
 /* NOTE: when more QA layers are added, replace kr_error with generic QA layer selector */
 /* points, notes & QA */
 
-/* points & notes */
+/* points, notes, markers */
 g.kr_error .stroke,
 g.note .stroke {
     stroke: #222;
@@ -110,9 +113,7 @@ g.note .shadow {
     stroke-opacity: 0;
 }
 
-g.kr_error.related:not(.selected) .shadow,
 g.kr_error.hover:not(.selected) .shadow,
-g.note.related:not(.selected) .shadow,
 g.note.hover:not(.selected) .shadow,
 g.point.related:not(.selected) .shadow,
 g.point.hover:not(.selected) .shadow {
diff --git a/css/55_cursors.css b/css/55_cursors.css
index 466e21ae2..f473301c4 100644
--- a/css/55_cursors.css
+++ b/css/55_cursors.css
@@ -96,7 +96,12 @@
     cursor: url(img/cursor-draw.png) 9 9, crosshair; /* FF */
 }
 
+.mode-browse .note,
+.mode-browse .kr_error,
+.mode-select .note,
+.mode-select .kr_error,
 .turn rect,
 .turn circle {
     cursor: pointer;
 }
+
diff --git a/css/65_data.css b/css/65_data.css
index a008021c8..71aafab9c 100644
--- a/css/65_data.css
+++ b/css/65_data.css
@@ -1,28 +1,5 @@
 
 /* OSM Notes and KeepRight Layers */
-.layer-keepRight,
-.layer-notes {
-    pointer-events: none;
-}
-.layer-keepRight .kr_error,
-.layer-notes .note * {
-    pointer-events: none;
-}
-.mode-browse .layer-notes .note .note-fill,
-.mode-select .layer-notes .note .note-fill,
-.mode-select-data .layer-notes .note .note-fill,
-.mode-select-note .layer-notes .note .note-fill,
-.layer-keepRight .kr_error .kr_error-fill,
-.layer-notes .note .note-fill {
-    pointer-events: visible;
-    cursor: pointer; /* Opera */
-    cursor: url(img/cursor-select-point.png), pointer; /* FF */
-}
-
-.note-header-icon .note-shadow,
-.layer-notes .note .note-shadow {
-    color: #000;
-}
 
 .kr_error-header-icon .kr_error-fill,
 .layer-keepRight .kr_error .kr_error-fill {
@@ -32,19 +9,19 @@
 
 .note-header-icon .note-fill,
 .layer-notes .note .note-fill {
-    color: #ff3300;
+    color: #f30;
     stroke: #333;
     stroke-width: 40px;
 }
 .note-header-icon.new .note-fill,
 .layer-notes .note.new .note-fill {
-    color: #ffee00;
+    color: #fe0;
     stroke: #333;
     stroke-width: 40px;
 }
 .note-header-icon.closed .note-fill,
 .layer-notes .note.closed .note-fill {
-    color: #55dd00;
+    color: #5d0;
     stroke: #333;
     stroke-width: 40px;
 }
@@ -88,7 +65,7 @@
 .kr_error_type_110,    /* poi without name */
 .kr_error_type_150,    /* railway crossing without tag */
 .kr_error_type_220,    /* misspelled tag */
-.kr_error_type_380 {    /* non-physical sport tag */
+.kr_error_type_380 {   /* non-physical sport tag */
     color: #5d0;
 }
 
diff --git a/modules/behavior/drag.js b/modules/behavior/drag.js
index 0221149ec..5dd432975 100644
--- a/modules/behavior/drag.js
+++ b/modules/behavior/drag.js
@@ -160,8 +160,8 @@ export function behaviorDrag() {
                 for (; target && target !== root; target = target.parentNode) {
                     var datum = target.__data__;
 
-                    var entity = datum instanceof osmNote ?
-                        datum : datum && datum.properties && datum.properties.entity;
+                    var entity = datum instanceof osmNote ? datum
+                        : datum && datum.properties && datum.properties.entity;
 
                     if (entity && target[matchesSelector](_selector)) {
                         return dragstart.call(target, entity);
diff --git a/modules/behavior/hover.js b/modules/behavior/hover.js
index 10ba3a3bd..ad5d1c43d 100644
--- a/modules/behavior/hover.js
+++ b/modules/behavior/hover.js
@@ -112,7 +112,11 @@ export function behaviorHover(context) {
                 entity = datum;
                 selector = '.data' + datum.__featurehash__;
 
-            } else if (datum instanceof osmNote || datum instanceof krError) {
+            } else if (datum instanceof krError) {
+                entity = datum;
+                selector = '.error-' + datum.id;
+
+            } else if (datum instanceof osmNote) {
                 entity = datum;
                 selector = '.note-' + datum.id;
 
diff --git a/modules/modes/drag_note.js b/modules/modes/drag_note.js
index 7f834cb10..5c9714fc6 100644
--- a/modules/modes/drag_note.js
+++ b/modules/modes/drag_note.js
@@ -20,13 +20,14 @@ export function modeDragNote(context) {
 
     var _nudgeInterval;
     var _lastLoc;
+    var _note;    // most current note.. dragged note may have stale datum.
 
 
-    function startNudge(note, nudge) {
+    function startNudge(nudge) {
         if (_nudgeInterval) window.clearInterval(_nudgeInterval);
         _nudgeInterval = window.setInterval(function() {
             context.pan(nudge);
-            doMove(note, nudge);
+            doMove(nudge);
         }, 50);
     }
 
@@ -45,58 +46,66 @@ export function modeDragNote(context) {
 
 
     function start(note) {
-        context.surface().selectAll('.note-' + note.id)
+        _note = note;
+        var osm = services.osm;
+        if (osm) {
+            // Get latest note from cache.. The marker may have a stale datum bound to it
+            // and dragging it around can sometimes delete the users note comment.
+            _note = osm.getNote(_note.id);
+        }
+
+        context.surface().selectAll('.note-' + _note.id)
             .classed('active', true);
 
         context.perform(actionNoop());
         context.enter(mode);
-        context.selectedNoteID(note.id);
+        context.selectedNoteID(_note.id);
     }
 
 
-    function move(note) {
+    function move() {
         d3_event.sourceEvent.stopPropagation();
         _lastLoc = context.projection.invert(d3_event.point);
 
-        doMove(note);
+        doMove();
         var nudge = geoViewportEdge(d3_event.point, context.map().dimensions());
         if (nudge) {
-            startNudge(note, nudge);
+            startNudge(nudge);
         } else {
             stopNudge();
         }
     }
 
 
-    function doMove(note, nudge) {
+    function doMove(nudge) {
         nudge = nudge || [0, 0];
 
         var currPoint = (d3_event && d3_event.point) || context.projection(_lastLoc);
         var currMouse = geoVecSubtract(currPoint, nudge);
         var loc = context.projection.invert(currMouse);
 
-        note = note.move(loc);
+        _note = _note.move(loc);
 
         var osm = services.osm;
         if (osm) {
-            osm.replaceNote(note);  // update note cache
+            osm.replaceNote(_note);  // update note cache
         }
 
         context.replace(actionNoop());   // trigger redraw
     }
 
 
-    function end(note) {
+    function end() {
         context.replace(actionNoop());   // trigger redraw
 
         context
-            .selectedNoteID(note.id)
-            .enter(modeSelectNote(context, note.id));
+            .selectedNoteID(_note.id)
+            .enter(modeSelectNote(context, _note.id));
     }
 
 
     var drag = behaviorDrag()
-        .selector('.layer-notes .new')
+        .selector('.layer-touch.markers .target.note.new')
         .surface(d3_select('#map').node())
         .origin(origin)
         .on('start', start)
diff --git a/modules/modes/select_note.js b/modules/modes/select_note.js
index 4e5fd5d15..dd24ffd78 100644
--- a/modules/modes/select_note.js
+++ b/modules/modes/select_note.js
@@ -72,6 +72,7 @@ export function modeSelectNote(context, selectedNoteID) {
         } else {
             selection
                 .classed('selected', true);
+
             context.selectedNoteID(selectedNoteID);
         }
     }
diff --git a/modules/renderer/map.js b/modules/renderer/map.js
index ed6895871..3c4c0c3f8 100644
--- a/modules/renderer/map.js
+++ b/modules/renderer/map.js
@@ -351,7 +351,7 @@ export function rendererMap(context) {
     function editOff() {
         context.features().resetStats();
         surface.selectAll('.layer-osm *').remove();
-        surface.selectAll('.layer-touch *').remove();
+        surface.selectAll('.layer-touch:not(.markers) *').remove();
 
         var mode = context.mode();
         if (mode && mode.id !== 'save' && mode.id !== 'select-note' &&
diff --git a/modules/svg/notes.js b/modules/svg/notes.js
index 9e1d2c1aa..7df4d876a 100644
--- a/modules/svg/notes.js
+++ b/modules/svg/notes.js
@@ -8,12 +8,18 @@ import { svgPointTransform } from './index';
 import { services } from '../services';
 
 
+var _notesEnabled = false;
+var _osmService;
+
+
 export function svgNotes(projection, context, dispatch) {
     if (!dispatch) { dispatch = d3_dispatch('change'); }
     var throttledRedraw = _throttle(function () { dispatch.call('change'); }, 1000);
     var minZoom = 12;
-    var layer = d3_select(null);
-    var _notes;
+    var touchLayer = d3_select(null);
+    var drawLayer = d3_select(null);
+    var _notesVisible = false;
+
 
     function markerPath(selection, klass) {
         selection
@@ -22,40 +28,49 @@ export function svgNotes(projection, context, dispatch) {
             .attr('d', 'm17.5,0l-15,0c-1.37,0 -2.5,1.12 -2.5,2.5l0,11.25c0,1.37 1.12,2.5 2.5,2.5l3.75,0l0,3.28c0,0.38 0.43,0.6 0.75,0.37l4.87,-3.65l5.62,0c1.37,0 2.5,-1.12 2.5,-2.5l0,-11.25c0,-1.37 -1.12,-2.5 -2.5,-2.5z');
     }
 
-    function init() {
-        if (svgNotes.initialized) return;  // run once
-        svgNotes.enabled = false;
-        svgNotes.initialized = true;
-    }
-
-    function editOn() {
-        layer.style('display', 'block');
-    }
-
-
-    function editOff() {
-        layer.selectAll('.note').remove();
-        layer.style('display', 'none');
-    }
-
 
+    // Loosely-coupled osm service for fetching notes.
     function getService() {
-        if (services.osm && !_notes) {
-            _notes = services.osm;
-            _notes.on('loadedNotes', throttledRedraw);
-        } else if (!services.osm && _notes) {
-            _notes = null;
+        if (services.osm && !_osmService) {
+            _osmService = services.osm;
+            _osmService.on('loadedNotes', throttledRedraw);
+        } else if (!services.osm && _osmService) {
+            _osmService = null;
         }
 
-        return _notes;
+        return _osmService;
     }
 
 
-    function showLayer() {
+    // Show the notes
+    function editOn() {
+        if (!_notesVisible) {
+            _notesVisible = true;
+            drawLayer
+                .style('display', 'block');
+        }
+    }
+
+
+    // Immediately remove the notes and their touch targets
+    function editOff() {
+        if (_notesVisible) {
+            _notesVisible = false;
+            drawLayer
+                .style('display', 'none');
+            drawLayer.selectAll('.note')
+                .remove();
+            touchLayer.selectAll('.note')
+                .remove();
+        }
+    }
+
+
+    // Enable the layer.  This shows the notes and transitions them to visible.
+    function layerOn() {
         editOn();
 
-        layer
-            .classed('disabled', false)
+        drawLayer
             .style('opacity', 0)
             .transition()
             .duration(250)
@@ -66,30 +81,35 @@ export function svgNotes(projection, context, dispatch) {
     }
 
 
-    function hideLayer() {
-        editOff();
-
+    // Disable the layer.  This transitions the layer invisible and then hides the notes.
+    function layerOff() {
         throttledRedraw.cancel();
-        layer.interrupt();
+        drawLayer.interrupt();
+        touchLayer.selectAll('.note')
+            .remove();
 
-        layer
+        drawLayer
             .transition()
             .duration(250)
             .style('opacity', 0)
             .on('end interrupt', function () {
-                layer.classed('disabled', true);
+                editOff();
                 dispatch.call('change');
             });
-
     }
 
 
-    function update() {
+    // Update the note markers
+    function updateMarkers() {
+        if (!_notesVisible || !_notesEnabled) return;
+
         var service = getService();
         var selectedID = context.selectedNoteID();
         var data = (service ? service.notes(projection) : []);
-        var transform = svgPointTransform(projection);
-        var notes = layer.selectAll('.note')
+        var getTransform = svgPointTransform(projection);
+
+        // Draw markers..
+        var notes = drawLayer.selectAll('.note')
             .data(data, function(d) { return d.status + d.id; });
 
         // exit
@@ -139,51 +159,89 @@ export function svgNotes(projection, context, dispatch) {
         // update
         notes
             .merge(notesEnter)
-            .sort(function(a, b) {
-                return (a.id === selectedID) ? 1
-                    : (b.id === selectedID) ? -1
-                    : b.loc[1] - a.loc[1];  // sort Y
+            .sort(sortY)
+            .classed('selected', function(d) {
+                var mode = context.mode();
+                var isMoving = mode && mode.id === 'drag-note';  // no shadows when dragging
+                return !isMoving && d.id === selectedID;
             })
-            .classed('selected', function(d) { return d.id === selectedID; })
-            .attr('transform', transform);
+            .attr('transform', getTransform);
+
+
+        // Draw targets..
+        if (touchLayer.empty()) return;
+        var fillClass = context.getDebug('target') ? 'pink ' : 'nocolor ';
+
+        var targets = touchLayer.selectAll('.note')
+            .data(data, function(d) { return d.id; });
+
+        // exit
+        targets.exit()
+            .remove();
+
+        // enter/update
+        targets.enter()
+            .append('rect')
+            .attr('width', '20px')
+            .attr('height', '20px')
+            .attr('x', '-8px')
+            .attr('y', '-22px')
+            .merge(targets)
+            .sort(sortY)
+            .attr('class', function(d) {
+                var newClass = (d.id < 0 ? 'new' : '');
+                return 'note target note-' + d.id + ' ' + fillClass + newClass;
+            })
+            .attr('transform', getTransform);
+
+
+        function sortY(a, b) {
+            return (a.id === selectedID) ? 1 : (b.id === selectedID) ? -1 : b.loc[1] - a.loc[1];
+        }
     }
 
 
+    // Draw the notes layer and schedule loading notes and updating markers.
     function drawNotes(selection) {
-        var enabled = svgNotes.enabled;
         var service = getService();
 
-        layer = selection.selectAll('.layer-notes')
+        var surface = context.surface();
+        if (surface && !surface.empty()) {
+            touchLayer = surface.selectAll('.data-layer.touch .layer-touch.markers');
+        }
+
+        drawLayer = selection.selectAll('.layer-notes')
             .data(service ? [0] : []);
 
-        layer.exit()
+        drawLayer.exit()
             .remove();
 
-        layer.enter()
+        drawLayer.enter()
             .append('g')
             .attr('class', 'layer-notes')
-            .style('display', enabled ? 'block' : 'none')
-            .merge(layer);
+            .style('display', _notesEnabled ? 'block' : 'none');
 
-        if (enabled) {
+        if (_notesEnabled) {
             if (service && ~~context.map().zoom() >= minZoom) {
                 editOn();
                 service.loadNotes(projection);
-                update();
+                updateMarkers();
             } else {
                 editOff();
             }
         }
     }
 
-    drawNotes.enabled = function(val) {
-        if (!arguments.length) return svgNotes.enabled;
 
-        svgNotes.enabled = val;
-        if (svgNotes.enabled) {
-            showLayer();
+    // Toggles the layer on and off
+    drawNotes.enabled = function(val) {
+        if (!arguments.length) return _notesEnabled;
+
+        _notesEnabled = val;
+        if (_notesEnabled) {
+            layerOn();
         } else {
-            hideLayer();
+            layerOff();
             if (context.selectedNoteID()) {
                 context.enter(modeBrowse(context));
             }
@@ -193,6 +251,6 @@ export function svgNotes(projection, context, dispatch) {
         return this;
     };
 
-    init();
+
     return drawNotes;
 }
diff --git a/modules/svg/touch.js b/modules/svg/touch.js
index 96bb1c871..860f95bd1 100644
--- a/modules/svg/touch.js
+++ b/modules/svg/touch.js
@@ -2,7 +2,7 @@ export function svgTouch() {
 
     function drawTouch(selection) {
         selection.selectAll('.layer-touch')
-            .data(['areas', 'lines', 'points', 'turns', 'notes'])
+            .data(['areas', 'lines', 'points', 'turns', 'markers'])
             .enter()
             .append('g')
             .attr('class', function(d) { return 'layer-touch ' + d; });