Skip to content

Added Support for Touch devices for MapD Draw #29

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.

Sign up for GitHub

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

Open
chinmaydalvi wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added Support for Touch devices for MapD Draw #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62b582a
added support for touch devices
Jul 1, 2019
39fff9b
fixed the warning of in case of touch events
Jul 27, 2019
1ec6668
added support for double click on polygon shape to select the edges a…
Jul 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
240 changes: 93 additions & 147 deletions dist/mapd-draw-dev.js
View file
Open in desktop

Large diffs are not rendered by default.

39 changes: 33 additions & 6 deletions dist/mapd-draw.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/mapd-draw.js.map
View file
Open in desktop

Large diffs are not rendered by default.

44 changes: 38 additions & 6 deletions src/engine/draw-engine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import Camera2d from "../view/camera2d"
import EventHander from "../util/event-handler"
import ResizeSensor from "css-element-queries/src/ResizeSensor"

const mouseevents = ["mousedown", "mouseup", "mousemove", "click", "dblclick", "mouseover", "mouseout"]
const mouseevents = ["mousedown", "mouseup", "mousemove", "click", "dblclick", "mouseover", "mouseout", "touchstart", "touchend", "touchmove"]
const marginProps = ["top", "bottom", "left", "right"]

export const EventConstants = {
Expand All @@ -23,7 +23,10 @@ export const EventConstants = {
MOUSEOVER: "mouseover",
MOUSEOUT: "mouseout",
SHAPE_ADD: "shape:add",
SHAPE_DELETE: "shape:delete"
SHAPE_DELETE: "shape:delete",
TOUCHSTART: "touchstart",
TOUCHEND: "touchend",
TOUCHMOVE: "touchmove"
}

class DrawStyleState extends BasicStyle {
Expand Down Expand Up @@ -111,7 +114,7 @@ function createCanvas(parent) {
const canvas = document.createElement("canvas")
const canvasContext = canvas.getContext("2d")
const ratio = CanvasUtils.makeCanvasAutoHighDPI(canvasContext)
// const ratio = 1
// const ratio = 1

// add class?
addClass(canvas, "mapd-draw-canvas")
Expand Down Expand Up @@ -239,6 +242,15 @@ export default class DrawEngine extends EventHander {
})
}

_touchstartCB(event) {
if (event.target !== this._parent) {
return
}
this.fire("touchstart", {
originalEvent: event
})
}

_mouseupCB(event) {
if (event.target !== this._parent) {
return
Expand All @@ -249,6 +261,16 @@ export default class DrawEngine extends EventHander {
})
}

_touchendCB(event) {
if (event.target !== this._parent) {
return
}

this.fire("touchend", {
originalEvent: event
})
}

_mousemoveCB(event) {
if (event.target !== this._parent) {
return
Expand All @@ -259,6 +281,16 @@ export default class DrawEngine extends EventHander {
})
}

_touchmoveCB(event) {
if (event.target !== this._parent) {
return
}

this.fire("touchmove", {
originalEvent: event
})
}

_clickCB(event) {
if (event.target !== this._parent) {
return
Expand Down Expand Up @@ -367,7 +399,7 @@ export default class DrawEngine extends EventHander {
this.registerEvents([EventConstants.SHAPE_ADD, EventConstants.SHAPE_DELETE])

bindAll(["_reorderCb", "_rerenderCb"], this)
bindAll(["_mousedownCB", "_mouseupCB", "_mousemoveCB", "_clickCB", "_dblclickCB", "_mouseoverCB", "_mouseoutCB"], this)
bindAll(["_mousedownCB", "_mouseupCB", "_mousemoveCB", "_clickCB", "_dblclickCB", "_mouseoverCB", "_mouseoutCB", "_touchstartCB", "_touchendCB", "_touchmoveCB"], this)

this._renderFrameCb = this.renderAll.bind(this)
this._renderRequestId = 0
Expand Down Expand Up @@ -401,7 +433,7 @@ export default class DrawEngine extends EventHander {
get sortedShapes() {
if (this._reorderedObjIdxs.size) {
console.assert(this._sortedObjs.length === this._objects.size,
`Size mismatch when rendering objets. Something got out of sync - sorted objs length: ${this._sortedObjs.length}, objects length: ${this._objects.size}`)
`Size mismatch when rendering objets. Something got out of sync - sorted objs length: ${this._sortedObjs.length}, objects length: ${this._objects.size}`)

// if (this._reorderedObjIdxs.length / this._sortedObjs.length > 0.7) {
// // might as well just resort the whole thing over
Expand Down Expand Up @@ -578,7 +610,7 @@ export default class DrawEngine extends EventHander {

renderAll() {
const ctx = this._drawCtx
// ctx.clearRect(0, 0, this.width, this.height)
// ctx.clearRect(0, 0, this.width, this.height)
ctx.clearRect(0, 0, this._drawCanvas.offsetWidth, this._drawCanvas.offsetHeight)

if (!this._objects.size) {
Expand Down
116 changes: 111 additions & 5 deletions src/engine/shape-builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,27 @@ const defaultXformStyle = {
strokeWidth: 2
}

const EventsTypes = {
MOUSEDOWN: "mousedown",
MOUSEUP: "mouseup",
MOUSEMOVE: "mousemove",
TOUCHSTART: "touchstart",
TOUCHEND: "touchend",
TOUCHMOVE: "touchmove"
}

const DOUBLE_CLICK_DELAY = 600 // To detect the double click in case of touch screen
Copy link
Contributor

@christopher-w-root christopher-w-root Oct 14, 2019

Choose a reason for hiding this comment

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

600 ms seems a bit too much for a double-click. How did you come up with this magic number? Did you feel it was appropriate in your testing? My first thought is that it should be at max 300ms.


// This method will Add clientX, clientY & offsetX and offsetY for Touch events
function getTouchCoordinates(event, canvas) {
event.clientX = event.touches[0].clientX
Copy link
Contributor

@christopher-w-root christopher-w-root Oct 14, 2019

Choose a reason for hiding this comment

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

I'm concerned whether grabbing the index 0 location of the touches will be consistent and produce stable interactions. For example, what would happen if you did a two-finger touch, and you removed the touch from the first finger, but kept the second finger in contact and moved it around. Or what would happen if you first pressed with one finger, then pressed with a second finger, then removed the first finger, and moved the second finger around.

I wonder if it would be best to limit the scope of the touch event handling to just a single touch. Anything more than that would cancel the event. Thoughts?

event.clientY = event.touches[0].clientY
const element = canvas.getBoundingClientRect()
event.offsetX = event.touches[0].clientX - element.left
event.offsetY = event.touches[0].clientY - element.top
return event
}

function inCanvas(canvas, x, y) {
const domrect = canvas.getBoundingClientRect()
let localX = 0
Expand Down Expand Up @@ -96,7 +117,7 @@ function selectShape(selectedShape, sortedShapes, currSelectedShapes, selectStyl
selectedShape.zIndex = maxZ + 1
BasicStyle.copyBasicStyle(selectStyle, selectedShape)
selectedShape.selected = true
// const dimensions = selectedShape.getDimensions()
// const dimensions = selectedShape.getDimensions()

let newSelectShape = null
if (selectOpts.scalable || selectOpts.rotatable) {
Expand Down Expand Up @@ -208,7 +229,29 @@ function updateCursorPosition(_event, target) {
}

export default class ShapeBuilder extends DrawEngine {

_touchstartCB(event) {
this._mousedownCB(event)
}

_touchmoveCB(event) {
this._mousemoveCB(event)
}

_touchendCB(event) {
this._mouseupCB(event)
}

_mousedownCB(event) {
this.setDenyMouseEventFlag(event)
if (this.denyMouseEvent && !event.touches) {
return
}
if (event.touches) {
event = getTouchCoordinates(event, this._drawCanvas)
this.previousEventObj = event // Assign event obj to variable to avoid the use it for touchend event
}

if (!inCanvas(this._drawCanvas, event.clientX, event.clientY)) {
return
}
Expand Down Expand Up @@ -300,14 +343,27 @@ export default class ShapeBuilder extends DrawEngine {
shapes: getSelectedObjsFromMap(this._selectedShapes)
})
}
event.preventDefault()
if (!event.touches) {
event.preventDefault()
}
}
}

_mouseupCB(event) {
if (this.denyMouseEvent && !event.touches) {
this.setDenyMouseEventFlag(event)
return // Returning on next line to avoid ESLint error
}
if (event.touches) {
// Use previously assigned event obj to get the offsetX & Y and clientX & Y calculation
event = this.previousEventObj
Copy link
Contributor

@christopher-w-root christopher-w-root Oct 14, 2019

Choose a reason for hiding this comment

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

Can you explain your reasoning for using the previous event coordinates here. What's wrong with just calling getTouchCoordinates on this event?

}

if (this._dragInfo && this._dragInfo.shape) {
event.stopImmediatePropagation()
event.preventDefault()
if (event.cancelable) {
event.preventDefault()
}
const canvas = document.querySelector(`${`#${this._parent.id} > canvas`}`)
if (canvas === null) {
this._parent.removeEventListener("mouseout", hideCursor)
Expand Down Expand Up @@ -338,11 +394,34 @@ export default class ShapeBuilder extends DrawEngine {
if (selectedShape && !selectedShape.selected) {
const selectEventObj = selectShape(selectedShape, shapes, this._selectedShapes, this._selectStyle, this._xformStyle, selectedInfo)
this.fire(EventConstants.SELECTION_CHANGED, selectEventObj)
} else {
// If user clicks anywhere outside then allow the movement of Base Map (Parents Container)
this._makeParentElementMovable()
}
}
// Added Support for Double click
if (event.touches) {
if (event.cancelable) {
event.preventDefault()
}
if ((Date.now() - this.firstTapTime) < DOUBLE_CLICK_DELAY) {
this._dblclickCB(event)
Copy link
Contributor

@christopher-w-root christopher-w-root Oct 15, 2019

Choose a reason for hiding this comment

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

This looks ok. Did you verify that this works like a mouse double-click event. In particular I'm wondering if a mouse double-click event actually runs the mousedown/mouseup twice first before firing the double click event. If that's true, then this looks right because the touchstart/touchend would be fired twice before _dblclickCB is called.

}
}
this.firstTapTime = Date.now()
}

_mousemoveCB(event) {
this.setDenyMouseEventFlag(event)
if (this.denyMouseEvent && !event.touches) {
Copy link
Contributor

@christopher-w-root christopher-w-root Oct 15, 2019

Choose a reason for hiding this comment

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

there's a lot of checks for event.touches since you've collapsed the mouse/touch callbacks together. I think it'd be a bit more readable to turn this into a utility function: isTouchEvent(event). It would return true if the event was a touch event.

return
}

if (event.touches) {
event = getTouchCoordinates(event, this._drawCanvas)
this.previousEventObj = event // Assign event obj to variable to avoid the use it for touchend event
}

if (!(inCanvas(this._drawCanvas, event.clientX, event.clientY)) && !this._dragInfo) {
return
}
Expand All @@ -352,7 +431,9 @@ export default class ShapeBuilder extends DrawEngine {
addEventKeysToSelectedInfo(event, this._dragInfo)
transformSelectedShape(this._drawCanvas, event, this._dragInfo, this._camera)
event.stopImmediatePropagation()
event.preventDefault()
if (!event.touches) {
event.preventDefault()
}
} else if (!event.buttons && this._selectedShapes.size) {
Point2d.set(tmpPt1, event.offsetX, event.offsetY)
Point2d.transformMat2d(tmpPt2, tmpPt1, this._camera.screenToWorldMatrix)
Expand Down Expand Up @@ -522,7 +603,9 @@ export default class ShapeBuilder extends DrawEngine {
} else {
event.stopImmediatePropagation()
}
event.preventDefault()
if (event.cancelable) {
event.preventDefault()
}
}

_mouseoverCB() {
Expand Down Expand Up @@ -553,6 +636,19 @@ export default class ShapeBuilder extends DrawEngine {
this.timer = 0
}

// This function allow the movement of Parent Container (In our case it is Map) when user clicks anywhere on Map except on Shape
// As well as it's changes the icon of mouse for Desktop devices
_makeParentElementMovable() {
removeCustomCursor()
this._parent.style.cursor = "default" // Change the Cursor icon for desktop device
for (let j = 0; j < this._parent.childNodes.length; j += 1) {
this._parent.childNodes[j].style.cursor = "default" // Change the Cursor icon for desktop device
if (this._parent.childNodes[j].nodeName.toLowerCase() !== "canvas") {
this._parent.childNodes[j].style.pointerEvents = "auto" // Allow movemnet of parent container i.e Map
}
}
}
Comment on lines +639 to +650
Copy link
Contributor

@christopher-w-root christopher-w-root Oct 14, 2019

Choose a reason for hiding this comment

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

Can you further explain the intent of this? IIRC the original intent of the mapd-draw design was to stay out of the way of DOM layer and event propogation as much as possible and instead give responsibility to a middle-layer to manage. If it's just the cursor state of sub-dom elements, my first inclination is that should be handled by such a middle layer. For immerse, that middle-layer is in mapd-charting.


_renderShapes(ctx, drawShapes, camera) {
const worldToScreenMat = camera.worldToScreenMatrix
drawShapes.forEach(shape => {
Expand Down Expand Up @@ -704,6 +800,16 @@ export default class ShapeBuilder extends DrawEngine {
this._activated = false
return this
}

// This method is used to stop Mouse Event propagation Triggered from the Touch event
setDenyMouseEventFlag(event) {
if (event.touches) {
this.denyMouseEvent = true
} else if (event.type === EventsTypes.MOUSEUP) {
// set the Flag false at the end of mouse event i.e on MouseUp Event
this.denyMouseEvent = false
}
}
Comment on lines +804 to +812
Copy link
Contributor

@christopher-w-root christopher-w-root Oct 14, 2019

Choose a reason for hiding this comment

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

Can you explain the scenario you're trying to avoid with setting the denyMouseEvent property? If I'm following along appropriately, it seems you're trying to avoid mouse/touch conflicts with a mouse and touch-screen configured system. That's a configuration we should be concerned with, but the logic seems strange.

So, if a touch event comes thru, all mouse events are ignored and continue to be ignored until a mouse click comes around again. So a mouse click needs to happen to re-activate mouse events. At first glance this seems unintuitive and I think we could avoid such secret interactions with other currently existing state, such as _dragInfo. But I may not quite get what you're trying to do. An explanation of your intent here might help my understanding.

}

Object.assign(EventConstants, DrawEngine.EventConstants)
Expand Down