Skip to content

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Mar 18, 2025

I made the button a bit bigger and moved the swipe recognizer around the whole screen. Typically these are used around the whole content without any affordances and not as a standalone scrubber. Ideally the swipe would be able to be inside the animating content but it can't yet due to this Safari bug.

Added back some paragraphs so that scrolling can be tested properly. It appears it's possible to get the swipe to be a bit misaligned if you scroll enough on iOS.

Screenshot 2025-03-17 at 10 27 42 PM

It's confusing to demo this updating when they happen to line up. It looks
like a bug when one side isn't updating.
@react-sizebot
Copy link

react-sizebot commented Mar 18, 2025

Comparing: 3c3696d...e5af461

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 517.29 kB 517.29 kB = 92.26 kB 92.26 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 608.06 kB 608.06 kB = 107.88 kB 107.88 kB
facebook-www/ReactDOM-prod.classic.js = 652.54 kB 652.54 kB = 114.95 kB 114.95 kB
facebook-www/ReactDOM-prod.modern.js = 642.82 kB 642.82 kB = 113.36 kB 113.36 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against e5af461

width: 200px;
border: 1px solid #333333;
width: 300px;
background: #eee;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

<p></p>
{show ? null : (
<div className="swipe-recognizer">
<SwipeRecognizer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i missed this, this is sick

@tjallingt
Copy link
Contributor

On android with chrome the reproduction page also behaves a bit strange. Im unable to scroll the scrollable area until the background has finished animating.

@sebmarkbage
Copy link
Collaborator Author

@tjallingt It’s expected that the start of a gesture can be slightly delayed if there was another view transition animation already happening. Since you can only have one and it’s not able to smoothly transition from one to another, we wait for one to finish.

It should be hard to repro though if the animation is fast enough.

Can you share a recording of your device using another camera so we can see if it’s expected?

@sebmarkbage sebmarkbage merged commit a35aaf7 into facebook:main Mar 18, 2025
194 checks passed
@sebmarkbage
Copy link
Collaborator Author

I think I know what you're saying now. While the momentum scrolling is going, the scroll doesn't receive new touches so you can't "catch" the scroll again until it reaches the scrollend and it can receive touches again. Unlike on iOS where this is possible. Albeit a bit strange if the "action" is allowed to be performed on touchend.

@tjallingt
Copy link
Contributor

tjallingt commented Mar 18, 2025

https://youtu.be/AHudV45jm7s

When starting the scroll/swipe the transition starts and the background starts to fade and the existing list becomes unresponsive.
If you release the scroll/swipe the list (both old and new) becomes unresponsive until the end of the transition.
If you keep the scroll/swipe gesture active the new list that fades in is "responsive" to the same scroll/swipe gesture.

It sounds like this is the "expected" behaviour but it feels a bit surprising that the new list would be unresponsive during the transition (if the scroll was ended).

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Mar 18, 2025

@tjallingt I see you're referring to the linked repro in https://bugs.webkit.org/show_bug.cgi?id=288795 and not this particular fixture editing in this PR. I was confused about the context.

The issue I'm observing in Chrome is slightly different from Safari. It seems like you have the same experience based on the video. If you keep the finger down while moving the whole time it works like expected. You can see this around 0:45 in your video. The momentum scroll stops so early so just a flick isn't enough in this rather poor repro (sorry) because it stops before the fade in of the "new" state becomes visible. Once you lift the finger you can't regain the scroll to work again by touching down again. I'm still looking into whether we can find a way to work around this.

The Safari issue is the reverse. If you keep the finger down, it suddenly stops but if you lift it and press down again you can regain scroll. This is worse because it means you can't have one continuous swipe drive the ViewTransition. Where as the Chrome one is annoying because you can't lift and then regain the swipe but you've already committed to an action at that point.

However, ideally both would work in both scenarios. "New" should just be live to existing scroll shouldn't be interrupted and new scroll should be able to start while it's running.

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Mar 18, 2025

The Chrome one seems especially weird because it happens even if the parent DOM element isn't actually running a ViewTransition like in the fixture in this PR. So that seems to be a different cause. Maybe something is stealing pointer events. See correction below.

@sebmarkbage
Copy link
Collaborator Author

Correction: The Chrome issue is actually just the regular thing where view-transitions can cover up the hit test unless they have pointer-events: none on them in which case it works as expected.

I was also additionally confused because in the fixture of this PR there's also a touch-action: pan-left (or pan-right) so you can't swipe in the opposite direction.

By adding pointer-events: none and touch-action: pan-x I can get it to work. So no "bug" per se. Although maybe it would be better to allow hit testing for simplicity.

sebmarkbage added a commit that referenced this pull request Mar 18, 2025
Follow up to #32656.

Remove touchAction from SwipeRecognizer. I was under the wrong
impression that this was only the touch-action applied to this
particular element, but that parents would still win but in fact this
blocks the parent from scrolling in the other direction. By specifying a
fixed direction it also blocked rage-swiping in the other direction
early on.

Disable pointer-events on view-transition so that the scroll can be hit.
This means that touches hit below the items animating above. This allows
swiping to happen again before momentum scroll has finished. Previously
they were ignored. This only works as long as the SwipeRecognizer is
itself not animating. This means you can now rage-swipe in both
directions quickly.
github-actions bot pushed a commit that referenced this pull request Mar 19, 2025
Follow up to #32656.

Remove touchAction from SwipeRecognizer. I was under the wrong
impression that this was only the touch-action applied to this
particular element, but that parents would still win but in fact this
blocks the parent from scrolling in the other direction. By specifying a
fixed direction it also blocked rage-swiping in the other direction
early on.

Disable pointer-events on view-transition so that the scroll can be hit.
This means that touches hit below the items animating above. This allows
swiping to happen again before momentum scroll has finished. Previously
they were ignored. This only works as long as the SwipeRecognizer is
itself not animating. This means you can now rage-swipe in both
directions quickly.

DiffTrain build for [c2a1961](c2a1961)
github-actions bot pushed a commit that referenced this pull request Mar 19, 2025
Follow up to #32656.

Remove touchAction from SwipeRecognizer. I was under the wrong
impression that this was only the touch-action applied to this
particular element, but that parents would still win but in fact this
blocks the parent from scrolling in the other direction. By specifying a
fixed direction it also blocked rage-swiping in the other direction
early on.

Disable pointer-events on view-transition so that the scroll can be hit.
This means that touches hit below the items animating above. This allows
swiping to happen again before momentum scroll has finished. Previously
they were ignored. This only works as long as the SwipeRecognizer is
itself not animating. This means you can now rage-swipe in both
directions quickly.

DiffTrain build for [c2a1961](c2a1961)

DiffTrain build for [646835f](646835f)
github-actions bot pushed a commit that referenced this pull request Mar 19, 2025
Follow up to #32656.

Remove touchAction from SwipeRecognizer. I was under the wrong
impression that this was only the touch-action applied to this
particular element, but that parents would still win but in fact this
blocks the parent from scrolling in the other direction. By specifying a
fixed direction it also blocked rage-swiping in the other direction
early on.

Disable pointer-events on view-transition so that the scroll can be hit.
This means that touches hit below the items animating above. This allows
swiping to happen again before momentum scroll has finished. Previously
they were ignored. This only works as long as the SwipeRecognizer is
itself not animating. This means you can now rage-swipe in both
directions quickly.

DiffTrain build for [c2a1961](c2a1961)

DiffTrain build for [646835f](646835f)

DiffTrain build for [db7dfe0](db7dfe0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants