make touch event handling available in core modestmaps.js

[RandomEtc]

Rather than in touch example.

Perhaps shouldn't depend on webkit transform though - do other browsers support touch events now?

[tmcw]

So the main objective here would be to add Android support before it becomes part of core? I've also found a small bug i which some two-touch events are not ended property - you'll do a pinch-zoom, lift up, and the next touch registers as part of the last, and continues zoom behavior?

[RandomEtc]

Android would be good enough ,yes, but any sufficiently modern mobile browser with multi-touch events should eventually work.

I'd love to support recent Nokia tablets, HPalm WebOSes, whatever Blackberry has and perhaps Windows Phone 7 too? Not that I know anyone with any of those devices, so don't hold your breath :)

Is that bug you're seeing on iOS? I have an ipad and an ipod to test with here, I'll see if I can find it.

[tmcw]

Okay - I've hacked around on the touch example, cleaning it up and adding Android support. It was pretty close - only some small tweaks to sameTouch, etc, were required. So, currently, the example works well on my testing iPad & Galaxy Tab.

The main caveat is that Android doesn't support multitouch in-browser at all (even in 3.x, I believe). So, you can zoom into the map with the android example, but cannot zoom out. I'm going to work on basically a mobile-able system in wax, but since there aren't controls in modestmaps, it'll probably need a note.

[RandomEtc]

Gmaps on iOS uses two-finger tap to zoom out. How about implementing that for mono-touch browsers?

[tmcw]

Had never noticed that before on iOS - useful but non-obvious kind thing. I don't think this is possible on mono-touch browsers, though, there's no differentiation between a single-tap and a two-finger tap in Android's WebKit, as far as I can see, just as there's no differentiation with multi-touch events. Checked out the Google Maps webapp on Android and no dice - if it were possible, I'd imagine they'd be the first to implement.

[tmcw]

Thoughts? I can pretty much confirm that two-finger tap is a no-go for Android. The only other possibilities... I don't know, triple-tap?

[mourner]

@tmcw, khtmlib (afair) uses an interesting approach - the map starts to zoom out if you touch the map (without movement) for more than several seconds, but it's not very intuitive... In Leaflet, this problem is somewhat solved by having zoom in/out buttons on the map by default, but it would be great to know if there are any other solutions.

[tmcw]

There's also the possibility of a triple-tap or a single-tap for zoom-out, but none of these are very predictable.

[tmcw]

Okay, I think touch stuff is ready to go as a general feature, but there's unfortunately more to do, that I hadn't thought about: what the touchhandler does should actually be closer to the map, and should spawn drawn events so that things like the multi-layer hack should work. There's also the question of whether it should actually transform the map parent itself, since controls often live within it. Ideally pulling out some of the webkit transforms from the touch stuff can get them closer to working in general browsers, too.

[RandomEtc]

I just reviewed all the current examples and updated everything except the touch example. It should be updated to use the new core touch handler and make sure the issues you're talking about are resolved. Let's keep chatting here to figure out the best way, whenever anyone is next ready to take a look at it.

[tmcw]

@RandomEtc @migurski: I've pretty much finished up with a big rewrite of the touch functionality in MM that moves CSS transformations into a core util function and those does tile loading, drawn events, etc., on mobile during panning. There are also a bunch of other optimizations in the code - using requestAnimationFrame, working on toKey, using CSS transforms in supported desktop browsers as well, etc.

I've been testing on iPad, Chrome, Firefox, Safari, and IE8 and it's been stable - I'm at the point where the refactoring is at a decent point and I think it's ready to merge. If someone could give it a kick in the tires and a review it's be super-appreciated.

[mourner]

Great work Tom! Looking forward to see this merged into master.

[RandomEtc]

@tmcw sorry for the lack of noise from me on this front, I'm just not able to spend much time on MMJS at this point. You're doing great work, I hope to get a chance to check it out soon.

From a cursory glance at the various commits my main hangup is to urge you to watch out for seams at in between zoom levels (e.g. the anyscale example should throw up possible issues). It should be possible to avoid them entirely with minimal cost in terms of map precision, but it's hard for me to track whether your changes to the various rounds, floors and ceils will have introduced issues, especially when combined with CSS transforms. I'll check for this as soon as I can :)

I saw the addition of a bind utility, which seems smart, but watch out for unnecessary re-creation of big closures: the purpose of those getXXXHandler functions was mainly to only close once. I've noticed that @mbostock has a different pattern for that in D3, using top level functions for the bulk of functionality and smaller closures that call out to them. The hope is it makes those functions easier for the JS engine to optimize, I think.

IE6+ was my target for the initial version, I'd be sad to see that dropped but CSS transforms etc. are very attractive so it might be time. ... I would love to get the zoompan demo running at +30fps, which seems totally doable, especially with modern JS.

Look forward to the full CHANGELOG... will this be version 1.0.0? :)

[tmcw]

Re: seams - I'm thinking that this is actually pretty core to doing transforms on individual tiles - I'm seeing few examples that both do transforms per-tile (which makes sense to me) but don't have seams. The workaround, I believe, is doing some interactions on a zoomLayer level and others on a tile level, though this needs more research.

If this is broken in IE, then it's a bug - the abstraction of moving elements to moveElement should let us use CSS transforms and inline standard CSS interchangeably. Admittedly I haven't done enough testing recently, but I also see no reason to drop IE support anytime soon.

And I totally agree with the bostocking suggestion - I've recently rewritten Wax in the style and it's awesome - compact and pretty clear. I guess the transforms-refactor branch was becoming the everything-changes branch and I didn't want to take that to the extreme :). But, definitely.

The branch is still moving a bit - deploying it on some bleeding-edge sites and tracking down bugs pretty quickly that way. Just today I realized a big gap in my understanding of matrix3d and thus the touch interactions are now way less wonky. There's still much more to do there.

[tmcw]

Just tested IE7+8 (I don't have an IE6 install handy, but I'll try to come up with one), and they're both working well with the new branch - moveElement correctly uses positioning when transforms aren't available.

I've also started up a branch called less-new, which is trying to get rid of mm.bind in handlers, and then see where else it can be stripped out. Handlers, I think, should be the first to make that leap, though it'll break that API (no more new). For other parts of the library, it's probably more of a decision between this style and passing around literals - which would work for coordinates, points, and locations, I believe.

[RandomEtc]

Which API will it break? Internal or external? Can you give a before/after example?

[tmcw]

Just to be clear, the transforms-refactor branch doesn't break any APIs, internal or external, AFAIK. However, the less-new branch will break the initialization of handlers.

map = new com.modestmaps.Map('map',
  provider,
  null, [
    new com.modestmaps.TouchHandler()
  ]);

becomes

map = new com.modestmaps.Map('map',
  provider,
  null, [
    com.modestmaps.touchHandler()
  ]);

[RandomEtc]

I think this should be either new com.modestmaps.TouchHandler() or com.modestmaps.createTouchHandler() - the verb and lowercase both signalling that it's a function that returns a handler, and not an instance of a 'class' function. I'd prefer the first to remain consistent but recognise that the second is more or less the prevailing javascript idiom these days, especially for one-off objects (as discussed when we considered moving away from prototypes in general).

[tmcw]

I'm unsure about that - TouchHandler() strikes me as incorrect, because it's not a proper Javascript function/prototype/class, and I don't know if the second is really any prevailing standard - are there any examples besides polymaps and d3 that can guide us in a naming decision?

Regardless, I'll open up another ticket for that - this issue is about touch handling in core (represented by the transforms-refactor branch), rather than the less-new work.

[RandomEtc]

I'll have a proper a look at the less-new branch as soon as I can, I haven't been following that work. But I thought we'd discussed abandoning prototypes so I'm a little uneasy about mixing styles here, but that's just my opinion. We're bike-shedding about naming style and that's my fault. Apologies.

That siad, I'm worried you missed the new on suggestion 1. Captial letter class-like things and new should match up; I think we're agreeing there but I just want to be sure. And for 2 I was suggesting (just a suggestion not a requirement, we have no formal style guide) that if it's a function I'd prefer it with a verb in. I'd go further and reiterate that I'd like to avoid d3/polymaps as a style guide for naming, as I've stated before, because I miss verbs when using those libraries.

...

