feat: implement getScrollableNode to support Animated.

[shirakaba]

Summary

Probably fixes #740 (which was only closed due to inactivity).

In short: the issue is that React Native WebView does not support Animated.

Full details in this discussion thread with the maintainers, quoted below: https://twitter.com/LinguaBrowse/status/1211375582073761799?s=20

Does react-native-webview support RN's Animated on iOS at all? Its onScroll event doesn't update my Animated.Values, whereas the same code used with Animated.ScrollView does.

To clarify, I am using createAnimatedComponent.

I've compared the react-native-webview iOS implementation to that of RCTScrollView, and it seems to be doing all the same things, so I can't see why Animated.call is failing to fire on onScroll.

Later in the above-linked Twitter thread, Krzysztof Magiera deduced why it wasn't working:

In order to hook into native events with animated you should use WebView component version that is wrapped with Animated's createAnimatedComponent (https://facebook.github.io/react-native/docs/animated#createanimatedcomponent). This however can be used on components that map directly to native components

This however seems not to be the case for RNWV as it renders View first (see here https://github.com/react-native-community/react-native-webview/blob/master/src/WebView.ios.tsx#L356) and only then renders RNCWebView. What needs to be wrapped with createAnimatedComponent is the component created here

What happens now is that animated events are hooked to the View component that gets rendered instead of the webview and unfortunately View does not generate any

Here is how the component to which events are hooked is selected: https://github.com/facebook/react-native/blob/8553e1acc4195479190971cc7a3ffaa0ed37a5e0/Libraries/Animated/src/createAnimatedComponent.js#L52

He also suggested a workaround:

This actually gives me an idea for a workaround. If RNWV had getScrollableNode implemented that'd return ref to the underlying RNCWebView component then you should be able to wrap RNWV with createAnimatedComponent and it should all work.

The scrollable node thing was added to make it possible for things like ListViews or FlatList to support animated props that hook to native ScrollView despite not rendering native SV component directly

This is exactly what I've implemented.

onScroll behaviour is not changed; this newly introduced getScrollableNode() function only exists to be called by Animated.createAnimatedComponent(), so there is no impact on the rest of the codebase.

Test Plan

Here is a video of Animated now working:

https://twitter.com/LinguaBrowse/status/1211621250604376065?s=20

What's required for testing (prerequisites)?

I don't have any expertise on UI-testing animations, unfortunately. But before, Animated.call was failing to fire on onScroll, and now I suspect it does, so the functionality of that aspect could be a test target.

What are the steps to reproduce (after prerequisites)?

Simulate a scroll event (or pan gesture, I suppose).

Compatibility

OS	Implemented
iOS	✅
Android	✅

Checklist

Some of these still pending...

• I have tested this on a simulator (iOS)
• I have tested this on a device (iOS)
• I have tested this on a simulator (Android)
• I have tested this on a device (Android)
• I added the documentation in README.md
• I mentioned this change in CHANGELOG.md
• I updated the typed files (TS and Flow)
• I added a sample use of the API in the example project (example/App.js)

[elicwhite]

As a heads up, this approach isn’t compatible with Fabric because of findNodeHandle and we are soon going to be helping all popular community modules that have this pattern migrate off, including the built in lists. I’d recommend adding this in a Fabric compatible way.

Instead of having a getter that returns the findNodeHandle, id recommend having another prop called something like nativeRef:

<WebView nativeRef={ref => {}} />

[jamonholmgren]

The nativeRef prop approach looks perfect.

[shirakaba]

@TheSavior I'm not following what you're suggesting. @kmagiera showed that createAnimatedComponent relies upon getScrollableNode being implemented; I'm not sure how accepting a nativeRef prop replaces that.

[elicwhite]

Ah, thanks for showing me that. I didn’t realize that was what createAnimatedComponent was doing. Looks like we’ll need to make some changes in createAnimatedComponent to make it not rely on that function.

This approach is fine for now then, but we’ll need to revisit it to become compatible with Fabric down the line. :-(

[shirakaba]

Note: I'm finding that while this PR fixes the situation for Animated, it does not do so for Reanimated. Not sure why.

[kmagiera]

Not sure why the implementation of getScrollableNode from virtualized list is so sophisticated. For this particular case I'd recommend that we just use the following, simpler implementation that also does not rely on findNodeHandle (both animated and reanimated already use findNodeHandle so they actually expect the ref and not a node ID):

  getScrollableNode = () => {
    return this.webViewRef.current;
  };

[shirakaba]

Note: I'm finding that while this PR fixes the situation for Animated, it does not do so for Reanimated. Not sure why.

This should be fixed by software-mansion/react-native-reanimated#531 – I'll test this in a moment.

I'd recommend that we just use the following, simpler implementation that also does not rely on findNodeHandle...

I can confirm that @kmagiera's suggested getScrollableNode() implementation is working on Animated. I'll test it on Reanimated along with Krzysztof's latest PR for Reanimated support next.

[shirakaba]

Forgot to mention – yes, as of PR software-mansion/react-native-reanimated#531 on the Reanimated repo, this PR indeed adds support for Reanimated as well as Animated.

[elicwhite]

Would another solution to this be to use forwardRef? So when you attach a ref to WebView you get the ref to the native component?

That would also solve the Fabric compatibility problem and is consistent with what we've done for other components. My initial comment was because I thought it was important to get a ref to the WebView JS component and not the native component. I don't actually think that is important from looking at the code.

For an example of how you can make this work with the additional imperative methods webview exposes like goForward you can use something like setAndForwardRef that we have in RN core: https://github.com/facebook/react-native/blob/cd1e34d4a27b32d33a636f16c1184e100aab9707/Libraries/Components/CheckBox/CheckBox.android.js#L135-L140

setLocalRef: ref => {
  this._nativeRef = ref;
  if (this._nativeRef != null) {
    this._nativeRef.goForward = this.goForward.bind(this);
  }
},

[shirakaba]

Would another solution to this be to use forwardRef? So when you attach a ref to WebView you get the ref to the native component?

That would also solve the Fabric compatibility problem and is consistent with what we've done for other components.

If it helps Fabric compatibility, we certainly should do it. Some questions:

• Is setAndForwardRef React magic, or React Native magic? Just wondering, as it's undocumented, whether it works in React DOM too, or just React Native.
• What import path would I have to write to import setAndForwardRef into react-native-webview?

We'll have to take care not to break getWebViewHandle(), which assumes that there is always a non-undefined reference (this.webViewRef) to the NativeWebView. For cases where the user doesn't pass in a ref into the ref-forwarding component, I believe we'll need it to fall back to a private ref (like I do here, where this.myRef is inherited from a superclass).

[shirakaba]

Although, is @kmagiera's suggested solution incompatible with Fabric in some way? It's not using findNodeHandle() anymore:

getScrollableNode = () => {
    return this.webViewRef.current;
};

[elicwhite]

It’s not magic, just a function. Yep, it would work in ReactDOM. I’d recommend duplicating it vs reusing it. It’s an internal function that is likely to go away at some point from the core repo, there are easier ways to do this when using hooks.

I don’t like the getScrollableNode solution because it is a hack to make animated think it is a list view. Also, getScrollableNode returns a react tag in the lists so this would have the same name but a different return type. A bit unfortunate.

I think the forward ref approach is the best one if possible.

[panda0603]

is this fixed? I can't seem to access ref after using createAnimatedComponent for webview. Is there any workarounds for accessing ref in animated webview?

[shirakaba]

I can't seem to access ref after using createAnimatedComponent for webview. Is there any workarounds for accessing ref in animated webview?

@panda0603 This aspect caused me problems, too. I found that Reanimated exposes an API, getNode(), for getting a ref to the underlying node of the wrapped component:

import { WebViewSharedProps } from 'react-native-webview/lib/WebViewTypes';
import Animated from "react-native-reanimated";

/* Instantiating the Animated wrapper for the WebView */
const AnimatedWebView = Animated.createAnimatedComponent(WebView) as React.ComponentClass<Animated.AnimateProps<ViewStyle, WebViewSharedProps>>;

// ...

/* Registering the ref */
const ref = React.createRef<AnimatedWebView>();
<AnimatedWebView ref={ref}/>

// ...

/* Getting a ref to the underlying WebView component instance */
const webView = webViewRef.current;
// If it has been wrapped with Reanimated, unwrap it.
const webViewUnwrapped = webView.getNode ? webView.getNode() : webView;

[panda0603]

I can't seem to access ref after using createAnimatedComponent for webview. Is there any workarounds for accessing ref in animated webview?

@panda0603 This aspect caused me problems, too. I found that Reanimated exposes an API, getNode(), for getting a ref to the underlying node of the wrapped component:

import { WebViewSharedProps } from 'react-native-webview/lib/WebViewTypes';
import Animated from "react-native-reanimated";

/* Instantiating the Animated wrapper for the WebView */
const AnimatedWebView = Animated.createAnimatedComponent(WebView) as React.ComponentClass<Animated.AnimateProps<ViewStyle, WebViewSharedProps>>;

// ...

/* Registering the ref */
const ref = React.createRef<AnimatedWebView>();
<AnimatedWebView ref={ref}/>

// ...

/* Getting a ref to the underlying WebView component instance */
const webView = webViewRef.current;
// If it has been wrapped with Reanimated, unwrap it.
const webViewUnwrapped = webView.getNode ? webView.getNode() : webView;

Sorry for the late reply. Thanks for the work around, should try. So this is still on going issue which react-native-webview has to fix or react native animated side?

[shirakaba]

@panda0603

So this is still on going issue which react-native-webview has to fix or react native animated side?

This is on the React Native Reanimated side, and it's not so much a bug as a subtlety that is hard to catch. Once a React component is wrapped by Animated.createAnimatedComponent(), a ref to the wrapper means exactly that – just a ref to the wrapper component rather than the underlying WebView component. It's not an issue that is specific to React Native Webview.

Reanimated doesn't ref-forward the underlying WebView component, and they probably have a good reason not to (i.e. sometimes it is necessary to obtain a reference to the wrapper to call instance methods on it). So the getNode() API is a compromise to get the best of both worlds. There's not really a way around it, sadly. There are inevitably surprising aspects encountered when using Reanimated, because it does some really magical stuff.

[elicwhite]

My understand was that the webview component needs to use forwardRef to the native webview component so that when you call getNode you get the native component and don’t then need to also call getScrollableNode.

[shirakaba]

@TheSavior At present, when we call animatedRef.current.getNode() on <AnimatedWebView ref={animatedRef}/>, it gives us a reference to the current instance of the <WebView> class component (not the native WebView).

This is desirable, as we often want to call the cross-platform JS APIs such as goBack() belonging to the <WebView> class component instance, and less often want to call anything directly on the native WebView (but, as this PR stands, would still have the option by calling getScrollableNode()).

Not to mention, it's backwards-compatible with the current behaviour of the ref set upon the <WebView> class component indeed referring to the <WebView> class component.

[elicwhite]

Yeah, to solve that we have been attaching the wrapper JS component methods to the native ref. Check out TextInput.js in core for an example.

This has helped enable us remove the need for getScrollableNode in user code, although the implementation details are a bit gross

[shirakaba]

Yeah, to solve that we have been attaching the wrapper JS component methods to the native ref. Check out TextInput.js in core for an example.

There still seems to be a disparity between the JS component methods and the methods callable on the native ref (unless I'm misunderstanding). For instance, instead of there being a clear() method on the native ref, you have to set its native text prop to "":

TextInput.js clear() method

I think moving all the class component instance methods into the native modules is a whole different discussion because it's a massive refactor and, unless done perfectly consistently, would be a breaking change. I don't see why to go to that amount of effort just to remove a call to getScrollableNode() (or whatever the equivalent call would be via the Fabric-friendly setAndForwardRef implementation).

[elicwhite]

This is what I'm referring to:

https://github.com/facebook/react-native/blob/7813e24cdb55b0716df019edc0d875730bf65169/Libraries/Components/TextInput/TextInput.js#L1014-L1047

This works now:

<TextInput ref={ref => { 
    ref.measure(() => {
      ref.clear();
    })
  }} />

Instead of what was required previously:

<TextInput ref={ref => { 
    ref.getNativeViewRef().measure(() => {
      ref.clear();
    })
  }} />

[shirakaba]

I saw this commentary:

Hi reader from the future. I'm sorry for this. This is a hack. Ideally we would forwardRef to the underlying host component. However, since TextInput has it's own methods that can be called as well, if we used the standard forwardRef then these methods wouldn't be accessible and thus be a breaking change.

We have a couple of options of how to handle this:

1. Return a new ref with everything we methods from both. This is problematic because we need React to also know it is a host component which requires internals of the class implementation of the ref.

2. Break the API and have some other way to call one set of the methods or the other. This is our long term approach as we want to eventually get the methods on host components off the ref. So instead of calling ref.measure() you might call ReactNative.measure(ref). This would hopefully let the ref for TextInput then have the methods like .clear. Or we do it the other way and make it TextInput.clear(textInputRef) which would be fine too. Either way though is a breaking change that is longer term.

3. Mutate this ref. :( Gross, but accomplishes what we need in the meantime before we can get to the long term breaking change.

I can see what the topic is, but I don't really get why this is regarded as a problem, and thus why you'd want to "solve" it. What's wrong with the class component having instance methods like clear(), yet also keeping a reference to an underlying host component this._inputRef? if the consumer needs to access the host component, then they can just access textInputRef.current._inputRef (or via a consistently-named getter like getNativeViewRef())? It's a little extra to type, but it allows a common JS component to create convenience APIs such as clear(), (based on setting the native text prop's value to "") rather than having to re-implement the same convenience API N times for N platforms.

Equally, your ref.measure() signature could equally be achieved by making a convenience function for measure() that calls this.getNativeViewRef.measure(), so I'm not really following what's being gained.

[elicwhite]

We are making these changes to make it easier for people to adopt Fabric. Fabric doesn't have findNodeHandle so it is important for people to be able to get to the underlying native component directly. I agree that JS components could have a getter that provides that ref.

For core components we don't want people to be confused by having to know which components are native components and you can call the methods like measure directly, and which are JS components where you need to call some unknown named getter first.

For example, I think it is reasonable for people to expect that ScrollView is a native component. However, it is actually a JS component and has methods like scrollTo, but if you want to call measure you have to call getNativeScrollRef first. That means that the distinction between what is implemented in JS and what is implemented in native is part of the public API and people have to just know this.

By making the change to have the ref work as a native component but also include all the JS methods, nobody has to know the distinction.

[elicwhite]

Another example is ref.measureLayout. The ref there has to be a host component, but so does the argument it is comparing to:

textInputRef.measureLayout(
  scrollViewRef, 
  (left, top, width, height) => {
    scrollViewRef.scrollResponderScrollNativeHandleToKeyboard(
      textInputRef,
      height + 10,
  });

If we didn't do this, then if you wanted to measure a TextInput's location inside of a ScrollView you'd need to know that you actually have to do this:

textInputRef.getNativeInputRef().measureLayout(
  scrollViewRef.getNativeScrollViewRef(), 
  (left, top, width, height) => {
    scrollViewRef.scrollResponderScrollNativeHandleToKeyboard(
      textInputRef.getNativeInputRef(),
      height + 10,
  });

FWIW, we are only trying to do this in core. We aren't trying to enforce this on community components. I'm sharing our approach in case this approach is also interesting for WebView.

Back to the meta point, you should expect getScrollableNode's support in createAnimatedComponent to go away in the future.

[shirakaba]

For example, I think it is reasonable for people to expect that ScrollView is a native component. However, it is actually a JS component and has methods like scrollTo, but if you want to call measure you have to call getNativeScrollRef first. That means that the distinction between what is implemented in JS and what is implemented in native is part of the public API and people have to just know this.

True, it is hard to know which methods are the responsibility of which layer. I'm just a bit concerned that TextInput has a bit of both (clear() still exists on the class component) rather than moving all public APIs into the native component or vice versa. I'll keep watch as the changes progress.

If we didn't do this, then if you wanted to measure a TextInput's location inside of a ScrollView you'd need to know that you actually have to do this:

Agreed, I dread having to use measureLayout because it's so unclear whether a ref to a class component or native component (or even a "handle"..?) is needed. Okay, I'm on board in that sense.

Back to the meta point, you should expect getScrollableNode's support in createAnimatedComponent to go away in the future.

Got it. I'll try proceeding with the Fabric-friendly approach instead. Might have a little time to implement it right now...

[elicwhite]

TextInput has a bit of both (clear() still exists on the class component)

Can you elaborate what you mean by this? I want to make sure I understand the concern as we are going to have to articulate this in our docs at some point.

[shirakaba]

Can you elaborate what you mean by this?

If you call textInputRef.clear(), it's calling clear() from the class component instance. There's no corresponding clear() method on the host component. So if the textInputRef were to be forwarded to the host component, you'd lose access the clear() method.

I see that there's some kind of adorning process going on inside _setNativeRef, though. Is this adorning your ref to the host component with a few methods from the class component instance?

[elicwhite]

Is this adorning your ref to the host component with a few methods from the class component instance?

Yep, exactly. When you attach a ref to the TextInput, you get access to all the public methods from the JS component as well as the native methods like measure. This is why people don't have to worry about the implementation anymore. 😀

[shirakaba]

@TheSavior I had a crack at implementing it, but there was far more work involved than I realised trying to convert the Flow to TypeScript and the night is now gone. I wrote the code to support iOS, but didn't get around to Android, nor testing it out.

shirakaba/animated-support-fabric for perusal. Of note, I'm not totally sure whether getScrollableNode() should still be included or not.

Could we defer Fabric support to a separate PR and focus on this existing implementation of Animated support? Just that supporting Fabric involves rather more major changes and entails testing a new ref-forwarding approach rather than just Animated support (which I can confirm works with this present PR).

[elicwhite]

Oh, by all means. Definitely don't consider anything I'm saying blocking. You should do whatever your libary needs to do. We'll figure out the rest later as things get closer and we start helping modules migrate. 👍

[shirakaba]

Thank you for keeping this upcoming transition on our radar!

I've opened a tracking issue #1202 on Fabric support to prevent this from fizzling out; would be happy to continue the discussion on Fabric there.

[github-actions]

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

[tkieft]

Just ran into the same problem and stumbled on this PR. Given that Fabric is still not released yet, it would be great to have this (very simple) change merged as a stopgap solution, and worry about Fabric support later, when the code and release plan has been shared with the community.

[shirakaba]

@tkieft I will no longer be working on this PR as I'm busy with other open-source stuff.

Here's how far I got with refactoring to support the then-upcoming (is it still upcoming?) React Native refactor. It was largely a guess so I can't say how correct it is:

shirakaba#2

[tkieft]

Makes sense - my feelings are that this PR should just be merged as-is, and the Fabric support should be a separate PR. It doesn't make sense that this small fix was blocked on a major refactor of the core that isn't even public yet, a year and a half later.

[shirakaba]

The original worry about merging this was its use of findNodeHandle, as findNodeHandle will no longer be available in Fabric. Fortunately, with this commit (which we'll call the "getScrollableNode solution"), the implementation ceased to depend upon findNodeHandle:

a231d26

However, this problem still remains:

I don’t like the getScrollableNode solution because it is a hack to make animated think it is a list view. Also, getScrollableNode returns a react tag in the lists so this would have the same name but a different return type. A bit unfortunate.

I do agree that this could lead to an unexpected break of this library if React Native core ever changed the way list views were handled internally.

So as a user, sure, I'd argue for merging it as a stopgap solution. But from the point of view of a maintainer, I'd be understandably worried about merging a ticking time bomb that could only be defused by either introducing a regression (removing Animated support again) or catching up with Fabric (which is hard).

Not to mention, macOS WKWebView handles scrolling differently to iOS WKWebView (if I recall, it doesn't expose a UIScrollView, or doesn't extend UIScrollView), so it would have to be checked that this behaves safely on macOS (particularly if it code-shares with iOS).

I wonder if we're not far off from Fabric support, though (skimming it now, it looks like the only pending issues were identifying the accurate TypeScript typings from their Flow equivalents). If my changes from shirakaba#2 could be rebased onto the latest commit and reviewed on all platforms, that would be the ideal, future-proofed solution.

[shirakaba]

Should this discussion be moved over to the Webview repo?

@TheSavior this is the WebView repo! 😉 Unless there's another one I'm unaware of.

[sandys]

@TheSavior @shirakaba any chance this will get merged ? or is there an alternative way in the latest react native ?

P.S. also i think this issue is in a closed state and maybe not surfacing on any project management.

[shirakaba]

@sandys I don't believe there's an alternative way in the latest React Native.

I will no longer be working on this PR as I'm busy with other open-source stuff.

If someone else could push it over the line, they'd be welcome!

[sandys]

@shirakaba understood. btw - wanted to drop a line of appreciation for the work u have done. I wouldnt have even begun to think of the complexity.