Fix a label dragging bug

Author: mbloch

browser-tests/label-tool.spec.mjs

@@ -1443,6 +1443,48 @@ test('the justification follows the text across its anchor', async function({pag
   expect(errors).toEqual([]);
 });
 
+test('an aligned label is dragged where it is drawn', async function({page}) {
+  // A label carrying a label-align is drawn with the anchor its alignment
+  // gives it, and the drag writes text-anchor=start against the correction the
+  // renderer applies. The preview used to write that stored value onto the
+  // rendered text, which drew a right-aligned label a full width right of the
+  // pointer and a centred one half a width -- and dropped it back into place
+  // on release, so the label appeared to jump left the moment it was let go.
+  var errors = collectPageErrors(page);
+  await loadFixture(page, FIXTURE);
+
+  await clickMap(page, 0.45, 0.45);
+  await writeLabel(page, 'Winnemucca');
+  await disarmTool(page);
+  await clickLabel(page, 0);
+  await setDragMode(page, 'draggable');
+
+  for (var align of ['right', 'center']) {
+    await page.locator('.text-style-panel .label-align-buttons [data-align="' +
+      align + '"]').click();
+    await page.waitForTimeout(200);
+    await hoverNothing(page);
+    var before = await getLabelBox(page, 0);
+    var from = await getGlyphPoint(page, 0, 0.5);
+
+    await page.mouse.move(from.x, from.y);
+    await page.waitForTimeout(80);
+    await page.mouse.down();
+    await page.mouse.move(from.x + 60, from.y - 30, {steps: 8});
+    // mid-drag: the text is under the pointer, not a width away from it
+    var dragging = await getLabelBox(page, 0);
+    expect(Math.abs(dragging.x - (before.x + 60))).toBeLessThan(2);
+
+    await page.mouse.up();
+    await page.waitForTimeout(250);
+    // and the command's redraw leaves it where the drag showed it
+    var after = await getLabelBox(page, 0);
+    expect(Math.abs(after.x - dragging.x)).toBeLessThan(2);
+    expect((await getLabelLayer(page)).records[0]['label-align']).toBe(align);
+  }
+  expect(errors).toEqual([]);
+});
+
 test('the drag mode is the tool\'s and not the label\'s', async function({page}) {
   // Nothing in the record says which segment is lit: a label with a position
   // can be dragged or not, and one that has been dragged stays dragged when

docs/development/label-tool-design.md

@@ -2792,6 +2792,14 @@ the block's width to hold the text still while its lines re-justify — so
 `start` is both the anchor whose `dx` is the left edge itself and the value that
 would leave the label where it is if the alignment were later removed.
 
+That is the value the drag *writes*, and not the one the label is *drawn* with
+while the drag is in progress, which is still the alignment's own. So
+`getOffsetDragValues()` returns both: `dx`/`text-anchor` for the command and
+`x`/`anchor` for the preview. Previewing with the stored pair drew a
+right-aligned label a full width to the right of the pointer, and a centred one
+half a width, for as long as the drag lasted — the text then dropped back into
+place on release, which read as the label jumping left the moment it was let go.
+
 A drag works in the label's own coordinate space, inside its symbol group,
 which is what `dx` and `dy` are measured in: pointer movement is divided by the
 symbol scale on the way in, or a drag would overshoot at any scale but 1.

src/gui/gui-label-offset.mjs

@@ -50,10 +50,15 @@ var PRECISION = 10;
 //   aligned: whether it carries a label-align (see below)
 // delta: {dx, dy} -- how far the pointer has moved, in the same space.
 //
-// Returns {dx, dy, text-anchor, x}, where x is where the text will be drawn
-// once the other three are written -- the same as dx except on an aligned
-// label. The three have to be written together: applying dx without its
-// text-anchor moves the text by half its own width or by all of it.
+// Returns two descriptions of the same placement:
+//
+//   dx, dy, text-anchor:  what the drag writes to the record. The three have to
+//     be written together: applying dx without its text-anchor moves the text
+//     by half its own width or by all of it.
+//   x, anchor:  where the text sits and how it is justified while it is drawn,
+//     which is what the preview writes onto the rendered label. The same as
+//     dx and text-anchor except on an aligned label, whose stored offset is
+//     measured against a justification it is not drawn with.
 export function getOffsetDragValues(start, delta) {
   var width = start.width > 0 ? start.width : 0;
   var drawnAnchor = normalizeAnchor(start.anchor);
@@ -72,12 +77,18 @@ export function getOffsetDragValues(start, delta) {
   // if the alignment were later removed.
   var anchor = start.aligned ? 'start' :
     width > 0 ? getAnchorForCentre(left + width / 2, width) : drawnAnchor;
+  // An aligned label goes on being drawn with the anchor its alignment gives
+  // it, whatever the drag writes: the alignment is unchanged, so the renderer
+  // will justify it the same way afterwards. Drawing it as 'start' instead
+  // would slide the text right by half its width or by all of it for the
+  // length of the drag, and drop it back on release.
+  var drawn = start.aligned ? drawnAnchor : anchor;
   return {
     dx: round(left + anchorOffsets[anchor] * width),
     dy: round(start.dy + delta.dy),
     'text-anchor': anchor,
-    x: round(start.aligned ? start.dx + delta.dx :
-      left + anchorOffsets[anchor] * width)
+    x: round(left + anchorOffsets[drawn] * width),
+    anchor: drawn
   };
 }
 

src/gui/gui-label-tool2.mjs

@@ -832,7 +832,10 @@ export function initLabelTool(gui, ext, hit) {
     // ended.
     setMultilineAttribute(nodes.text, 'x', o.values.x);
     nodes.text.setAttribute('y', o.values.dy);
-    nodes.text.setAttribute('text-anchor', o.values['text-anchor']);
+    // The anchor the label is drawn with, not the one the drag writes: on an
+    // aligned label the two differ, and the stored one belongs with the stored
+    // dx rather than with the x being previewed here.
+    nodes.text.setAttribute('text-anchor', o.values.anchor);
     // The cue is drawn around the text rather than moved with it, so it has to
     // be rebuilt to follow -- one getBBox on one label per mouse move.
     selection.refresh(true);

test/gui-label-offset-test.mjs

@@ -16,6 +16,12 @@ function leftEdge(values, width) {
   return getTextCentreOffset(values.dx, values['text-anchor'], width) - width / 2;
 }
 
+// How far to the left of x a justification puts the text, as a fraction of its
+// width -- the same table the module works from.
+function anchorOffset(anchor) {
+  return {start: 0, middle: 0.5, end: 1}[anchor];
+}
+
 describe('gui label offset drags', function() {
 
   describe('getOffsetDragValues()', function() {
@@ -90,6 +96,30 @@ describe('gui label offset drags', function() {
       var o = getOffsetDragValues(centred({dx: 20}), {dx: 5, dy: 0});
       assert.equal(o.x, o.dx);
     });
+
+    // The preview writes x and the anchor onto the rendered label, so the two
+    // have to describe the same placement. An aligned label keeps the anchor
+    // its alignment gives it; taking the stored 'start' instead drew the text
+    // half a width or a whole width to the right until the drag was released.
+    it('draws an aligned label with the justification it already had', function() {
+      var o = getOffsetDragValues(centred({dx: 20, aligned: true}), {dx: 5, dy: 0});
+      assert.equal(o.anchor, 'middle');
+      assert.equal(o['text-anchor'], 'start');
+    });
+
+    it('draws an aligned label where it was dragged to', function() {
+      ['start', 'middle', 'end'].forEach(function(anchor) {
+        var o = getOffsetDragValues(centred({dx: 20, anchor: anchor, aligned: true}),
+          {dx: 5, dy: 0});
+        // the preview's own left edge, from the pair it writes
+        assert.equal(o.x - anchorOffset(o.anchor) * 40, 20 + 5 - anchorOffset(anchor) * 40);
+      });
+    });
+
+    it('draws an unaligned label with the justification it writes', function() {
+      var o = getOffsetDragValues(centred(), {dx: 30, dy: 0});
+      assert.equal(o.anchor, o['text-anchor']);
+    });
   });
 
   describe('getAnchorForCentre()', function() {