Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DragElement: initialize clientX and clientY for touch events #2997

Merged
merged 3 commits into from
Sep 18, 2018

Conversation

jonfunkhouser
Copy link
Contributor

Fixes #1967

Fx.hover() uses evt.clientX and evt.clientY to calculate an offset for the hover. In a TouchEvent, these properties are undefined, which results in the hover being rejected.

https://github.com/plotly/plotly.js/blob/master/src/components/fx/hover.js#L285

this sets clientX and clientY to the values from the first touch in TouchEvent.changedTouches
@etpinard
Copy link
Contributor

Thanks very much for this PR @jonfunkhouser !

It's probably the least-bad way to fix the regressions introduced in #1804. If I understand correctly, plotly_hover is now emitted correctly on touch and there's no way to trigger plotly_click on touch?

At some point, we'll probably need another attribute (e.g. layout.touchmode) or maybe new mobile-specific modebar buttons that could toggle between "hover on touch" / "click on touch" / "select on touch", but this seems like the correct "default" behavior, so 👌


Now, @jonfunkhouser have you checked that all the features added in #1804 still work on your branch?

@etpinard etpinard added bug something broken status: in progress labels Sep 11, 2018
@jonfunkhouser
Copy link
Contributor Author

It ends up emitting plotly_click, not plotly_hover.

Fx.click() is called by dragElement's onDone() handler, which then calls hover() to determine what point was clicked: https://github.com/plotly/plotly.js/blob/master/src/components/fx/click.js#L22

The above hover() call was returning before gd._hoverData could be set, which then stopped plotly_clicked from being emitted.

I'm just now starting to validate the features in #1804, I'll reply again when that's complete.

@etpinard
Copy link
Contributor

I'm just now starting to validate the features in #1804, I'll reply again when that's complete.

Thanks very much !

@jonfunkhouser
Copy link
Contributor Author

I just pushed three sample scatter charts here: https://jonfunkhouser.github.io/plotly-mobile/

The first chart demonstrates default behavior, including plotly_clicked events on mobile. The second chart is set to box selection, and the third chart is set to lasso selection. Box and lasso selection still emit plotly_selected events.

@etpinard
Copy link
Contributor

@jonfunkhouser I was able to test your patch on my Android phone. I must say: this is a wonderful PR.

I confirm that touching on points triggers the plotly_click event and does not trigger plotly_hover nor plotly_unhover. This is in fact how plotly.js behaved in v1.28.3 the last release before #1804 which slipped in this regression. So this is fantastic 🎉

Now, there's still no way to "hover" when layout.dragmode is either 'select' and 'lasso'. This is different from the behavior in v1.28.3 where "hover" worked in any dragmode, but back then there was no way to select points!

Applying your patch on-top of the latest master (to include click-to-select from #2944) and setting layout.clickmode: 'select' disables "hover" on this branch. That's ok as that never worked. This would be a cool feature though.

In brief, this PR:


It would be nice to lock the touch hover behavior down so that regression like #1967 don't happen again. We do have few touch tests, for example this one that tests selections:

it('@flaky should trigger selecting/selected/deselect events for touches', function(done) {
resetEvents(gd);
drag(lassoPath, {type: 'touch'});
selectedPromise.then(function() {
expect(selectingCnt).toBe(3, 'with the correct selecting count');
assertEventData(selectingData.points, [{
curveNumber: 0,
pointNumber: 10,
pointIndex: 10,
x: 0.099,
y: 2.75
}], 'with the correct selecting points (1)');
expect(selectedCnt).toBe(1, 'with the correct selected count');
assertEventData(selectedData.points, [{
curveNumber: 0,
pointNumber: 10,
pointIndex: 10,
x: 0.099,
y: 2.75,
}], 'with the correct selected points (2)');
return doubleClick(250, 200);
})
.then(deselectPromise)
.then(function() {
expect(doubleClickData).toBe(null, 'with the correct deselect data');
})
.catch(failTest)
.then(done);
});
});

@jonfunkhouser Would you be interesting in adding a touch hover test somewhere hover_label_test.js?

@etpinard
Copy link
Contributor

etpinard commented Sep 18, 2018

Thanks for adding that test @jonfunkhouser !!!

I pushed a small commit to fix up npm run test-jasmine fca6f9b

I'll now merge this PR and release it as part of 1.41.1

@etpinard etpinard merged commit fe76a86 into plotly:master Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants