Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(iOS): setting correct default DecelerationRate for iOS to normal #2815

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ftlno
Copy link

@ftlno ftlno commented Jan 19, 2023

  • bugfix setting correct default DecelerationRate for iOS to normal rate if undefined.
  • updating wrong docs.

Source: developer.apple.com
https://developer.apple.com/documentation/uikit/uiscrollview/decelerationrate

@ftlno ftlno changed the title bugfix - setting correct default DecelerationRate for iOS to normal fix(iOS): setting correct default DecelerationRate for iOS to normal Jan 19, 2023
Copy link
Collaborator

@TheAlmightyBob TheAlmightyBob left a comment

Choose a reason for hiding this comment

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

The Types fix makes sense, and looks like that should also be fixed in the documentation? (the Reference.md files)

src/WebView.ios.tsx Outdated Show resolved Hide resolved
@TheAlmightyBob
Copy link
Collaborator

@Titozzz This "fix" makes sense, but thoughts on dealing with the "breaking change" aspect (changing the default, thus likely leading to different scrolling behavior for existing apps)? Would something like this justify a major version bump?

@ftlno
Copy link
Author

ftlno commented Mar 7, 2023

Can this be merged @Titozzz ?

@Titozzz
Copy link
Collaborator

Titozzz commented Mar 7, 2023

@Titozzz This "fix" makes sense, but thoughts on dealing with the "breaking change" aspect (changing the default, thus likely leading to different scrolling behavior for existing apps)? Would something like this justify a major version bump?

It does and when I merge it, it will mark as breaking

@ftlno I've been hard working on the new arch pr but hopelly u can go through all open PRs soon 😉

@github-actions
Copy link

github-actions bot commented May 7, 2023

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label May 7, 2023
@github-actions github-actions bot closed this May 14, 2023
@eirikluka
Copy link

It would still be great to see this merged

@github-actions github-actions bot removed the Stale label May 23, 2023
@Titozzz
Copy link
Collaborator

Titozzz commented Jun 10, 2023

Hello, after going through as many stack overflow and apple threads I could:

From my understanding, WebView, even if it's using a scrollview has it's own default. See (https://stackoverflow.com/questions/14763255/why-does-scrolling-a-uiwebview-feel-so-much-different-than-scrolling-any-other#comment25190463_14763634)

Understanding this, I'm not sure that change make sense as the idea is to stick to native behavior as much as possible. Also users are free to set decelerationRate to normal if they want to. What we could probably improve is the documentation, explaining this.

WDYT?

@TheAlmightyBob
Copy link
Collaborator

@Titozzz The thing is, right now we're not really using the WebView default I don't think. The code is explicitly setting the deceleration rate to an uninitialized float (which maybe is treated the same as if we didn't set it at all? but I wouldn't bet on that..).

Point is valid though that maybe it's worth checking what the default value really is for the webview, in case it's different from a standalone scrollview (and update the documentation either way).

@ewan-m
Copy link

ewan-m commented Jun 15, 2023

It was my expectation that a standard web wrapper app such as this

export default function App() {
  return (
    <WebView
      style={styles.container}
      source={{ uri: "https://expo.dev" }}
    />
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    marginTop: Constants.statusBarHeight,
  },
});

would exactly match the scroll behaviour in the safari or chrome iOS apps. The fact it didn't felt janky and to me it's a hard fix to find that the decelerationRate={0.998} needs to be explicitly set.

I made a similar starter app in SwiftUI using the same underlying WebView component and the scroll behaviour matches safari/chrome apps by default

I'd suggest merging this PR or a similar fix and introducing a major version bump to account for users which expect the current scroll behaviour as default 😁 unless there's something I'm missing here! happy to share code for the swift ui demo too

@Titozzz
Copy link
Collaborator

Titozzz commented Jun 18, 2023

React native WebView should match whatever the default of WebView is when building a native app with ObjC/Swift. Someone should create a test app to find out the default even though I expect it will be "deceleration: fast"

@ewan-m
Copy link

ewan-m commented Jun 20, 2023

React native WebView should match whatever the default of WebView is when building a native app with ObjC/Swift. Someone should create a test app to find out the default even though I expect it will be "deceleration: fast"

It's set to 0.998 which corresponds to normal

Here's the logs and code from inside a swift app

UIScrollView.DecelerationRate.fast // = 0.99
UIScrollView.DecelerationRate.normal // = 0.998
WKWebView().scrollView.decelerationRate // = 0.998