Anyway... as you say this discussion isn't needed yet. Looks like you have new functionality ready (touchHandler or TouchHandler) that isn't currently in core (just half-baked samples), so technically there's no API breakage and you can call it what you like... And regardless, that's what the CHANGELOG and version numbers are for. When it's in master we can worry about backwards compatibility :)

[tmcw]

I've moved that discussion to #47. My main priority is really just to get this touch work in - I've been using it in production for a bit already and it contains enough broad refactoring that developing another branch (like less-new) that's not based off of it and expected to merge cleanly would be difficult.

[RandomEtc]

Bringing this issue back on topic then... I'm assuming from the above discussion that transforms-refactor is the branch you're hoping to land soon, and that it addresses this issue #8, and that you would like some feedback... :)

Using github's nifty comparison tool, https://github.com/stamen/modestmaps-js/compare/master...transforms-refactor and reading the Files Changed tab, everything basically looks good to me but with one or two edge cases still to fix. JSHint is a tough mistress but aside from formatting the substantive changes are well encapsulated, so it should be easy...

If I understand correctly, the main change is to rework tile positioning to use CSS transforms where available, and that's done with a new MM.moveElement function. Reworking the tile positioning and transforms means that the Map state is correct while panning/zooming, where previously the demo touch handler example broke the Map state while panning and this made synchronizing overlays difficult?

Assuming I've got that right, I see a couple of issues with the use of moveElement. I'm testing using the anyscale example, which is where most of the tile positioning/scaling logic was ironed out in previous versions. In Safari, I see seams when zooming, and also shakiness which could be precision errors depending on how scaling is being handled. In IE6 (yes, I know) which doesn't have any transforms available, tiles aren't scaled correctly at in between zoom levels. moveElement needs to apply the scale property of the supplied point, even if matrix transforms aren't being used. To give the same behavior as the current master, you want something like el.style.width = Math.ceil(point.width * point.scale) + 'px'; but you might want to move the ceil to creation of point to keep moveElement generic.

Also, it looks like you've got a nice general purpose routine for finding CSS transform properties but you're only applying them for MM._browser.webkit3d browsers. It would be good to use transforms wherever they're available, I'm guessing.

Apart from moveElement, I see the cleanups related to function binding and a move to use requestAnimationFrame wherever it's available:

• For requestFrame, I don't fully understand whether deferring draw where it was previously called synchronously will have undue side-effects, but since most overlays will be pegged to a 'drawn' callback, and centerCoordinate continues to be updated, I'm pretty sure it will be All Good.
• For bind, though I <3 Underscore I would have been more conservative than that and gone with a dumber implementation that only replaced the getXXX handler pattern (which I'm happy to see the back of, BTW). My conservatism isn't entirely unjustified if you read up on JS engine internals: https://plus.google.com/111090511249453178320/posts/ikjTyY6UKcE (fun stuff). I wouldn't hold back this branch just for that, but let's have a look in a profiler sometime and see if bound handlers are busy when interacting.

Comparing the other way: https://github.com/stamen/modestmaps-js/compare/transforms-refactor...master it looks like the only new things on master since the branch are the updated examples. Probably best to bring these into tranforms-refactor (or another merge branch) and run through them all in various browsers before finally pulling the trigger.

[RandomEtc]

TL;DR:

It looks like moveElement needs to apply the scale property of the supplied point if matrix transforms aren't being used.

Also there are seams visible in the anyscale branch related to the loss of the ceil for tile scales that was removed when moving to moveEvent. :)

I would consider both of these as blocking bugs on the transforms-refactor branch. Let me know if you need help testing/fixing them.

[tmcw]

Thanks! Should be able to get these bugs sorted out pretty quickly.

If I understand correctly, the main change is to rework tile positioning to use CSS transforms where available, and that's done with a new MM.moveElement function. Reworking the tile positioning and transforms means that the Map state is correct while panning/zooming, where previously the demo touch handler example broke the Map state while panning and this made synchronizing overlays difficult?

Yep - there are a few benefits:

• Transforms will be used in desktop browsers as well, hopefully upping speed
• Map state is updated during pinches
• Tiles can (optionally) continue to load during panning on mobile browsers
• The touch handler goes back to being a generalized handler instead of containing transforms logic
• Stuff you attach to the map.parent isn't transformed along with the map itself.

Also, it looks like you've got a nice general purpose routine for finding CSS transform properties but you're only applying them for MM._browser.webkit3d browsers. It would be good to use transforms wherever they're available, I'm guessing.

Addressed in 6ea7bf16165f50304157927cbcd02e655df6dea4

To give the same behavior as the current master, you want something like el.style.width = Math.ceil(point.width * point.scale) + 'px'; but you might want to move the ceil to creation of point to keep moveElement generic.

Addressed in 011353c88a83ab3346aba372722a759d9a6e52bf . I think that the ceil should stay in moveElement for now - since it's more correcting for browser pecularities than doing anything else.

For requestFrame, I don't fully understand whether deferring draw where it was previously called synchronously will have undue side-effects, but since most overlays will be pegged to a 'drawn' callback, and centerCoordinate continues to be updated, I'm pretty sure it will be All Good.

Yeah, I haven't seen any negative side effects from this.

The ratchety zooming in Safari and Firefox seem to be due to some really extreme rounding in their usage of css transforms: http://bit.ly/pXwj2Z so that'll take a little while to fix, and might just require switching off transforms for them.

[RandomEtc]

Hmm, given the simple scales and translates being performed I'm surprised that the rounding hurts that badly, that's a shame. Might be worth trying translate3d and scale3d instead of a big matrix? (Would need transform-origin setting too to keep it simple, I think). Or are you allowed to set the WebKitCSSMatrix object in code rather than creating strings?

To avoid seams when scaling with CSS it's probably worth trying to round the scales to end up with a scaled size that has round pixels (consider calculating scale from point.width / el.width, assuming el.width is the pixel size of the image?).

I just tested the touch example on the ipad and aside from the seams I think the pinch behavior is wrong. Where previously we had it zooming like the Google Maps app does (map locations under your fingers are constant) it now zooms around the center of the screen. This might not be clearly wrong on the iphone with a small screen, but it doesn't feel right on the ipad to me. I can look at this if you'd like?

[tmcw]

I just tested the touch example on the ipad and aside from the seams I think the pinch behavior is wrong. Where previously we had it zooming like the Google Maps app does (map locations under your fingers are constant) it now zooms around the center of the screen. This might not be clearly wrong on the iphone with a small screen, but it doesn't feel right on the ipad to me. I can look at this if you'd like?

You're right - just addressed this in ecdb8fedf04c2292218c8ca6f31a5e520e50edcf - that also does away with the silly translation-from-initial-coordinate trick I was using, but it also introduces some jitteriness, like on the desktop. Maybe this is due to zoomByAbout? That's the only thing I can easily think of. The jitteriness you've been seeing is with the MouseWheelHandler, right?

[tmcw]

Yep - zoomByAbout was the problem - it was calling draw() twice, and one time without getFrame. Didn't make much sense to call draw() twice, so I just pulled the one operation it's doing into zoomByAbout. So, fixed that in 78a7523fceb432c29138e7334e31b22399634cbf

Now all that's left from where I can see is seams. Will check that out now.

[tmcw]

Whoah, well, coffee worked and I got the math right on the first try (as far as I can see). White boundary lines fixed in 4b9437aa00514f0265848c0e550f1487f3dfcb3e

[RandomEtc]

Awesome. iPad touch test looking good from here*. Safari, Firefox and Chrome anyscale tests looking good too. Also working nicely on the unholy WineBottler version of IE6 I'm running. Nice catch on the zoomByAbout connection - this is solid!

Look forward to seeing this merged once others have had a chance to check it out!

*fading could use some extra work when zooming out but I see that's only in the example so no worries there, and it looks lovely when panning/zooming so I'd love to see it in core eventually.

[RandomEtc]

One more since this has become the epic-branch-merge-issue. You create var ms = MM.matrixString(point); but never use it, and call matrixString again instead.

[tmcw]

Fixed that in 09d0de0cc160e20d2f74367e26ae23ad606d4455 - just left over from debugging :/

[RandomEtc]

This epic thread is now closed :)

v0.18.3, now on master, contains all this excellent work.