Repository navigation
Conversation
jasmussen
added
[Type] Enhancement
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Size Change: +1.36 kB (+0.02%) Total Size: 7.73 MB 📦 View Changed
|
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
c6eca7d to
022d8e4
Compare
|
Rebased, and worked in some small improvements. |
|
Flaky tests detected in 022d8e4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/30269701069
|
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
022d8e4 to
0b51fe4
Compare
|
It works well for me. It would be nicer if, after a certain number of pixels of swiping, the next/prev image is revealed to make the transition smoother and reinforce the swipe direction. Right now, the new image shows up slightly abruptly. |
I agree. When I worked on that, this aspect was a bit of a larger undertaking, so in the hopes of making this a small iterative improvement, I chose this simpler approach. But it should be possible to upgrade it with a second PR. Curious if we can get a developer instinct on this, happy either way. |
|
Noting that this PR is now blocking a followup in #81500, as both need to touch mobile swiping. |
|
I'd love eyes on this one and #81500 if you have time, @WordPress/gutenberg-design, I think they'd make nice little 7.2 improvements. |
|
I had the same instinct as @fcoveram but it seems fine to handle separately. |
There was a problem hiding this comment.
Great idea! Left a couple of code quality / code style nits, but generally testing pretty well in the mobile view of Chrome.
One bug I ran into is with cancelled drag events. I haven't tested this on a real phone or tablet, but for drag events in Chrome you can hit escape to cancel a drag part way through. In this case, the state gets stuck and isn't cleared, so if you re-open the item you closed, it'll be stuck in its drag offset:
2026-10-08.17.26.13.mp4
Should we add an event handler for touch cancel?
One other thought (not a blocker for this PR) is that it'd be nice to have this behaviour for click drags as well and not just for touch devices (I know I want to click and drag things on desktop quite a bit, rather than just clicking next and back buttons). So, there's an opportunity to put some of the logic here into functions that could be re-used across the different event handlers. Again, that could be a follow-up, though if you were keen on getting this PR in, in the shorter-term without refactoring.
I'm wrapping up for the week now, but happy to give this a re-review / closer look next week if you'd like a hand with it!
Edit: one other note is that it seems is-fading-in sticks around after it concludes. Is that an issue, or is it fine? I couldn't see any visual issues with it lingering, just wondering if there's potential for it to conflict with other animations.
| left: 50%; | ||
| transform-origin: top left; | ||
| transform: translate(-50%, -50%); | ||
| transform: translate(calc(-50% + var(--wp--lightbox-drag-offset, 0px)), -50%); |
There was a problem hiding this comment.
Not sure how much of an issue it is for this PR as it might become more apparent if/when we get to the point of showing the next / prev image while dragging, but just wondering if the transform drag offset belongs on the lightbox-image-container or on the img element?
(This could be looked at in a follow-up, so not a blocker for this PR)
|
|
||
| - Playlist: Shorten the track toolbar button label from "Add track" to "Add". | ||
| - Gallery: Rename the dynamic variation's "Convert to images" action to "Detach", and confirm it in a dialog explaining that the gallery will keep its current images but stop updating automatically ([#80727](https://github.com/WordPress/gutenberg/pull/80727)). | ||
| - Image: Animate touch-swipe navigation in the gallery lightbox so the image follows the finger and slides off on commit, with a quick fade-in for the next image ([#79114](https://github.com/WordPress/gutenberg/pull/79114)). |
There was a problem hiding this comment.
Tiny nit: this needs to be moved up to the unreleased section
| */ | ||
| const touchDrag = { | ||
| isDragging: false, | ||
| direction: 'unknown', |
There was a problem hiding this comment.
'unknown' seems like a bit of an odd value for a default state. Why not null or undefined?
| touchDrag.isDragging = false; | ||
| touchDrag.direction = 'unknown'; | ||
| touchDrag.overlayEl = null; |
There was a problem hiding this comment.
We're repeating this clearing behaviour in a couple of places. Is it worth adding a function to handle resetting touchDrag to defaults? This might be useful for the bug with cancelling a drag (I'll comment on that separately).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ghtbox The swipe commit animation read the media query inline. Trunk has since moved `prefersReducedMotion()` into the block library's shared utils (#84297), so use that instead of a second copy of the same query. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ipes Addresses review feedback on #79114. A drag the browser cancels — Escape during a drag in Chrome, or a system gesture taking over — fires `touchcancel` instead of `touchend`, so the drag offset was left on the overlay. The overlay is a singleton, so the offset survived closing and reopening the lightbox and the image appeared pushed off-centre. Reopening a different image hid it, because that recomputes the overlay styles; reopening the same one did not. Handle `touchcancel` and abandon the drag. The reset was also spelled out in two places and did not cover the styles, so collect it in `resetTouchDrag()`, `clearDragStyles()` and `flushPendingSlide()`. Flushing rather than cancelling a pending slide fixes a second stuck state: touching the overlay during the commit slide dropped the timer that both swapped the image and cleared the offset, leaving the image parked off-screen. Take `is-fading-in` back off once the fade is over, so the class means what it says. It outranks the `.zoom.active` rule that animates the image in, though nothing restarts that animation while the overlay stays open, and closing the lightbox clears the class along with the rest of the rendered class list — so there is no visual bug to fix here, only a state that should not be left set. Also use `null` rather than 'unknown' for the un-latched drag direction, and move the changelog entry to the unreleased section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Taking `is-fading-in` off once the fade was over restarted the zoom-up animation the lightbox plays when an image is first opened: the `.zoom.active` rule declares that animation on the image container for as long as the overlay is open, and the fade rule only shadows it. Removing the class changes the container's computed `animation-name` back, which the browser treats as a new animation and runs from the start — so a swipe finished with the next image fading in, blinking out, and zooming up from the gallery as if it had just been opened. An animationstart trace over one committed swipe shows the fade at 245ms and `lightbox-zoom-in` at 386ms; pinning the class on removes the second entry. So leave the class on, as it was before, and say why. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The swipe used a CSS transition and a CSS animation, switched on by classes toggled on the overlay, with a `setTimeout` waiting out durations that had to be kept in step with `style.scss` by comment. That coupling produced both of the bugs in this branch: toggling a class changed the image container's computed `animation-name`, which restarted the open-zoom animation, and a stale timer could strand the image off-screen. Animate the slide and the fade with `element.animate()` instead. Scripted animations take precedence over an element's CSS animations without touching its class list, so the open zoom the lightbox plays — rebuilt in #79058 — is left entirely alone, and the swipe cannot restart it. The durations now live only in `view.js`; `is-animating-slide`, `is-fading-in` and their keyframes are gone from the stylesheet. `animation.cancel()` replaces the hand-rolled abort, and settling a swipe that is still in flight replaces the flush, so an interrupted or cancelled gesture has one way to end rather than several. The drag offset also moves from the overlay to the image containers. The overlay's `style` attribute is bound to `state.overlayStyles`, and the Interactivity API assigns a bound style string to `cssText`, so a resize part way through a gesture rewrote the attribute and wiped the drag out from under the finger. Nothing is bound to the containers. Verified in a gallery lightbox: no CSS animation starts at all during a swipe, `lightbox-zoom-in` still plays on open and reopen, a drag survives a viewport change mid-gesture, and cancelled, interrupted and uncommitted gestures all return the image to rest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c4ee325 to
4de181b
Compare
🤖 PR meta 🤖🎉 PropsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Updated as activity occurs, without notifying anyone named here. Add the 📦 Bundle sizeSize Change: +767 B (+0.01%) Total Size: 8.3 MB 📦 View Changed
⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
🏁 Flaky testsSome tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information. should update the URL from the last navigation if only varies in the URL fragment in
|
|
Thanks a ton for the review. I rrebased on trunk, which pulled in #79058, which rebuilt the open animation, and it turned out to collide with this PR. Fixing that properly meant moving the swipe off CSS classes altogether. The slide and fade are now Your feedback should be addressed as part of this, even if the JS is now larger:
On where the transform belongs: the containers rather than the It works pretty well for me: 
|
This branch has not been deployed
4 participants
What?
Followup to #62906. Adds animation to the swiping gesture for mobile galleries:
Why?
When you have a gallery set to lightbox ("expand on click"), no mobile you can open it and use a swipe gesture to advance between images. However there is no visual feedgback that this is possible. This PR adds that.
Testing Instructions
Create a gallery, set to expand on click, then test on mobile, or using the web-inspector set to device emulation. Test swiping images to advance.
Use of AI Tools
Claude Opus 4.7.