@eirikluka
Copy link

eirikluka commented Jun 23, 2023

I expect it will be "deceleration: fast"

"Fast" decelerates too fast compared to Safari. We had to override to "normal" in our app to get the expected scroll desceleration behaviour

Edit: Also Apple's documentation on UIScrollView.DecelerationRate and decelerationRate

The default rate is normal

@TheAlmightyBob
Copy link
Collaborator

Has anybody tried just commenting out

scrollView.decelerationRate = _decelerationRate;
in a patch (and maybe having it log the current value instead of setting it), for science?

(I'm not ignoring the science @ewan-m already did above, I'm just curious to further double-check the "default" value in the context of react-native-webview)

@ewan-m
Copy link

ewan-m commented Jul 6, 2023

Has anybody tried just commenting out

scrollView.decelerationRate = _decelerationRate;

in a patch (and maybe having it log the current value instead of setting it), for science?
(I'm not ignoring the science @ewan-m already did above, I'm just curious to further double-check the "default" value in the context of react-native-webview)

that's a good suggestion quite curious too - but is there anything else to do to help getting this in?

@TheAlmightyBob
Copy link
Collaborator

that's a good suggestion quite curious too - but is there anything else to do to help getting this in?

It seemed to me like the main blocker was disagreement/confusion over what the correct default behavior would be. My suggestion was intended to help resolve that debate.

@ewan-m
Copy link

ewan-m commented Jul 25, 2023

that's a good suggestion quite curious too - but is there anything else to do to help getting this in?

It seemed to me like the main blocker was disagreement/confusion over what the correct default behavior would be. My suggestion was intended to help resolve that debate.

https://github.com/ewan-m/webview-scroll-speeds

Here's a repo with basic webview apps built in both Expo and Swift. Running on an iOS device you can noticeably feel the difference in scrolling behaviour. The Swift app prints out the defaults too. These defaults should be the same in both apps as they are both using the same underlying WebKit Webview

UIScrollView.DecelerationRate.fast // = 0.99
UIScrollView.DecelerationRate.normal // = 0.998
WKWebView().scrollView.decelerationRate // = 0.998

In order to make the Expo app match the Swift app you need to add decelerationRate="normal" or decelerationRate={0.998} to the props of the webview.

Funnily enough if you're using Apple Silicon (M1/M2) you can't notice the bug at all because there's a bug in the scroll behaviour of V14 Simulator itself https://developer.apple.com/forums/thread/668488 - I imagine this is where a lot of confusion must comes from!

Would you like a screen recording of both apps in my phone with examples of what changing the deceleration rate prop does?

Appreciate the diligence btw in double checking we're doing the right thing!

https://github.com/ewan-m/react-native-webview here's a fork with your suggestion as well and confirms the same thing

@ftlno
Copy link
Author

ftlno commented Aug 14, 2023

I'm looking forward to seeing this getting merged.

It seems like a anti pattern to make every developer writing webviews to understand that they should change this to normal to get the correct default behaviour. And understand that it breaks the UX of those who are used to using the wrong default, but its way better for future apps and developers to getting the right default.

@eirikluka
Copy link

Is there anything else we can do to get this merged?

Copy link

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Nov 13, 2023
@eirikluka
Copy link

@Titozzz @TheAlmightyBob – Please consider merging this fix

@github-actions github-actions bot removed the Stale label Nov 14, 2023
Copy link

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Jan 13, 2024
@eirikluka
Copy link

This is very much a bug still. Are there any reasons why you won't implement this fix, @Titozzz @TheAlmightyBob ?

@github-actions github-actions bot removed the Stale label Jan 20, 2024
@ftlno
Copy link
Author

ftlno commented Feb 22, 2024

Any reason why this isnt merged?

@ewan-m
Copy link

ewan-m commented Feb 29, 2024

hello! @Titozzz @TheAlmightyBob

Would be great if this would be merged. I think it contributes greatly to the perception of react native having a poor performance because a lot of people starting a migration project from a web app will be likely to begin with a Webview. They'll then notice it "seems to struggle to scroll the webpage" and think hmm maybe react native has poor performance - but that's not true! just this default is wrong 😁

Copy link

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Apr 30, 2024
@eirikluka
Copy link

The default is still wrong. Could we please get a response on this? @TheAlmightyBob @Titozzz

@github-actions github-actions bot removed the Stale label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants