Added touchmove support to slider component

[keul]

See #5855

Note: the pull request is for the master branch, but I'm wondering if another one can be accepted for the old 1.x branch

[alexcjohnson]

@keul this looks good! Re: 1.x - we'd really rather keep our efforts focused on v2 except for security fixes. Is there something specific preventing you from updating to v2?

The one thing we should add is a test. Perhaps we could copy or extend this test:

plotly.js/test/jasmine/tests/sliders_test.js

Lines 470 to 508 in e27ca3c

	it('should respond to mouse clicks', function(done) {
	var firstGroup = gd._fullLayout._infolayer.select('.' + constants.railTouchRectClass);
	var firstGrip = gd._fullLayout._infolayer.select('.' + constants.gripRectClass);
	var railNode = firstGroup.node();
	var touchRect = railNode.getBoundingClientRect();
	var originalFill = firstGrip.node().style.fill;
	// Dispatch a click on the right side of the bar:
	railNode.dispatchEvent(new MouseEvent('mousedown', {
	clientY: touchRect.top + 5,
	clientX: touchRect.left + touchRect.width - 5,
	}));
	expect(mockCopy.layout.sliders[0].active).toEqual(5);
	var mousedownFill = firstGrip.node().style.fill;
	expect(mousedownFill).not.toEqual(originalFill);
	// Drag to the left side:
	gd.dispatchEvent(new MouseEvent('mousemove', {
	clientY: touchRect.top + 5,
	clientX: touchRect.left + 5,
	}));
	var mousemoveFill = firstGrip.node().style.fill;
	expect(mousemoveFill).toEqual(mousedownFill);
	delay(100)()
	.then(function() {
	expect(mockCopy.layout.sliders[0].active).toEqual(0);
	gd.dispatchEvent(new MouseEvent('mouseup'));
	var mouseupFill = firstGrip.node().style.fill;
	expect(mouseupFill).toEqual(originalFill);
	expect(mockCopy.layout.sliders[0].active).toEqual(0);
	})
	.then(done, done.fail);
	});

Note we have assets/touchEvent (and assets/mouseEvent) which were probably created after sliders_test but help to ensure these events are more realistic without making them harder to construct, like here in the shape drawing test:

plotly.js/test/jasmine/tests/draw_newshape_test.js

Lines 11 to 38 in e27ca3c

	var mouseEvent = require('../assets/mouse_event');
	var touchEvent = require('../assets/touch_event');
	var click = require('../assets/click');
	function drag(path, options) {
	var len = path.length;
	if(!options) options = { type: 'mouse' };
	if(options.type === 'touch') {
	touchEvent('touchstart', path[0][0], path[0][1], options);
	path.slice(1, len).forEach(function(pt) {
	touchEvent('touchmove', pt[0], pt[1], options);
	});
	touchEvent('touchend', path[len - 1][0], path[len - 1][1], options);
	return;
	}
	mouseEvent('mousemove', path[0][0], path[0][1], options);
	mouseEvent('mousedown', path[0][0], path[0][1], options);
	path.slice(1, len).forEach(function(pt) {
	mouseEvent('mousemove', pt[0], pt[1], options);
	});
	mouseEvent('mouseup', path[len - 1][0], path[len - 1][1], options);

[keul]

@alexcjohnson

Re: 1.x - we'd really rather keep our efforts focused on v2 except for security fixes. Is there something specific preventing you from updating to v2?

Nothing but not knowing the compatibility with server side modules. My use of plotly.js depends on the plotly Python module, so I try to keep the same version distributed with the lib itself.
But this is not a problem: I can continue providing my patch until I finally migrate to v2.

---

I'll take a look adding tests (not sure I'll be able to solve this before summer vacation). Thanks for references!

[archmoj]

Thanks for the PR.

Please create draftlogs/5856_add.md as described in this README.
Something like:

 - Add touch support to `slider` component [[#5856](https://github.com/plotly/plotly.js/pull/5856)], 
   with thanks to @keul for the contribution!

[keul]

I was forced to complete the signature of the event or simulating the touch event on the slider will generate a very strange behavior (slider was always moved on the start position).

[archmoj]

Do we really need to add these? It looks the test passes without it.

[keul]

@archmoj not on my machine (MacOS). Just tested again removing these lines and I have the failure i fixed adding this

[keul]

BTW: I'm able to reproduce the issue also outside Jasmine/jsdom.

I'm 100% for removing this if you think it's just an issue on my Chrome… but seems really strange to me.

[archmoj]

Thanks for testing.
LGTM 🏅
Over to @alexcjohnson

[keul]

@alexcjohnson I added the test.

But now there's another test that is failing: 5fe96ec#diff-09276d81835ab1fc83e8eb085e4faae2de13348f4be40367cb03276a3f651cbfR549

Seems like should respond to touch interactions is not really an isolated test. It starts to fails as soon as I add this new test, and it's back to normal when removing it, but I'm not able to understand what's wrong.

[archmoj]

@alexcjohnson I added the test.

But now there's another test that is failing: 5fe96ec#diff-09276d81835ab1fc83e8eb085e4faae2de13348f4be40367cb03276a3f651cbfR549

Seems like should respond to touch interactions is not really an isolated test. It starts to fails as soon as I add this new test, and it's back to normal when removing it, but I'm not able to understand what's wrong.

What happens if you add it to the end?

[keul]

What happens if you add it to the end?

@archmoj still fails

[archmoj]

The test passes on my machine.
To get all the greens, please fetch upstream/master in your fork, then pull and merge it into this branch.

[keul]

@archmoj really sorry for the noise… It works now 🙇

[alexcjohnson]

Love it. 💃

[archmoj]

We will wait for @nicolaskruchten before merging.