fix(frontend): 横スワイプの挙動改善#17101
fix(frontend): 横スワイプの挙動改善#17101kakkokari-gtyih wants to merge 12 commits intomisskey-dev:developfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #17101 +/- ##
============================================
- Coverage 63.48% 13.63% -49.85%
============================================
Files 1161 241 -920
Lines 115968 11858 -104110
Branches 8351 3983 -4368
============================================
- Hits 73619 1617 -72002
+ Misses 40159 8040 -32119
- Partials 2190 2201 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves the horizontal swipe behavior and refactors animation handling in the frontend components. The changes address issues with swipe gesture tracking, animation artifacts, and performance.
Changes:
- Fixed horizontal swipe initiation by ignoring initial distance traveled before swipe starts
- Replaced CSS easing with JavaScript-controlled animations for better touch tracking
- Improved swipe detection by relaxing element checks in
hasSomethingToDoWithXSwipe - Refactored
MkPullToRefreshto userequestAnimationFrameinstead ofsetIntervalfor better performance
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/frontend/src/components/MkSwiper.vue | Refactored horizontal swipe handling with improved tracking, effective distance calculation, and requestAnimationFrame-based animations |
| packages/frontend/src/components/MkPullToRefresh.vue | Replaced setInterval with requestAnimationFrame for animation loop and added cancellation support |
Comments suppressed due to low confidence (1)
packages/frontend/src/components/MkSwiper.vue:249
- On line 244, when
currentTabIndex.value === 0, accessingprops.tabs[currentTabIndex.value - 1](i.e.,props.tabs[-1]) is undefined in JavaScript. The short-circuit evaluation prevents the error from occurring, but the logic is checking a non-existent array element. Similarly on line 247, when at the last index,props.tabs[currentTabIndex.value + 1]accesses beyond array bounds. While JavaScript returnsundefinedfor out-of-bounds access and the conditions work correctly due to short-circuiting, this could be confusing. Consider checking bounds before accessing array elements for clarity:(currentTabIndex.value > 0 && props.tabs[currentTabIndex.value - 1]?.onClick)and(currentTabIndex.value < props.tabs.length - 1 && props.tabs[currentTabIndex.value + 1]?.onClick).
if (currentTabIndex.value === 0 || props.tabs[currentTabIndex.value - 1].onClick) {
distanceX = Math.min(distanceX, 0);
}
if (currentTabIndex.value === props.tabs.length - 1 || props.tabs[currentTabIndex.value + 1].onClick) {
distanceX = Math.max(distanceX, 0);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
packages/frontend/src/components/MkPullToRefresh.vue:293
- The
onUnmountedcleanup doesn't handle the case wheremouseuportouchendevent listeners might still be registered on the window. These listeners are added withonce: trueon lines 116 and 135, but if the component unmounts before the user releases their mouse/touch, these listeners will remain until they're triggered. Consider removing these listeners explicitly inonUnmountedto prevent potential memory leaks or unexpected behavior after unmount.
onUnmounted(() => {
if (moveBySystemCancel != null) {
moveBySystemCancel();
moveBySystemCancel = null;
}
moveBySystemRafId = null;
// pull中にwindowへ登録したリスナーが残るのを防ぐ
window.removeEventListener('mousemove', onMouseMove);
window.removeEventListener('touchmove', onTouchMove);
unlockDownScroll();
if (rootEl.value) rootEl.value.removeEventListener('mousedown', moveStartByMouse);
if (rootEl.value) rootEl.value.removeEventListener('touchstart', moveStartByTouch);
if (rootEl.value) rootEl.value.removeEventListener('touchend', toggleScrollLockOnTouchEnd);
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (swipeAborted) return; | ||
|
|
||
| if (hasSomethingToDoWithXSwipe(event.target as HTMLElement)) return; |
There was a problem hiding this comment.
movingByPointer calls hasSomethingToDoWithXSwipe(event.target as HTMLElement) without verifying that event.target is a non-null HTMLElement. If the pointer events are retargeted (e.g. to a Text node, SVGElement, or null), this will throw at runtime inside hasSomethingToDoWithXSwipe (uses tagName/getComputedStyle). Add the same instanceof HTMLElement guard used in moveStartByPointer (or use event.currentTarget).
| if (hasSomethingToDoWithXSwipe(event.target as HTMLElement)) return; | |
| const target = event.target; | |
| if (!(target instanceof HTMLElement)) return; | |
| if (hasSomethingToDoWithXSwipe(target)) return; |
|
|
||
| if (!isSwiping.value) return; | ||
|
|
||
| if (hasSomethingToDoWithXSwipe(event.target as HTMLElement)) return; |
There was a problem hiding this comment.
moveEndByPointer also passes event.target as HTMLElement into hasSomethingToDoWithXSwipe without checking the target type/nullability. Pointerup targets can be outside the component (or non-HTMLElement), which can lead to a runtime error. Consider guarding with event.target instanceof HTMLElement (or switching to event.currentTarget).
| if (hasSomethingToDoWithXSwipe(event.target as HTMLElement)) return; | |
| const target = event.currentTarget ?? event.target; | |
| if (!(target instanceof HTMLElement)) { | |
| resetState(); | |
| return; | |
| } | |
| if (hasSomethingToDoWithXSwipe(target)) return; |
| function moveCancelByPointer(event: PointerEvent) { | ||
| if (event.pointerType === 'mouse') return; | ||
| if (activePointerId != null && event.pointerId !== activePointerId) return; | ||
| resetState(); | ||
| closeContent().finally(() => { | ||
| isSwipingForClass.value = false; | ||
| }); |
There was a problem hiding this comment.
moveCancelByPointer runs for lostpointercapture, but its guard allows execution even when activePointerId is already null (e.g. after a normal pointerup where resetState() already ran). This can trigger an extra closeContent() call that cancels/restarts the release animation unnecessarily. Tighten the guard to only handle cancel when isTracking is true and event.pointerId === activePointerId (or track the last captured id separately), and/or release pointer capture explicitly on end/cancel and drop the lostpointercapture handler if it’s redundant.
|
/preview |
|
スワイプした時の感触が軽すぎるのでなんか入れる |
What
いくつかの修正
いくつかのリファクタ
Why
UX / Refactor
Additional info (optional)
Checklist