Improved zoom a lot #19

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
2 participants
@aseemk

aseemk commented Mar 6, 2011

Hey there,

First of all, great work on iScroll! And thanks for providing such a useful library to the community.

I'm helping Bondi build an iPad magazine viewing web app, and we're using iScroll for (a) zooming into the magazine and (b) panning around smoothly while you're zoomed in. We saw some room for improvement (and bug fixes) in your implementation of zoom, so I made those fixes and thought you might find them valuable too. You can view my specific commits for the details, but the two most relevant commits are 2b40324 and ec42671.

Note that I originally forked pre-beta-1, from your Feb 10 "Updated zoom" commit 97e0158 specifically, but I've taken care of merging both of our changes.

Here is the overall summary of what I did:

  • Improved the robustness of zoom in general a lot. Most importantly, the code behaved differently if you started with one finger then added a second vs. put both fingers down at the same time, and same with release. It also improperly snapped to the top-left every time if you never panned during the gesture. Fixed all these things.
  • Changed min and max zoom clamping (constraints) to no longer apply while you're zooming; now they only kick in after your fingers leave the screen. This is not only consistent with native iOS apps (e.g. Photos), it feels nicer because it's real direct manipulation.
  • These min and max zoom resets are now smoothly animated instead of instantly applied. This again matches native iOS apps.
  • Added a "zoom spread" test page that shows an example two-page magazine spread (it's an advertisement from the 70s!) and wires up iScroll to zoom in on it like the way we use it. If you play with different combinations of starting and doing and ending zoom with your two fingers, you will see a night-and-day difference before and after.
  • If you're at the edge and drag beyond the edge, changed the dampening to 1/2 instead of 1/2.4. This is consistent with native iOS behavior. (You can see for yourself in any list, e.g. contacts, email, etc.)
  • Some scrollbar code tended to randomly throw errors because some element didn't exist. It wasn't obvious why this was happening, and it didn't seem crtical, so I wrapped these cases in try-catch blocks and logged the errors to the console. I saw in some cases you just commented those blocks out.
  • It didn't work on desktop browsers initially because you used to assume e.touches existed. I added checks for this, but saw you did too, so kept yours. (FWIW, I think straight feature detection via checking e.touches is a bit more explicit than your current hasTouches.)

Hope you find these changes valuable! Let me know if you have any questions; I'd love to get your feedback if you have any issues. Thanks again!

Cheers,
Aseem

aseemk added some commits Mar 4, 2011

Clamping zoom only after gesture now.
Min and max zoom constraints no longer apply *during* zoom, only *after* now.
Like the iPhone's Photos app, the reset is also a smooth animation now.

Also, changing the drag-to-pan dampener to 1/2 now when past the edge, instead
of the previous 1/2.4, to match the native iOS feel.
Improving the robustness of zoom a lot!
Before, logic for recognizing zoom start was inconsistent between:
- *one* touchstart event w/ *two* fingers, vs.
- *two* touchstart events w/ *one* finger each.
Now, the logic is consistent; the same event handlers are added, etc.

Before, if the only touchmove events were with two fingers down, they were
ignored completely, so the touchend handler would reset x and y, assuming no
movement happened. Now, we properly recognize zoom and don't reset then.

Together, these almost entirely (but not quite, argh!) eliminate the annoying
bug where at the end of a pinch zoom, the content snaps to the top-left.
Overall, though, the entire zooming experience is *much* better now.
Fixing my changes to have tabs instead of spaces.
(To be consistent with the way it already was.)
Merge branch 'master' into bondi
Conflicts:
	src/iscroll.js
@cubiq

This comment has been minimized.

Show comment
Hide comment
@cubiq

cubiq Mar 6, 2011

Owner

I was going to debug the zoom functionality today, you just saved my Sunday ;) I'll review your modifications and consider them for inclusion. Thanks!

Owner

cubiq commented Mar 6, 2011

I was going to debug the zoom functionality today, you just saved my Sunday ;) I'll review your modifications and consider them for inclusion. Thanks!

@aseemk

This comment has been minimized.

Show comment
Hide comment
@aseemk

aseemk Mar 6, 2011

Awesome, glad to help. =) Fwiw, if you're new to pull requests like I am, the "diff" tab above is way more helpful for looking at the code changes than the "commits" tab (because the merge commit has almost every change).

aseemk commented Mar 6, 2011

Awesome, glad to help. =) Fwiw, if you're new to pull requests like I am, the "diff" tab above is way more helpful for looking at the code changes than the "commits" tab (because the merge commit has almost every change).

This issue was closed.

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