Add double-tap-and-drag zoom gesture to 1up/2up modes - #1591
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1591 +/- ##
==========================================
+ Coverage 63.81% 64.56% +0.75%
==========================================
Files 69 69
Lines 6256 6358 +102
Branches 1391 1412 +21
==========================================
+ Hits 3992 4105 +113
+ Misses 2225 2214 -11
Partials 39 39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ow, reuse pinch-zoom pipeline Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…natively Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
9de00c2 to
7c0fdfb
Compare
…te against its native text-selection gesture Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jbuckner
left a comment
jbuckner
left a comment
There was a problem hiding this comment.
Really like this, and the state machine is well thought through. Notes inline.
The _pinchEnd one on ModeSmoothZoom.js:357 looks like a real blocker, the touch-action lock on :379 I think strands scrolls, and the multi-touch guards on :268/:308 are dead on every browser we ship to. The rest are nits plus one question about plain double-tap.
Lint is clean and all 42 tests pass.
| if (!this.activeTouch) return; | ||
|
|
||
| if (this.activeTouch.dragEngaged) { | ||
| await this._pinchEnd(); |
There was a problem hiding this comment.
I think this can hang. _drawPinchZoomFrame only resolves pinchMoveFramePromise when curScale != newScale, so any frame that doesn't change the scale leaves it pending forever, and _pinchEnd awaits it.
Pinch rarely hits that, but this gesture hits it easily since the scale only depends on dy. Any horizontal-only move at the end of the drag does it:
down (50,50) t=0 ; up t=10 ; down (50,50) t=100
move (50,90) t=120 // engage, zoom in
move (80,90) t=140 // sideways only, same dy, so same scale
up (80,90) t=160 // never completes
When I ran that, _handleTapUp never finished, so touch-action stayed none, BRsmooth-zooming and willChange: transform stayed on, and attachCtrlZoom() / attachScrollListeners() never ran. In 1up that drops the scroll -> updateVisibleRegion listener, so scrolling stops loading pages until another zoom gesture completes cleanly.
Think it's just a matter of resolving pinchMoveFramePromiseRes in the curScale == newScale case too.
| // Preemptively block native panning for a possible second tap, since | ||
| // touchAction is read by the browser at the *start* of that touch -- | ||
| // reacting to it once that touch is already moving is too late. | ||
| this.mode.$container.style.touchAction = "none"; |
There was a problem hiding this comment.
This locks the whole container for ~300ms after every tap, and I think it strands touches that turn out not to be double-taps.
If the next touch is more than 100px away it's not a candidate, so _handleTapDown calls _restoreTouchAction(), but as your comment here says, that's too late for a touch that already started. And _handleTapMove bails at if (!this.activeTouch.isDoubleTapCandidate) return, so nothing takes over either. So: tap, then flick-scroll somewhere else within 300ms, and nothing happens at all. Most noticeable in 1up where scrolling is the main thing you do.
Chrome's own version doesn't have this problem since it resolves the ambiguity natively. The Samsung Internet path below already handles this exact shape though (permanent touch-action: none plus interact.draggable -> _dragMove). Could we route non-candidate touches through _dragMove while the lock is on?
| * @param {{ pointerType: string, clientX: number, clientY: number, timeStamp: number, originalEvent: Event }} e | ||
| */ | ||
| _handleTapDown = (e) => { | ||
| const multiTouch = (e.originalEvent?.touches?.length ?? 1) > 1; |
There was a problem hiding this comment.
I don't think this ever fires. interact.js switches to Pointer Events whenever window.PointerEvent exists (browser.supportsPointerEvent, interact.js:403), which is Chrome/Edge/Firefox and Safari 13+. So originalEvent is a PointerEvent and has no .touches at all, and the ?? 1 always wins. Same for the copy in _handleTapMove on line 308.
I checked with a PointerEvent-shaped fake event: a "second finger" during an engaged drag doesn't cancel, activeTouch and pinching both stay set. The 'a second finger touching down cancels an in-progress double-tap-drag' test only passes because it builds touches: [{}, {}], which won't happen in a browser.
The down path gets rescued by the this.pinching check so it's not as bad as it looks, but the move path is unguarded. TouchesMonitor further down reads raw touchstart/touchend, which fire everywhere, so attaching it on all touch platforms instead of just iOS would make these guards real. (FWIW e.interaction.pointers won't work, the proxy only exposes interactable, element, prepared, pointerIsDown, pointerWasMoved.)
|
|
||
| // If this tap turns out to be the first of a double-tap(-drag) zoom, | ||
| // don't flip the page out from under it. | ||
| if (await this.smoothZoomer.isSingleTap()) { |
There was a problem hiding this comment.
Q: should the second tap be suppressed too? Tap 1's isSingleTap() resolves false so its flip is skipped, but tap 2's resolves true and flips. So a plain double-tap on a page still flips it once.
The description says plain double-tapping does nothing right now, and the comment in _handleTapUp says not to let the second tap seed a third, so I wasn't sure if that's intended. Probably matters for #1587.
| this.detachCtrlZoom(); | ||
| clearTimeout(this.doubleTapWindowTimer); | ||
| this.doubleTapWindowTimer = null; | ||
| this._resolveSingleTap(false); |
There was a problem hiding this comment.
Small one: detach() doesn't restore touch-action or clear pendingTap / activeTouch. Tap once, then switch modes within 300ms, and the old container keeps touch-action: none until it gets re-attached.
A _restoreTouchAction() right after this line should cover it. The _resolveSingleTap(false) above already nulls the resolver, so it won't accidentally flip a page.
| startY: e.clientY, | ||
| isDoubleTapCandidate, | ||
| dragEngaged: false, | ||
| startScale: this.mode.scale, |
There was a problem hiding this comment.
startScale doesn't look like it's read anywhere. Leftover?
| * count as a double-tap. Matches Android's ViewConfiguration.DOUBLE_TAP_TIMEOUT | ||
| * and Flutter's kDoubleTapTimeout, both 300ms; iOS doesn't publish a number. | ||
| */ | ||
| const DOUBLE_TAP_TIME_MS = 300; |
There was a problem hiding this comment.
Nit: the doc says end-of-first-tap to start-of-second, but the code measures down to down (line 370 and the "measured from the first tap's down" test both say so).
2 participants
QA: https://deploy-preview-1591--ia-bookreader.netlify.app/bookreaderdemo/demo-internetarchive?ocaid=theworksofplato01platiala#page/n9/mode/2up
Double-tap-and-drag to zoom is a common interaction on mobile devices, allowing an easy one-handed alternative to pinch-zooming. First notably added to Google Maps, it has since become a standard throughout Android, and is now present in Chrome, Firefox, Drive PDF viewer, and now -- BookReader!
Here is how it works:
(src)
Note: plain double tapping currently does nothing ; this will be a future expansion in #1587 . This is a stepping stone to that feature.
Note: On iOS this doesn't seem to be as prevalent, but I do see it in Apple Maps -- although of course the direction was reversed -- dragging up zoomed in instead of our. I made BR reverse the direction for iOS as a result, but not sure... UPDATE: removed it from iOS ; it seems like iOS has another action for double-tap-drag ; text selection! So just disabled on iOS for now. UPDATE: Turned it on for iOS so folks can try it and we can discuss.