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

Support message passing to native webview without react tag #6

Conversation

donaldchen
Copy link

@donaldchen donaldchen commented May 26, 2022

Description

Prior to this PR, there was an issue where if I navigated away from the WebView, message passing between the WebView (via onMessage and injectJavaScript) and the react native app stopped working. When I navigated away from the WebView, the WKWebView stayed alive, but the RNCWebView got destroyed. Without the RNCWebView and without the react tag from the react component that had unmounted, we didn't have a way to send messages to the WebView because injectJavascript required both the RNCWebView and the react tag. Additionally, the onMessage callback went through a script handler delegate that depended on the RNCWebView. That also stopped working when the RNCWebView got destroyed. Technically, we could fix the message passing when navigating back to the WebView by re-adding the script handlers. However, this still has a problem where when the WKWebView is alive with no RNCWebView, we're dropping messages, and the react-native app is not handling messages posted to the WebView window.

This PR creates a solution for passing messages from the WebView to the react native app and vice versa without the react component being mounted and without the RNCWebView instance. For sending messages from the WebView to the react native app, we send the messages via an RCTEventEmitter (https://reactnative.dev/docs/native-modules-ios#sending-events-to-javascript) that includes the web view key along with the message data. The react native app can check the web view key to determine which WebView the message came from.

For sending messages to the WebView, this PR adds a new bridge API called injectJavaScriptWithWebViewKey. This method does not require a react tag, so it can be called even if there is no JS WebView component mounted. The react-native-webview objective-C code can look up the WKWebView instance with just the webViewKey and without the react tag.

Test Plan

This PR updates the sample app to test the new functionality in this PR.

Handling Messages From the WebView
In the Portals page, the WebView loads a script that posts a message to the window every second. The react native app listens for the message for the web view key and increments a counter in the UI. When we navigate away from the WebView and back to the WebView, the counter continues to increment even whether there is no JS WebView component mounted.

Filmage.2022-05-25_173226.mp4

Sending Messages to the WebView
The Portals page has a button that can inject javascript to the WKWebView even when the JS WebView component is unmounted. As a simple test, we can inject javascript that console-logs some text. Then we can use the Safari WebView inspector to verify that the log happened in the WebView.

Filmage.2022-05-26_172153.mp4

@@ -1,5 +1,3 @@
#import <WebKit/WebKit.h>
Copy link
Author

Choose a reason for hiding this comment

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

This seemed like an unnecessary import. I don't think any of the types in this file needed Webkit.

@donaldchen donaldchen marked this pull request as ready for review May 27, 2022 19:53
sharedWKWebViewDictionary[webViewKey] = nil;

RNCWebView *rncWebView = sharedRNCWebViewDictionary[webViewKey];
dispatch_async(dispatch_get_main_queue(), ^{
Copy link
Author

Choose a reason for hiding this comment

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

I updated this to always run on the main thread because I was running into this error.

Screen Shot 2022-05-25 at 4 24 47 PM

The error makes sense because cleanUpWebView removes WKWebView from its super view which probably affects the layout.

Choose a reason for hiding this comment

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

If you look at the other methods, I wonder if it might be better (just for consistency sakes) to use addUIBlock instead of doing a dispatch_async

Choose a reason for hiding this comment

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

It also queues the work on a different queue, and seems to be the natural fit here.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense! At first, I was concerned about addUIBlock providing parameters that this didn't need like the view registry. However, I realized I can just mark those parameters as unused

- (void) addScriptHandlerForMessages: (WKUserContentController *)userContentController {
if ([self shouldReuseWebView]) {
[[RNCScriptMessageManager sharedManager] addScriptMessageHandlerWithName:MessageHandlerName withUserContentController:userContentController withWebViewKey: _webViewKey];

Choose a reason for hiding this comment

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

nit random new line

- (void) removeScriptHandlerForMessages: (WKUserContentController *)userContentController {
if ([self shouldReuseWebView]) {
[[RNCScriptMessageManager sharedManager] removeScriptMessageHandlerWithUserContentController:userContentController withWebViewKey:_webViewKey];

Choose a reason for hiding this comment

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

nit new line

@@ -199,6 +200,18 @@ - (RCTUIView *)view
}];
}

RCT_EXPORT_METHOD(injectJavaScriptWithWebViewKey:(nonnull NSString *)webViewKey script:(NSString *)script)
{
dispatch_async(dispatch_get_main_queue(), ^{

Choose a reason for hiding this comment

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

use addUIBlock here too

if (_hasListeners && [[notification name] isEqualToString:kScriptMessageNotificationName]) {
NSDictionary* userInfo = notification.userInfo;
[self sendEventWithName:@"onMessage" body:userInfo];
// [self sendEventWithName:@"onMessage" body:@{ @"webViewKey": userInfo[@"webViewKey"], @"message": userInfo[@"message"]}];

Choose a reason for hiding this comment

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

remove

Copy link
Author

Choose a reason for hiding this comment

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

Will do! Good catch

@donaldchen donaldchen merged commit 1f2679a into 11.18.1-discord-1 May 31, 2022
nealmanaktola added a commit that referenced this pull request Sep 6, 2022
This PR adds messaging passing. This was much simpler to do than the iOS counter part. This is the analogue to the following PR: #6. I've followed the same test plan. The sample app was really nice to use when testing.

In the previous PR #11, I broke all native->react-native communication since events were being passed by the `InternalWebView` viewId not the `RNCWebView` viewId. This is now fixed.
nealmanaktola pushed a commit that referenced this pull request Sep 13, 2022
…w in view hierarchy

make sure release function is defined for Android WebView react component (#3)

[iOS] support releasing native WebView without a ref to the React WebView (#4)

fix EXC_BAD_ACCESS error by removing WKWebView reference to RNCWebView

Support message passing to native webview without react tag (#6)

fix EXC_BAD_ACCESS during remount situations (#7)

There are some specific edge cases where a crash currently happens
1) Let's say RNCWebview A is mounted on the screen.
2) RNCWebview B starts to mount (while A is still mounted) with the same webview key
3) RNCWebview B removes the WKWebview from A and attaches it to itself
4) RNCWebview A starts to unmount, and upon unmounting tries to remove WKWebview A from itself. Since RNCWebview B already did this step from (3), this causes a crash.

Here we only remove the view if the currently "set" RNCWebview inside the shared dict (where we keep track of the active view for a key) is self

fix segfault with webview remount (#8)

* fix another segfault

* address comments
nealmanaktola added a commit that referenced this pull request Sep 13, 2022
…bView in view hierarchy

* add support to reuse instance with webViewKey

* change up

* address comments related to layout params

* add nullability checks

* fix

* get full stack trace

* add secondary map

* rename to ifHasInternalWebView

* additional fixes

* fix

* fix boolean

[android] add messaging passing without a react tag (#12)

This PR adds messaging passing. This was much simpler to do than the iOS counter part. This is the analogue to the following PR: #6. I've followed the same test plan. The sample app was really nice to use when testing.

In the previous PR #11, I broke all native->react-native communication since events were being passed by the `InternalWebView` viewId not the `RNCWebView` viewId. This is now fixed.

[android] check source prop if changed (#14)

* [android] check source prop if changed

* detect if new source is null too
donaldchen added a commit that referenced this pull request Mar 28, 2023
…w in view hierarchy

make sure release function is defined for Android WebView react component (#3)

[iOS] support releasing native WebView without a ref to the React WebView (#4)

fix EXC_BAD_ACCESS error by removing WKWebView reference to RNCWebView

Support message passing to native webview without react tag (#6)

fix EXC_BAD_ACCESS during remount situations (#7)

There are some specific edge cases where a crash currently happens
1) Let's say RNCWebview A is mounted on the screen.
2) RNCWebview B starts to mount (while A is still mounted) with the same webview key
3) RNCWebview B removes the WKWebview from A and attaches it to itself
4) RNCWebview A starts to unmount, and upon unmounting tries to remove WKWebview A from itself. Since RNCWebview B already did this step from (3), this causes a crash.

Here we only remove the view if the currently "set" RNCWebview inside the shared dict (where we keep track of the active view for a key) is self

fix segfault with webview remount (#8)

* fix another segfault

* address comments
donaldchen pushed a commit that referenced this pull request Mar 28, 2023
…bView in view hierarchy

* add support to reuse instance with webViewKey

* change up

* address comments related to layout params

* add nullability checks

* fix

* get full stack trace

* add secondary map

* rename to ifHasInternalWebView

* additional fixes

* fix

* fix boolean

[android] add messaging passing without a react tag (#12)

This PR adds messaging passing. This was much simpler to do than the iOS counter part. This is the analogue to the following PR: #6. I've followed the same test plan. The sample app was really nice to use when testing.

In the previous PR #11, I broke all native->react-native communication since events were being passed by the `InternalWebView` viewId not the `RNCWebView` viewId. This is now fixed.

[android] check source prop if changed (#14)

* [android] check source prop if changed

* detect if new source is null too
donaldchen added a commit that referenced this pull request Mar 28, 2023
…w in view hierarchy

make sure release function is defined for Android WebView react component (#3)

[iOS] support releasing native WebView without a ref to the React WebView (#4)

fix EXC_BAD_ACCESS error by removing WKWebView reference to RNCWebView

Support message passing to native webview without react tag (#6)

fix EXC_BAD_ACCESS during remount situations (#7)

There are some specific edge cases where a crash currently happens
1) Let's say RNCWebview A is mounted on the screen.
2) RNCWebview B starts to mount (while A is still mounted) with the same webview key
3) RNCWebview B removes the WKWebview from A and attaches it to itself
4) RNCWebview A starts to unmount, and upon unmounting tries to remove WKWebview A from itself. Since RNCWebview B already did this step from (3), this causes a crash.

Here we only remove the view if the currently "set" RNCWebview inside the shared dict (where we keep track of the active view for a key) is self

fix segfault with webview remount (#8)

* fix another segfault

* address comments
donaldchen pushed a commit that referenced this pull request Mar 28, 2023
…bView in view hierarchy

* add support to reuse instance with webViewKey

* change up

* address comments related to layout params

* add nullability checks

* fix

* get full stack trace

* add secondary map

* rename to ifHasInternalWebView

* additional fixes

* fix

* fix boolean

[android] add messaging passing without a react tag (#12)

This PR adds messaging passing. This was much simpler to do than the iOS counter part. This is the analogue to the following PR: #6. I've followed the same test plan. The sample app was really nice to use when testing.

In the previous PR #11, I broke all native->react-native communication since events were being passed by the `InternalWebView` viewId not the `RNCWebView` viewId. This is now fixed.

[android] check source prop if changed (#14)

* [android] check source prop if changed

* detect if new source is null too
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants