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

Add support for touch and hold gesture #7286

Merged
merged 11 commits into from
Mar 20, 2019
Merged

Conversation

adridavid
Copy link
Contributor

Currently, the only available event types in touch mode are LEFT_DOWN/LEFT_UP/LEFT_CLICK and LEFT_DOUBLE_CLICK. Since the touch and hold gesture is widely used on mobile devices, this is a pull request to add support for this event type.

Generally the touch and hold gesture is associated with the right click. For this reason, I linked it with the RIGHT_CLICK event type. But we may also think it as a new event type (for instance TOUCH_HOLD).

The amount of time before a touch becomes a touch and hold can be customized. It is set by default to 1,5 seconds.

@cesium-concierge
Copy link

Thank you so much for the pull request @adridavid! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ❌ Missing CLA.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@adridavid adridavid changed the title Touch events Add support for touch and hold gesture Nov 26, 2018
@hpinkos
Copy link
Contributor

hpinkos commented Nov 26, 2018

Thanks for the pull requests @adridavid! Please send us a CLA so we can review this and #7285
You can find instructions here: Contributor License Agreement

We should be able to review this after the release next week

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 28, 2018

Thanks again @adridavid, we received your CLA.

@hpinkos
Copy link
Contributor

hpinkos commented Dec 27, 2018

@adridavid please merge in master when you have a chance. Thanks!

@cesium-concierge
Copy link

Thanks again for your contribution @adridavid!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 4, 2019

Thanks @adridavid. Can you merge in master again and move your CHANGES.md change under the 1.55 section?

@emackey can you review this?

@adridavid
Copy link
Contributor Author

I just merged in master and updated CHANGES.md @hpinkos .

@emackey
Copy link
Contributor

emackey commented Feb 11, 2019

Overall this seems great, and I love that it's wired into the RIGHT_CLICK event because that's usually what touch-hold equates to on desktop. Cesium doesn't make much use of that event, but our customers might.

One wrinkle I had while testing this is it was very tricky for me to hold my finger steady enough (on a Surface Pro 4 with a fairly large screen) such that I triggered the event rather than just panning the globe. Is there a way to increase the robustness of that? (Obviously without making the pan wait for it, but maybe by allowing the pan to travel some larger number of pixels without voiding it). Just a suggestion.

Here's a Sandcastle I was using for testing. It's the picking demo, modded to use right-clicking, which on this branch can be done with an actual mouse or by touch-hold.

var viewer = new Cesium.Viewer('cesiumContainer', {
    selectionIndicator : false,
    infoBox : false
});

var scene = viewer.scene;
if (!scene.pickPositionSupported) {
    console.log('This browser does not support pickPosition.');
}

var handler;

Sandcastle.addDefaultToolbarButton('Show Cartographic Position on Right-click', function() {
    var entity = viewer.entities.add({
        label : {
            show : false,
            showBackground : true,
            font : '14px monospace',
            horizontalOrigin : Cesium.HorizontalOrigin.LEFT,
            verticalOrigin : Cesium.VerticalOrigin.TOP,
            pixelOffset : new Cesium.Cartesian2(15, 0)
        }
    });

    // Right-click or Touch-hold the globe to see the cartographic position
    handler = new Cesium.ScreenSpaceEventHandler(scene.canvas);
    handler.setInputAction(function(movement) {
        var cartesian = viewer.camera.pickEllipsoid(movement.position, scene.globe.ellipsoid);
        if (cartesian) {
            var cartographic = Cesium.Cartographic.fromCartesian(cartesian);
            var longitudeString = Cesium.Math.toDegrees(cartographic.longitude).toFixed(2);
            var latitudeString = Cesium.Math.toDegrees(cartographic.latitude).toFixed(2);

            entity.position = cartesian;
            entity.label.show = true;
            entity.label.text =
                'Lon: ' + ('   ' + longitudeString).slice(-7) + '\u00B0' +
                '\nLat: ' + ('   ' + latitudeString).slice(-7) + '\u00B0';
        } else {
            entity.label.show = false;
        }
    }, Cesium.ScreenSpaceEventType.RIGHT_CLICK);
});

Sandcastle.reset = function() {
    viewer.entities.removeAll();
    handler = handler && handler.destroy();
};

localhost:8080 Sandcastle demo

@adridavid
Copy link
Contributor Author

Thank you @emackey !

Concerning the number of pixels without voiding the event, I wanted it to be consistent with the simple click and I used the same behavior as for left click in touch mode :
https://github.com/AnalyticalGraphicsInc/cesium/blob/72cd1e799bc09cf24bbda2919b6a087fa2fbfad5/Source/Core/ScreenSpaceEventHandler.js#L136-L144

The pixel tolerance was previously set to 5, but there is a comment that suggests it should be revisited :
https://github.com/AnalyticalGraphicsInc/cesium/blob/72cd1e799bc09cf24bbda2919b6a087fa2fbfad5/Source/Core/ScreenSpaceEventHandler.js#L717-L719

Maybe we could use a second value which would be greater for the touch and hold event. However, I don't know which tolerance would be a good choice to handle correctly all screen sizes.

@emackey
Copy link
Contributor

emackey commented Feb 11, 2019

@adridavid Do you think it's worth adding a this._touchPixelTolerance, maybe set to 25 or some such, and using that just for the new touch-hold value here in this PR? That way it can be tweaked and refined later.

@adridavid
Copy link
Contributor Author

@emackey I did the change.

@cesium-concierge
Copy link

Thanks again for your contribution @adridavid!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@hpinkos
Copy link
Contributor

hpinkos commented Mar 18, 2019

@emackey what's the status of this?

@emackey
Copy link
Contributor

emackey commented Mar 18, 2019

Sorry, lost track of this, I have to test it at home because I don't have a touch screen at work. I was pretty close to merging it before, so I think it's probably ready now, just want to test it on a touch screen once more.

Copy link
Contributor

@emackey emackey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @adridavid!

@emackey emackey merged commit ead9f36 into CesiumGS:master Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants