mapbox-gl-draw icon indicating copy to clipboard operation
mapbox-gl-draw copied to clipboard

Drawing a LineString prematurely completes final double tap on touch devices

Open jaybo opened this issue 1 year ago • 2 comments

mapbox-gl-js version: 3.1.2 mapbox-gl-draw version: 1.4.3

Steps to Trigger Behavior

  1. Begin drawing a LineString.
  2. Note that completing the line with a mouse click twice on the same vertex is a precisely triggered operation and works great.
  3. Note that completing the line with a touch device often happens prematurely. This happens because multiple touchStart messages often happen within the 25 pixel touchBuffer quite accidentally in the course of attempting to make a single tap. Reducing the size of touchBuffer slightly improves the situation, but it is still a significant problem on an iPhone 13, and presumably most touch devices.

Expected Behavior

Drawing lines should work on touch devices.

Actual Behavior

Drawing lines terminates prematurely in ~25% of attempts.

Diagnosis

In draw_line_string.js line drawing completion is handled identically for both mouse click and touch.

DrawLineString.onTap = DrawLineString.onClick = function(state, e) {
  if (CommonSelectors.isVertex(e)) return this.clickOnVertex(state, e);
  this.clickAnywhere(state, e);
};

I believe that to solve this problem, a debounce needs to be applied to only the onTap path, so that additional taps within (say) 300mS of the first tap are ignored.

jaybo avatar Mar 03 '24 09:03 jaybo

I tried adding the following tap debounce code to draw_line_string.js and it seems to have completely solved the problem:

DrawLineString.lastTapTime = Date.now();

DrawLineString.onTap = function (state, e) {
  const now = Date.now();
  const tapDebounceTimeMS = 300;
  if (now - this.lastTapTime < tapDebounceTimeMS) {
    return;
  }
  this.lastTapTime = now;
  if (CommonSelectors.isVertex(e)) return this.clickOnVertex(state, e);
  this.clickAnywhere(state, e);
};

To get tests to run, I had to add a 400mS delay immediately before every instance of touchTap(...) in draw_line_string.test.js. I'm sure my naive sleep() below isn't the best solution, but I don't really understand how to best implement such a delay within the testing framework:

const tapDebounceDelayMS = 400;

function sleep(milliseconds) {
  const date = Date.now();
  let currentDate = null;
  do {
    currentDate = Date.now();
  } while (currentDate - date < milliseconds);
}

Used like:

      sleep(tapDebounceDelayMS);
      touchTap(map, makeTouchEvent(100, 200));

Although I didn't try it, all of the Polygon code should have the same treatment.

jaybo avatar Mar 04 '24 01:03 jaybo