Repository navigation
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
There several accidental zoom in/zoom out screen-20261011-164227.mp4 |
|
|
||
| // The gesture received a real movement; remember the latest distance | ||
| // even when the preview itself is throttled. | ||
| gesture.moved = true; |
There was a problem hiding this comment.
moved is set on any touchmove, including sub-pixel finger jitter. On a real device a two-finger tap almost always emits at least one touchmove, so the guard in endPinch that's meant to protect taps (9.5px → 10px, 99px → 72px) basically never applies. The tests only cover the no-touchmove case. Could we set moved only once the distance changes past a small threshold (a few px)?
| // even when the preview itself is throttled. | ||
| gesture.moved = true; | ||
| gesture.pendingDistance = touchDistance( | ||
| event.touches[0], |
There was a problem hiding this comment.
Using touches[0]/touches[1] without tracking Touch.identifier means the finger pair can change mid-gesture: pinch with A+B, a third finger C lands (ignored since pinching), A lifts → touches.length === 2 so endPinch returns early, and the next move measures B–C against the A–B startDistance → font size jumps (and can be persisted). Suggest storing both identifiers on touchstart, looking them up on move, and ending the pinch when either disappears.
| function onTouchStart(event: TouchEvent) { | ||
| if (gesture.pinching || event.touches.length < 2) return; | ||
|
|
||
| event.preventDefault(); |
There was a problem hiding this comment.
Only the second finger's touchstart gets preventDefault. The first finger's touchstart/pointer events still reach CodeMirror and touchSelectionMenu's capture-phase pointer handlers:
- If the first finger moves slightly before the second lands, native scroll has started and subsequent touchmoves are
cancelable=false→ the editor scrolls and zooms. - With the quick-tools Shift/Ctrl modifier active,
#onGlobalPointerDowncaptures a selection session for the second pointer; if it lifts withinTAP_MAX_DISTANCE(common when only one finger moves),#commitPointerSelectionextends/adds a selection.
Might be worth suppressing/cancelling those when a pinch starts.
| ); | ||
|
|
||
| const now = Date.now(); | ||
| if (now - gesture.lastUpdate < ZOOM_THROTTLE_MS) return; |
There was a problem hiding this comment.
Leading-edge throttle with no trailing flush: if the fingers stop within 50ms of the last applied frame, the preview stays stale until touchend, then jumps to a size the user never previewed before it's saved. Coalescing via requestAnimationFrame (store pendingDistance, schedule one rAF, cancel in destroy) would be smoother and simpler.
| // (rebuilding the font theme in every pane) and writes settings.json | ||
| // exactly once per gesture, before the inline preview is dropped below. | ||
| settings.value.fontSize = `${gesture.lastPx}px`; | ||
| settings.update(false); |
There was a problem hiding this comment.
settings.update() returns a promise (it awaits the settings.json write) that's neither awaited nor caught. If the write fails, it becomes an unhandled rejection; the editor already shows the new size but it silently reverts next launch. A .catch(...) would help.
|
|
||
| const { dom } = view; | ||
|
|
||
| dom.addEventListener("touchstart", onTouchStart, { passive: false }); |
There was a problem hiding this comment.
Listeners are on view.dom, which also contains the search panel, autocomplete/hover tooltips, and gutters. A two-finger gesture on any of those zooms the editor and preventDefault blocks two-finger scrolling inside scrollable tooltips/panels. Consider attaching to view.scrollDOM/contentDOM, or skipping targets inside .cm-panels / .cm-tooltip.
| // value, so writing settings mid-gesture can never undo a preview once | ||
| // the pinch returns to its starting size, and would leave the font | ||
| // theme stale as soon as the inline preview is removed. | ||
| view.dom.style.fontSize = `${px}px`; |
There was a problem hiding this comment.
Changing view.dom.style.fontSize outside CM's update cycle relies on the contentDOM ResizeObserver to re-measure, which skips onResize if the doc view updated in the last 75ms. If a selection/LSP update lands mid-pinch, cursor/selection layers and gutters can stay at stale geometry. Calling view.requestMeasure() after applying the inline size would make this deterministic.
| import settings from "lib/settings"; | ||
|
|
||
| const ZOOM_THROTTLE_MS = 50; | ||
| const MIN_FONT_SIZE = 6; |
There was a problem hiding this comment.
Nit: these limits (and the px-parsing fallback) duplicate adjustFontSize in src/cm/commandRegistry.js (Math.min(72, Math.max(6, …)), parseInt(..., 10) || 12). Exporting the constants/clamp helper from one place and reusing it there would keep pinch and the font-size commands from drifting.
| pushExtension(extensions, options.commandKeymapExtension); | ||
| pushExtension(extensions, options.themeExtension); | ||
| extensions.push(fixedHeightTheme); | ||
| extensions.push(pinchZoom()); |
There was a problem hiding this comment.
Nit: pinchZoom() calls ViewPlugin.define on every createMainEditorExtensions call (each file open / state recreate), producing a new plugin identity each time. A single module-level ViewPlugin constant (like searchMatchHighlighter below) would be cheaper and lets view.plugin(...) find it.
3 participants
Description
Adds pinch-to-zoom support to the editor to make editing more convenient on mobile devices.
Users can adjust the editor's font size using a two-finger pinch gesture, improving readability and usability on smaller screens.
Changes Made
Testing
Related Issue
Closes #2426