Skip to content

Use pointer events #1840

Merged
merged 101 commits into from Mar 14, 2014

6 participants

@tsauerwein
OpenLayers member

This pull request bases the internal event system of OpenLayers on pointer events.

So far, there were separate interactions for touch and mouse events. Now, the interactions only receive pointer events. These pointer events are generated from a PointerEventHandler, which listens to mouse events, touch events or real pointer events (on IE 10 and 11). The code is based on the pointer events polyfill of the Polymer library.

All existing interactions have been ported to pointer events. The modify and draw feature interaction have been adapted by @ahocevar.

Also thanks to @fredj and @elemoine for the feedback!

Closes #1099

@fredj
OpenLayers member
fredj commented Mar 11, 2014

Should also fixes #984 and #987

@tschaub tschaub and 2 others commented on an outdated diff Mar 11, 2014
src/ol/interaction/pointerinteraction.js
+ if (this.handled_) {
+ if (mapBrowserPointerEvent.type ==
+ ol.MapBrowserEvent.EventType.POINTERDRAG) {
+ this.handlePointerDrag(mapBrowserEvent);
+ } else if (mapBrowserPointerEvent.type ==
+ ol.MapBrowserEvent.EventType.POINTERUP) {
+ this.handled_ = this.handlePointerUp(mapBrowserPointerEvent);
+ if (!this.handled_) {
+ view.setHint(ol.ViewHint.INTERACTING, -1);
+ }
+ }
+ }
+ if (mapBrowserPointerEvent.type == ol.MapBrowserEvent.EventType.POINTERDOWN) {
+ var handled = this.handlePointerDown(mapBrowserPointerEvent);
+ if (!this.handled_ && handled) {
+ view.setHint(ol.ViewHint.INTERACTING, 1);
@tschaub
OpenLayers member
tschaub added a note Mar 11, 2014

The "interacting" view hint is used elsewhere to mean that user generated events are actually modifying the view state. By setting this hint here, it means that all other interactions that return true from handlePointerDown should be modifying the view state.

Instead, I think that interactions that actually modify the view state should be responsible for setting the view hint. See also #1277.

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 12, 2014

This is the original logic from the previous TouchInteraction. But if you want to change it, we should handle it maybe in a separate pull request?

@elemoine
OpenLayers member
elemoine added a note Mar 12, 2014

Agree.

And having the specific interactions set/unset the interaction hint would require that the this.handled_ state is shared between the base class and the specific class.

@tschaub
OpenLayers member
tschaub added a note Mar 12, 2014

Agree.

With what?

What I'm saying is that an interaction that modifies the view state should set the interacting hint. This doesn't require sharing this.handled_.

Basically, only call view.setHint when you plan on modifying the view state. See also b0362b8.

If I wanted to create an interaction that extended ol.interaction.Pointer but didn't necessarily modify the view state (e.g. maybe it just draws a box), the interacting hint would be incorrectly set.

@elemoine
OpenLayers member
elemoine added a note Mar 12, 2014

With what?

I agree with @tsauerwein that this can be addressed with a separate pull request.

What I'm saying is that an interaction that modifies the view state should set the interacting hint. This doesn't require sharing this.handled_.

I think I understand what you are saying, and I agree with you, but I also think that this code/logic is sensitive and easy to break.

Some observations:

  • Currently we make sure we don't set the interacting hint again if we already set it. The this.handled_ variable is used for that. So if a pointerdown was handled already, then we will not set the interaction hint again if a second pointer is added to the screen.

  • In the case of the DragPan interaction we set the interacting hint even though handlePointerDown did not change the view state. We need to set the interacting it because handlePointerDown requested a render frame.

If I wanted to create an interaction that extended ol.interaction.Pointer but didn't necessarily modify the view state (e.g. maybe it just draws a box), the interacting hint would be incorrectly set.

True. We do set the interacting hint when drawing zoom boxes today, although it may not be necessary.

So, I agree with you that the specific pointer interactions should be responsible for setting/unsetting the interacting hint, but I think making the required changes is not trivial. Hence my recommendation to address this in the separate pull request. @tsauerwein "just" ported the interactions and kept the current "view hints" logic.

@elemoine
OpenLayers member
elemoine added a note Mar 12, 2014

Hmm... There may well be problems here. With b0362b8 the DragPan, DragRotateAndZoom, and DragRotate interactions do set/unset the interacting hints themselves. But these interactions now inherit from the Pointer interaction, which also sets/unsets the interacting hint. This means the interacting hint is set/unset twice on pointerdown/pointerup.

@tschaub
OpenLayers member
tschaub added a note Mar 13, 2014

Right, this is what I was suggesting we avoid. I think the interacting hint should only be set when modifying the view. This is more specific than the "handled" response. You might handle the event without actually intending to modify the view state.

@elemoine
OpenLayers member
elemoine added a note Mar 13, 2014
@tsauerwein
OpenLayers member
tsauerwein added a note Mar 13, 2014

Your changes look good to me, I added them to the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tschaub tschaub and 1 other commented on an outdated diff Mar 11, 2014
examples/modify-features.js
@@ -165,6 +166,18 @@ var vectorLayer = new ol.layer.Vector({
style: styleFunction
});
+// FIXME Handle this elsewhere - this is only to not render selected features
+// on the original layer, and it does not work for MultiGeometry geometries.
+vectorLayer.setRenderGeometryFunctions(new ol.Collection([function(geometry) {
@tschaub
OpenLayers member
tschaub added a note Mar 11, 2014

Should these changes to the modify features example be removed (or handled in a separate PR)?

@ahocevar
OpenLayers member
ahocevar added a note Mar 11, 2014

Yes, we can remove this. There is a pull request already: #1834.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tschaub tschaub and 2 others commented on an outdated diff Mar 11, 2014
src/ol/mapbrowserevent.js
SINGLECLICK: 'singleclick',
- TOUCHSTART: goog.events.EventType.TOUCHSTART,
- TOUCHMOVE: goog.events.EventType.TOUCHMOVE,
- TOUCHEND: goog.events.EventType.TOUCHEND
+ DBLCLICK: goog.events.EventType.DBLCLICK,
+ POINTERDRAG: 'pointermove',
@tschaub
OpenLayers member
tschaub added a note Mar 11, 2014

Should this value be pointerdrag? As it is, you couldn't separately listen for drag and move events (without keeping track of down).

@ahocevar
OpenLayers member
ahocevar added a note Mar 11, 2014

Good catch. Looks like a typo to me. @tsauerwein, can you confirm? I did some testing after changing the value, and everything seemed right.

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 12, 2014

Yes, this was a typo. Thanks for noticing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tschaub tschaub commented on an outdated diff Mar 11, 2014
src/ol/pointer/touchsource.js
+ */
+ol.pointer.TouchSource.prototype.dedupSynthMouse_ = function(inEvent) {
+ var lts = this.mouseSource.lastTouches;
+ var t = inEvent.getBrowserEvent().changedTouches[0];
+ // only the primary finger will synth mouse events
+ if (this.isPrimaryTouch_(t)) {
+ // remember x/y of last touch
+ var lt = new goog.math.Coordinate(t.clientX, t.clientY);
+ lts.push(lt);
+ var fn = goog.bind(function(lts, lt) {
+ var i = lts.indexOf(lt);
+ if (i > -1) {
+ lts.splice(i, 1);
+ }
+ }, null, lts, lt);
+ goog.global.setTimeout(fn, this.DEDUP_TIMEOUT);
@tschaub
OpenLayers member
tschaub added a note Mar 11, 2014

Minor, but fn could be substituted for the anonymous function above. That is, the goog.bind call is unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tschaub
OpenLayers member
tschaub commented Mar 11, 2014

This looks like a lot of great work!

@fredj fredj commented on the diff Mar 12, 2014
src/ol/pointer/touchsource.js
+};
+
+
+/**
+ * Prevent synth mouse events from creating pointer events.
+ *
+ * @private
+ * @param {goog.events.BrowserEvent} inEvent
+ */
+ol.pointer.TouchSource.prototype.dedupSynthMouse_ = function(inEvent) {
+ var lts = this.mouseSource.lastTouches;
+ var t = inEvent.getBrowserEvent().changedTouches[0];
+ // only the primary finger will synth mouse events
+ if (this.isPrimaryTouch_(t)) {
+ // remember x/y of last touch
+ var lt = new goog.math.Coordinate(t.clientX, t.clientY);
@fredj
OpenLayers member
fredj added a note Mar 12, 2014

goog.math.Coordinate should not be necessary here, [t.clientX, t.clientY] is sufficient (not tested)

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 12, 2014

I used goog.math.Coordinate on purpose to make it more type safe, especially since the array lastTouches is used across two classes (TouchSource and MouseSource). But if you think it is not necessary, I will remove it.

@fredj
OpenLayers member
fredj added a note Mar 12, 2014

ok, I had not noticed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elemoine
OpenLayers member

Impressive work!

I just have a comment on the naming of interactions.

Up to now we've used names like DoubleClickZoom, where DoubleClick is the gesture and Zoom is the action performed on that gesture. And we still have interactions following that naming convention: DoubleClickZoom, DragRotate, etc.

So to continue using that convention I would suggest to rename Pan to DragPan, Zoom to PinchZoom, and Rotate to PinchRotate.

(I know Select, Draw, and Modify do not follow that convention, but they're also more complex/rich interaction.)

@fredj fredj and 1 other commented on an outdated diff Mar 12, 2014
src/ol/pointer/touchsource.js
+ */
+ol.pointer.TouchSource.prototype.isPrimaryTouch_ = function(inTouch) {
+ return this.firstTouchId_ === inTouch.identifier;
+};
+
+
+/**
+ * Set primary touch if there are no pointers, or the only pointer is the mouse.
+ * @param {Touch} inTouch
+ * @private
+ */
+ol.pointer.TouchSource.prototype.setPrimaryTouch_ = function(inTouch) {
+ if (this.pointerMap.getCount() === 0 ||
+ (this.pointerMap.getCount() === 1 && this.pointerMap.containsKey(1))) {
+ this.firstTouchId_ = inTouch.identifier;
+ this.firstXY = {X: inTouch.clientX, Y: inTouch.clientY};
@fredj
OpenLayers member
fredj added a note Mar 12, 2014

this.firstXY should be declared in the constructor (BTW the variable doesn't seems to be used)

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 12, 2014

You are right, firstXY is used nowhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fredj fredj commented on an outdated diff Mar 12, 2014
src/ol/pointer/touchsource.js
+ * @const
+ * @type {goog.structs.Map}
+ */
+ this.pointerMap = dispatcher.pointerMap;
+
+ /**
+ * @const
+ * @type {ol.pointer.MouseSource}
+ */
+ this.mouseSource = mouseSource;
+
+ /**
+ * @private
+ * @type {?number}
+ */
+ this.firstTouchId_ = null;
@fredj
OpenLayers member
fredj added a note Mar 12, 2014

Undefined values for basic types (string, number, ...) should be undefined instead of null (you can cherry-pick fredj@e1793f7)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fredj fredj commented on the diff Mar 12, 2014
src/ol/interaction/dragboxinteraction.js
/**
* @inheritDoc
*/
-ol.interaction.DragBox.prototype.handleDrag = function(mapBrowserEvent) {
+ol.interaction.DragBox.prototype.handlePointerDrag = function(mapBrowserEvent) {
+ if (!ol.events.condition.mouseOnly(mapBrowserEvent)) {
@fredj
OpenLayers member
fredj added a note Mar 12, 2014

I'm wondering if it's really necessary to deactivate the interaction for non mouse input

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 12, 2014

For example for the DragZoom interaction: You wouldn't press Shift and move one finger over the screen to zoom it. You would rather use a pinch zoom. That is why these interactions ignore events that do not come from a mouse device.

@fredj
OpenLayers member
fredj added a note Mar 13, 2014

I think this should be configurable and not hard coded in the lib (same for the other interactions).
this.condition_ can be:

  /**
   * @type {ol.events.ConditionType}
   */
  this.condition_ = goog.isDef(options.condition) ?
      options.condition : ol.events.condition.mouseOnly;
@fredj
OpenLayers member
fredj added a note Mar 13, 2014

Something like: fredj@5cd2159

@elemoine
OpenLayers member
elemoine added a note Mar 13, 2014

I don't think that the Drag* interactions work properly with touches (except DragPan obviously). So for now I'd leave the mouseOnly condition as non-configurable.

@fredj
OpenLayers member
fredj added a note Mar 13, 2014

I don't think you need to test in every handle* function; only in handlePointerDown (just like browserEvent.isMouseActionButton())

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 13, 2014

For example for DragZoom: Let's say Shiftis pressed and a mousedown occurs, this activates the interaction. Now, a touchstart occurs (which is not handled by handlePointerDown). But the following pointermove and pointerup events issued from this touch, would be handled by the interaction (without the check). So, the interaction might get deactivated before the mouseup happens.
Anyway, it doesn't work that you can zoom with the DragZoom while touching the screen. But with these checks I try to avoid that the interactions are left in a broken state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne and 1 other commented on an outdated diff Mar 12, 2014
src/ol/interaction/pointerinteraction.js
+ */
+ol.interaction.Pointer.prototype.handlePointerDown =
+ goog.functions.FALSE;
+
+
+/**
+ * @inheritDoc
+ */
+ol.interaction.Pointer.prototype.handleMapBrowserEvent =
+ function(mapBrowserEvent) {
+ if (!(mapBrowserEvent instanceof ol.MapBrowserPointerEvent)) {
+ return true;
+ }
+
+ var mapBrowserPointerEvent =
+ /** @type {ol.MapBrowserPointerEvent} */ (mapBrowserEvent);
@twpayne
twpayne added a note Mar 12, 2014

For info, you can use goog.asserts.assertInstanceOf here to avoid the typecast.

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 13, 2014

I thought I would need the typecast for the Closure compiler, but apparently I don't. I did not add an assertion, because I am doing an instanceof check in the line above (which is required to ensure that events other than pointer events are not handler by the PointerInteraction).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on an outdated diff Mar 12, 2014
src/ol/mapbrowserevent.js
@@ -9,9 +10,12 @@ goog.require('goog.events.BrowserEvent');
goog.require('goog.events.EventTarget');
goog.require('goog.events.EventType');
goog.require('goog.object');
+
@twpayne
twpayne added a note Mar 12, 2014

Stray blank line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on the diff Mar 12, 2014
src/ol/interaction/pointerinteraction.js
@@ -0,0 +1,177 @@
+
@twpayne
twpayne added a note Mar 12, 2014

Stray blank line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on the diff Mar 12, 2014
src/ol/mapbrowserevent.js
SINGLECLICK: 'singleclick',
- TOUCHSTART: goog.events.EventType.TOUCHSTART,
- TOUCHMOVE: goog.events.EventType.TOUCHMOVE,
- TOUCHEND: goog.events.EventType.TOUCHEND
+ DBLCLICK: goog.events.EventType.DBLCLICK,
+ POINTERDRAG: 'pointerdrag',
+
@twpayne
twpayne added a note Mar 12, 2014

Stray blank line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on the diff Mar 12, 2014
src/ol/pointer/eventsource.js
@@ -0,0 +1,56 @@
+
@twpayne
twpayne added a note Mar 12, 2014

Stray blank line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on the diff Mar 12, 2014
src/ol/pointer/eventsource.js
@@ -0,0 +1,56 @@
+
+goog.provide('ol.pointer.EventSource');
+
+goog.require('goog.events.BrowserEvent');
+goog.require('goog.object');
+
+
+
+/**
+ * @param {ol.pointer.PointerEventHandler} dispatcher
+ * @param {Object.<string, function(goog.events.BrowserEvent)>} mapping
+ * @constructor
@twpayne
twpayne added a note Mar 12, 2014

It would be good to add a @struct annotation to this class to catch any properties initialised outside the constructor.

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 13, 2014

I can not add the @struct annotation, because PointerEvent is inheriting from goog.events.Event, which is not a struct.

@twpayne
twpayne added a note Mar 13, 2014

OK, thanks for trying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne and 2 others commented on an outdated diff Mar 12, 2014
src/ol/pointer/mousesource.js
+ * @constructor
+ * @extends {ol.pointer.EventSource}
+ */
+ol.pointer.MouseSource = function(dispatcher) {
+ var mapping = {
+ 'mousedown': this.mousedown,
+ 'mousemove': this.mousemove,
+ 'mouseup': this.mouseup,
+ 'mouseover': this.mouseover,
+ 'mouseout': this.mouseout
+ };
+ goog.base(this, dispatcher, mapping);
+
+ /**
+ * @const
+ * @type {goog.structs.Map}
@twpayne
twpayne added a note Mar 12, 2014

Does this have to be a goog.structs.Map? Is it possible to use an Object here instead? Note that it is not possible to use an Object if the keys to the map could include strings like hasOwnProperty.

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 13, 2014

The keys for this map are pointer ids, so I could use a plain object. But for me it feels more natural to call map.getCount() or map.containsKey(key) instead of goog.object.getCount(obj) or goog.object.containsKey(obj, key).

@elemoine
OpenLayers member
elemoine added a note Mar 13, 2014

FWIW using static functions rather than methods may avoid object property lookup operations at runtime. The compiler may rename goog.object.getCount to a non-namespaced function.

@twpayne
twpayne added a note Mar 13, 2014

Note that goog.structs.Map is both much slower and brings in more code that using a plain old JavaScript object. It may be that goog.structs.Map is already included in the build (I haven't checked) but unless you need to store arbitrary keys then it's better to use an Object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on the diff Mar 12, 2014
src/ol/pointer/pointerevent.js
+/**
+ * Checks if the `MouseEvent` type is supported.
+ */
+(function() {
+ try {
+ var ev = ol.pointer.PointerEvent.createMouseEvent('click', {buttons: 1});
+ ol.pointer.PointerEvent.NEW_MOUSE_EVENT = true;
+ ol.pointer.PointerEvent.HAS_BUTTONS = ev.buttons === 1;
+ } catch (e) {
+ }
+})();
+
+
+/**
+ * Warning is suppressed because Closure thinks the MouseEvent
+ * constructor takes no arguments.
@twpayne
twpayne added a note Mar 12, 2014

Does the MouseEvent constructor really take two arguments?

My reading of the DOM level 2 spec that MouseEvent is an interface, not a constructor, and that instances should be created with createEvent, but I might have misunderstood.

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 13, 2014

The MouseEvent constructor was introduced in DOM level 3 and this check tests if the buttons property is supported (since DOM level 4).

@fredj
OpenLayers member
fredj added a note Mar 13, 2014

This is a bug in closure compiler externs, fixed with https://code.google.com/p/closure-compiler/source/detail?r=654c073d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on an outdated diff Mar 12, 2014
src/ol/pointer/pointereventhandler.js
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+goog.provide('ol.pointer.PointerEventHandler');
+
+
@twpayne
twpayne added a note Mar 12, 2014

Stray blank line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on an outdated diff Mar 12, 2014
src/ol/pointer/pointereventhandler.js
+ * @extends {goog.events.EventTarget}
+ * @param {Element|HTMLDocument} element Viewport element.
+ */
+ol.pointer.PointerEventHandler = function(element) {
+ goog.base(this);
+
+ /**
+ * @const
+ * @private
+ * @type {Element|HTMLDocument}
+ */
+ this.element_ = element;
+
+ /**
+ * @const
+ * @type {goog.structs.Map}
@twpayne
twpayne added a note Mar 12, 2014

Same comment about use of goog.structs.Map vs. Object as before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on an outdated diff Mar 12, 2014
src/ol/pointer/pointereventhandler.js
+ if (handler) {
+ this.eventMap_[e] = goog.bind(handler, s);
+ }
+ }, this);
+ this.eventSourceList_.push(s);
+ }
+};
+
+
+/**
+ * Set up the events for all registered event sources.
+ * @private
+ */
+ol.pointer.PointerEventHandler.prototype.register_ = function() {
+ var l = this.eventSourceList_.length;
+ for (var i = 0, es; (i < l) && (es = this.eventSourceList_[i]); i++) {
@twpayne
twpayne added a note Mar 12, 2014

Is there a good reason to use an assignment inside an expression here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on an outdated diff Mar 12, 2014
src/ol/pointer/pointereventhandler.js
+ */
+ol.pointer.PointerEventHandler.prototype.register_ = function() {
+ var l = this.eventSourceList_.length;
+ for (var i = 0, es; (i < l) && (es = this.eventSourceList_[i]); i++) {
+ this.addEvents_(es.getEvents());
+ }
+};
+
+
+/**
+ * Remove all registered events.
+ * @private
+ */
+ol.pointer.PointerEventHandler.prototype.unregister_ = function() {
+ var l = this.eventSourceList_.length;
+ for (var i = 0, es; (i < l) && (es = this.eventSourceList_[i]); i++) {
@twpayne
twpayne added a note Mar 12, 2014

Same comment re assignments inside expressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on an outdated diff Mar 12, 2014
src/ol/pointer/pointereventhandler.js
+
+ return eventCopy;
+};
+
+
+// EVENTS
+
+
+/**
+ * Triggers a 'pointerdown' event.
+ * @param {Object} pointerEventData
+ * @param {goog.events.BrowserEvent } browserEvent
+ */
+ol.pointer.PointerEventHandler.prototype.down =
+ function(pointerEventData, browserEvent) {
+ this.fireEvent('pointerdown', pointerEventData, browserEvent);
@twpayne
twpayne added a note Mar 12, 2014

Is it possible to use an @enum for the event types (pointerdown, pointermove, etc.). Use of @enums can help catch typos.

@twpayne
twpayne added a note Mar 12, 2014

ol.pointer.EventType could be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on an outdated diff Mar 12, 2014
src/ol/pointer/pointereventhandler.js
+ * Constants for event names.
+ * @enum {string}
+ */
+ol.pointer.EventType = {
+ POINTERMOVE: 'pointermove',
+ POINTERDOWN: 'pointerdown',
+ POINTERUP: 'pointerup',
+ POINTEROVER: 'pointerover',
+ POINTERENTER: 'pointerenter',
+ POINTERLEAVE: 'pointerleave',
+ POINTERCANCEL: 'pointercancel'
+};
+
+
+/**
+ * List of properties to copy when cloning an event.
@twpayne
twpayne added a note Mar 12, 2014

I think it's clearer and less error prone to merge ol.pointer.CLONE_PROPS and ol.pointer.CLONE_DEFAULTS into a single array. See twpayne@858d268.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on an outdated diff Mar 12, 2014
src/ol/pointer/touchsource.js
+ this.firstTouchId_ = undefined;
+ this.resetClickCount_();
+ }
+};
+
+
+/**
+ * @private
+ */
+ol.pointer.TouchSource.prototype.resetClickCount_ = function() {
+ var fn = function() {
+ this.clickCount_ = 0;
+ this.resetId_ = undefined;
+ };
+ this.resetId_ = goog.global.setTimeout(goog.bind(fn, this),
+ ol.pointer.TouchSource.CLICK_COUNT_TIMEOUT);
@twpayne
twpayne added a note Mar 12, 2014

fn and goog.bind(fn, this) can be created once in the constructor, stored in a private member variable, and re-used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on the diff Mar 12, 2014
src/ol/pointer/touchsource.js
+/**
+ * @private
+ * @param {Object} inPointer
+ */
+ol.pointer.TouchSource.prototype.removePrimaryPointer_ = function(inPointer) {
+ if (inPointer.isPrimary) {
+ this.firstTouchId_ = undefined;
+ this.resetClickCount_();
+ }
+};
+
+
+/**
+ * @private
+ */
+ol.pointer.TouchSource.prototype.resetClickCount_ = function() {
@twpayne
twpayne added a note Mar 12, 2014

You should probably add an

goog.asserts.assert(!goog.isDef(this.resetId_));

here

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 13, 2014

To make sure that an previous reset id is not overridden, agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on an outdated diff Mar 12, 2014
src/ol/pointer/touchsource.js
+ p.preventDefault = function() {
+ inEvent.preventDefault();
+ };
+ }, this);
+ goog.array.forEach(pointers, goog.partial(inFunction, inEvent), this);
+};
+
+
+/**
+ * @private
+ * @param {TouchList} touchList
+ * @param {number} searchId
+ * @return {boolean} True, if the `Touch` with the given id is in the list.
+ */
+ol.pointer.TouchSource.prototype.findTouch_ = function(touchList, searchId) {
+ for (var i = 0, l = touchList.length, t; i < l && (t = touchList[i]); i++) {
@twpayne
twpayne added a note Mar 12, 2014

Assignment inside an expression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne twpayne commented on an outdated diff Mar 12, 2014
src/ol/pointer/touchsource.js
+ */
+ol.pointer.TouchSource.prototype.dedupSynthMouse_ = function(inEvent) {
+ var lts = this.mouseSource.lastTouches;
+ var t = inEvent.getBrowserEvent().changedTouches[0];
+ // only the primary finger will synth mouse events
+ if (this.isPrimaryTouch_(t)) {
+ // remember x/y of last touch
+ var lt = new goog.math.Coordinate(t.clientX, t.clientY);
+ lts.push(lt);
+
+ goog.global.setTimeout(function() {
+ // remove touch after timeout
+ var i = lts.indexOf(lt);
+ if (i > -1) {
+ lts.splice(i, 1);
+ }
@twpayne
twpayne added a note Mar 12, 2014

You can use goog.array.remove here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twpayne
twpayne commented Mar 12, 2014

This is fantastic work @tsauerwein! I've added a few comments, but am very much looking forward to this being merged.

@twpayne twpayne referenced this pull request Mar 12, 2014
Merged

Remove leading whitespace #1853

@elemoine elemoine and 1 other commented on an outdated diff Mar 13, 2014
examples/google-map.js
@@ -3,7 +3,7 @@
goog.require('ol.Map');
goog.require('ol.View2D');
goog.require('ol.interaction');
-goog.require('ol.interaction.DragPan');
+goog.require('ol.interaction.Pan');
@elemoine
OpenLayers member
elemoine added a note Mar 13, 2014

Isn't the interaction DragPan rather than Pan now?

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 13, 2014

It is. Too bad these kind of mistakes are not detected by build.py check.

@elemoine
OpenLayers member
elemoine added a note Mar 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elemoine elemoine commented on an outdated diff Mar 13, 2014
src/objectliterals.jsdoc
@@ -468,7 +460,7 @@
*/
/**
- * @typedef {Object} olx.interaction.TouchPanOptions
+ * @typedef {Object} olx.interaction.DragPanOptions
@elemoine
OpenLayers member
elemoine added a note Mar 13, 2014

Minor, but the type definitions in this file are ordered alphabetically. This means that DragPanOptions should come before DragRotateOptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elemoine elemoine commented on the diff Mar 13, 2014
src/ol/events/condition.js
@@ -120,3 +121,17 @@ ol.events.condition.targetNotEditable = function(mapBrowserEvent) {
tagName !== goog.dom.TagName.SELECT &&
tagName !== goog.dom.TagName.TEXTAREA);
};
+
+
+/**
+ * @param {ol.MapBrowserEvent} mapBrowserEvent Map browser event.
+ * @return {boolean} True if the event originates from a mouse device.
+ * @todo stability experimental
+ */
+ol.events.condition.mouseOnly = function(mapBrowserEvent) {
+ goog.asserts.assertInstanceof(mapBrowserEvent, ol.MapBrowserPointerEvent);
@elemoine
OpenLayers member
elemoine added a note Mar 13, 2014

It may be better that mouseOnly takes an ol.MapBrowserPointerEvent rather than a ol.MapBrowserEvent.

@fredj
OpenLayers member
fredj added a note Mar 13, 2014

If ol.MapBrowserPointerEvent is used, the function will not be a ol.MapBrowserPointerEvent anymore

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 13, 2014

... not be a ol.events.ConditionType anymore, yes.

@elemoine
OpenLayers member
elemoine added a note Mar 13, 2014

I see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elemoine
OpenLayers member

"pointerdown" (non-prefixed) should be added to this list https://github.com/tsauerwein/ol3/blob/pointerevents/src/ol/map.js#L259-264.

@elemoine elemoine and 1 other commented on an outdated diff Mar 13, 2014
src/ol/mapbrowserevent.js
+ * which is required when the pointer is moved outside
+ * the viewport when dragging.
+ */
+ this.documentPointerEventHandler_ =
+ new ol.pointer.PointerEventHandler(document);
+
+ this.dragListenerKeys_ = [
+ goog.events.listen(this.documentPointerEventHandler_,
+ ol.MapBrowserEvent.EventType.POINTERMOVE,
+ this.handlePointerMove_, false, this),
+ goog.events.listen(this.documentPointerEventHandler_,
+ ol.MapBrowserEvent.EventType.POINTERUP,
+ this.handlePointerUp_, false, this),
+ goog.events.listen(this.pointerEventHandler_,
+ ol.MapBrowserEvent.EventType.POINTERCANCEL,
+ this.handlePointerUp_, false, this)
@elemoine
OpenLayers member
elemoine added a note Mar 13, 2014

Any reason for treating pointercancel events as pointerup events. This also means that interactions will get a pointerup event when a pointercancel event actually occurred.

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 13, 2014

On touch devices, if a finger leaves the screen outside the viewport element, you might not receive a pointerup but a pointercancel instead. That is why I also listen to pointercancel to properly stop the dragging.

@elemoine
OpenLayers member
elemoine added a note Mar 13, 2014

The conversion from pointercancel to pointerup bothers me, but we can address this with a separate PR I guess.

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 13, 2014

I understand your concern. This pointerup is more like dragend. Especially since pointerup events are only dispatched if a pointerdown happened before.

@elemoine
OpenLayers member
elemoine added a note Mar 13, 2014

I am realizing that the drag listeners are on the document rather than the viewport, which makes sense. pointercancel is the exception though. Is this normal? Shouldn't it be this.documentPointerEventHandler_ as well on line 360?

@tsauerwein
OpenLayers member
tsauerwein added a note Mar 14, 2014

I wasn't aware of it, but I think it still makes sense. TouchSource.vacuumTouches_() issues pointercancel events, when there was no touchend for a touchstart.
Now, let's say a first touchstart is registered on pointerEventHandler_. The documentPointerEventHandler_ is set up. But documentPointerEventHandler_ doesn't know about the first touchstart. If there is no touchend for the touchstart, we can only receive a touchcancel from pointerEventHandler_, because it is only registered there.

I hope this makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tsauerwein and others added some commits Feb 3, 2014
@tsauerwein tsauerwein pointer events generated by mouse events 7140f60
@tsauerwein tsauerwein source for ms pointer events (IE10) df4bd7c
@tsauerwein tsauerwein source for native pointer events (IE11) 445ff5e
@tsauerwein tsauerwein source for touch events 0858f68
@tsauerwein tsauerwein add polymer license info 4925496
@tsauerwein tsauerwein use goog.bind, safari does not support the native bind 37ab50e
@tsauerwein tsauerwein base interactions for pointer events on existing touch interactions 014ef96
@tsauerwein tsauerwein former touch interactions now use pointer events 1c75ecc
@tsauerwein tsauerwein copy properties for pointer event from internal browser event, not fr…
…om google.event.BrowserEvent
ee21747
@tsauerwein tsauerwein allow dragging for mouse/ms/native when pointer is outside map viewpo…
…rt element
c4c1eb7
@tsauerwein tsauerwein only track events when "pointerdown" event was received, because on n…
…ative android browser wrong move events are sent with no "pointerdown" event
a31cc3f
@tsauerwein tsauerwein removes not needed WeakMap struct e1f4410
@tsauerwein tsauerwein add comment about an open bug with native Android browser (4.1.2) whe…
…n the page is scrolled
7c8713f
@tsauerwein tsauerwein makes `PointerEvent` a plain closure class, instead of trying to mimi…
…c a mouse event, which was required for the polyfill
d772c95
@tsauerwein tsauerwein refactoring, type annotations 7fbd111
@tsauerwein tsauerwein when copying properties for touch events, make sure that those proper…
…ties that might be different for each `Touch` object are not copied from the touch event
2e4badb
@tsauerwein tsauerwein use separate pointer event handler for document when dragging b52f793
@tsauerwein tsauerwein move checks for pointer support into `browserfeature` module eca6846
@tsauerwein tsauerwein introduce type POINTERDRAG to make a distinction between pointer move…
… and drag
692a686
@tsauerwein tsauerwein add assertion e0b345b
@tsauerwein tsauerwein fixes tests for mapbrowserevent 68b7ee5
@tsauerwein tsauerwein tests for pointereventhandler 2846996
@tsauerwein tsauerwein tests for mouse- and touchsource 3d7ae92
@tsauerwein tsauerwein PointerEventHandler test 122dac8
@tsauerwein tsauerwein fixes `preventDefault()` on IE11 bfc9545
@tsauerwein tsauerwein base interaction for zoom box on pointer events a696150
@tsauerwein tsauerwein PointerInteraction uses MapBrowserPointerEvent not MapBrowserEvent fca9c50
@tsauerwein tsauerwein refactoring pointer interactions 411b725
@tsauerwein tsauerwein pointer interaction might also receive events that are not pointer ev…
…ents
12b0fee
@tsauerwein tsauerwein base dragrotateinteraction on pointer interaction c593add
@tsauerwein tsauerwein base dragrotateandzoominteraction on pointer interaction 56dcdd0
@tsauerwein tsauerwein removes now superfluous DragPanInteraction dc164fd
@tsauerwein tsauerwein fixes interaction options in tests 43aac54
@tsauerwein tsauerwein removes dead code bc31d42
@tsauerwein tsauerwein use anonymous function fa434be
@ahocevar ahocevar Rename ol.interaction.PointerInteraction 05dd760
@ahocevar ahocevar Updating google-map example to use pointer events 296da6f
@ahocevar ahocevar Relay pointermove events again
This will be needed for hover actions.
32ffb1e
@ahocevar ahocevar The Modify interaction now uses pointer events
While dragging a vertex, the feature on the original layer is
not updated until the first pointer move after dragging. See
#1796. Previously, the Modify interaction did not set the
interacting hint on the view, so the feature was also updated
on the original layer. But now, the interacting hint is set,
which exposes this behaviour.
f6efcbc
@ahocevar ahocevar Do not render selected features on the original layer
See #1796 for a discussion on how to handle this in a better
way.
f663a3d
@ahocevar ahocevar Adding basic support for non-mouse devices be1318f
@ahocevar ahocevar Making the draw interaction work with pointer events
This is not yet optimized for mobile, and tests need to be
updated.
d688003
@ahocevar ahocevar Adding basic support for mobile
Usability still needs improvement.
bc1489e
@ahocevar ahocevar Updating tests for Draw interaction f5f2dae
@ahocevar ahocevar Fixing API docs 18ba2f8
@ahocevar ahocevar Note about MultiGeometry rendering 29454c2
@ahocevar ahocevar Adding missing require 5b25826
@tsauerwein tsauerwein Adding missing default returns 50d09eb
@tsauerwein tsauerwein Remove DragInteraction 5f75104
@tsauerwein tsauerwein Remove old event types
Pointer events for the win.
0d72f45
@tsauerwein tsauerwein Add comment about mouse support only 79424ec
@tsauerwein tsauerwein Use goog.dispose 42f0dfa
@tsauerwein tsauerwein Adding missing return d6ca93e
@tsauerwein tsauerwein Fix support for legacy IE b17957b
@tsauerwein tsauerwein Make goog.events.listen return proper listener key
When goog.events.listen receives an array of event types,
it only return null as listener key. So that calling
goog.events.unlistenByKey does not work.
51d7cb2
@fredj fredj Add extern for Navigator.prototype.pointerEnabled cbb875e
@fredj fredj Use goog.object.get instead of ol.pointer.PointerEvent#getValueOr_ 6b4b62d
@fredj fredj Remove ol.pointer.*Source#events properties a74ad0e
@fredj fredj Add ol.pointer.TouchSource.POINTER_TYPE constant a916e25
@fredj fredj Set ol.pointer.*Source#mapping_ properties to private b2e9467
@fredj fredj Simplify pointerType string conversion a13ade9
@fredj fredj Pass the mapping object to the ol.pointer.EventSource constructor
Conflicts:
	src/ol/pointer/mssource.js
028a183
@tsauerwein tsauerwein Work on a copy of `changedTouches`
On Opera Mobile 11.10 it is not possible to
directly work on `changedTouches` (e.g. map
fails). That is why a copy is made first.
6e2f0dc
@fredj fredj Use goog.object.get instead of ol.pointer.PointerEvent#getValue_ 08b297e
@fredj fredj Change ol.BrowserFeature.HAS_POINTER feature detection
Because navigator.pointerEnabled will not be part of the final spec.
See https://www.w3.org/Bugs/Public/show_bug.cgi?id=22890
aee76c9
@tsauerwein tsauerwein Read pointerId/pointerType from browser event
and not from the Google Closure event.
4468d96
@tsauerwein tsauerwein Fix type annotation 6742e6a
@ahocevar ahocevar Removing code for functionality now handled by #1834 e28faa3
@tsauerwein tsauerwein Fix typo in POINTERDRAG constant 5283953
@tsauerwein tsauerwein Remove unnecessary call to goog.bind fcb52d2
@fredj fredj Move DEDUP_TIMEOUT and CLICK_COUNT_TIMEOUT constants out of the instance f5b15f7
@tsauerwein tsauerwein Remove not used property TouchSource.firstXY 18c2888
@fredj fredj Use 'undefined' for undefined numbers
Conflicts:
	src/ol/pointer/touchsource.js
ee80238
@tsauerwein tsauerwein Rename interactions
The idea behind the naming scheme is that for example
for `DragRotate`, `Rotate` is the action and `Drag` the
gesture that issues the action.
5a35891
@twpayne twpayne Merge ol.pointer.CLONE_PROPS and CLONE_DEFAULTS into a single array df71854
@elemoine elemoine Use a more explicit variable name b939ca2
@elemoine elemoine Change a property from private to protected 35b3173
@elemoine elemoine Remove view hint handling from pointer interaction d1ef5b0
@elemoine elemoine DragPan interaction sets/unsets interacting hint b533955
@elemoine elemoine PinchRotate interaction sets/unsets interacting hint 544ceaf
@elemoine elemoine PinchZoom interaction sets/unsets interacting hint d08ab1e
@tsauerwein tsauerwein Remove leading whitespace 6933023
@tsauerwein tsauerwein Remove not needed type cast 873d47d
@tsauerwein tsauerwein Remove blank lines 77aa29e
@tsauerwein tsauerwein Remove unused constant NEW_MOUSE_EVENT 73c33e5
@tsauerwein tsauerwein Remove assignments inside expression b693ddf
@tsauerwein tsauerwein Remove unused property pointerMap on NativeSource 72d7a6e
@tsauerwein tsauerwein Use enums for pointer event types e08fa37
@tsauerwein tsauerwein Re-use handler d170281
@tsauerwein tsauerwein Assert that timeout reset ids are not overridden 1a477b8
@tsauerwein tsauerwein Add missing require 71b424f
@tsauerwein tsauerwein Remove assignment inside expression debdc26
@tsauerwein tsauerwein Make use of goog.array.remove 68fa5fb
@tsauerwein tsauerwein Set preventDefault in PointerEvent constructor 8a26ac6
@tsauerwein tsauerwein Replace goog.structs.Map with plain object 51a557e
@tsauerwein tsauerwein Fix Google Maps example 7789d5e
@tsauerwein tsauerwein Fix order of olx.interaction.* types 89da5de
@tsauerwein tsauerwein Listen also to pointerdown on overlay container 599de6a
@tsauerwein tsauerwein Remove assertion again
When doing a double tap, the timeout id might get overridden.
41df4e0
@tsauerwein tsauerwein Remove not needed require b2a37b8
@tsauerwein tsauerwein Add comment about pointercancel listener 155b9b3
@tsauerwein
OpenLayers member

Thanks a lot for all the comments! From my side the pull request is now ready to merge.

@elemoine
OpenLayers member

So merging... :-) Thanks again everyone.

@elemoine elemoine merged commit c8f981b into openlayers:master Mar 14, 2014

1 check passed

Details default The Travis CI build passed
@tschaub

Looks like a missing clock.restore() here (see #2063).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.