[android] Add rebind method #22
Conversation
|
@donaldchen this is ready for review now |
| // Since we are moving views outside of React, the layout request might be dropped | ||
| // on a normal add/remove view | ||
| // By calling .layout directly on the parent, it'll force the layout change | ||
| // See this issue for more context: https://github.com/facebook/react-native/issues/17968 |
There was a problem hiding this comment.
facebook/react-native#17968 (comment)
Unbelievable that this bug is still open.
Wow, same. Anyway, nice work on identifying this issue, and I'm glad calling layout directly fixes this!
| MeasureSpec.makeMeasureSpec(getMeasuredWidth(), MeasureSpec.EXACTLY), | ||
| MeasureSpec.makeMeasureSpec(getMeasuredHeight(), MeasureSpec.EXACTLY) | ||
| ); | ||
| parent.layout(0, 0, parent.getMeasuredWidth(), parent.getMeasuredHeight()); |
There was a problem hiding this comment.
nit: It may be slightly more readable to name the l / t / r / b parameters inline with code comments sort of like this.
parent.layout(
0 /* left */ ,
0 /* top */,
parent.getMeasuredWidth() /* right */,
parent.getMeasuredHeight() /* bottom */
);
| // The chrome client was originally setup on instance creation but might be pointing to the wrong webview | ||
| // so it's reset here. | ||
| // Not entirely sure why there is a single instance of the webchrome client for all webviews? | ||
| setupWebChromeClient((ThemedReactContext) existingWebView.getContext(), existingWebView); |
There was a problem hiding this comment.
I'm seeing this deleted but not added back anywhere. Do we still need to call setupWebChromeClient anywhere?
There was a problem hiding this comment.
Would it have to get called again in onAttachedToWindow when the webViewKey is non-null?
| view.attachWebView(webView); | ||
| // Remove the default webview on instance creation | ||
| // The actual webview will be attached | ||
| WebView activeWebview = RNCWebViewMapManager.INSTANCE.getInternalWebViewMap().get(webViewKey); |
There was a problem hiding this comment.
nit: Can we name this something like activeInternalWebView? Looking at https://github.com/discord/react-native-webview/pull/22/files#diff-e9700631fc9fa5600d2e1ee70eb2d28aa2a1fbc695b85f99981972d3904ca485R819 , it seems that sometimes we're using the name activeWebView for the internal WebView, and sometimes we use it for the RNCWebView. That makes it a little harder for me to follow the logic sometimes
There was a problem hiding this comment.
Alternatively, we could keep this one named activeWebView, and we can rename the other one to activeRncWebView
| // Attach the webview to this view | ||
| view.attachWebView(internalWebView); | ||
| // Update the map accordingly | ||
| RNCWebViewMapManager.INSTANCE.getRncWebViewMap().put(webViewKey, view); |
There was a problem hiding this comment.
Do we also need to update RNCWebViewMapManager.viewIdMap here?
| webViewKey="TEST" | ||
| source={{ uri: "https://google.ca" }} | ||
| style={{flex: 1}} | ||
|
|
There was a problem hiding this comment.
nit: we can probably delete this whitespace
| <View style={{ width: 300, height: 200, borderWidth: 2, borderColor: 'red' }}> | ||
| <WebView | ||
| ref={this.googleRef} | ||
| webViewKey="TEST" |
There was a problem hiding this comment.
nit: Should we put "TEST" in a const, since it's used in four places in this file?
| <Button | ||
| title="Bind to google" | ||
| onPress={() => { | ||
| console.log('google ref', this.googleRef); |
There was a problem hiding this comment.
nit: Is this console log still necessary?
| <Button | ||
| title="Bind to yelp" | ||
| onPress={() => { | ||
| this.yelpRef.current.rebind('TEST'); |
There was a problem hiding this comment.
For my understanding, with this Rebind example, is the expectation that between the Google and Yelp web pages, only one of them will show at a time, and then clicking the buttons will determine which web page appears (by determining which RNCWebView the internal WebView gets attached to)?
|
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 |
The goal is to add an imperative method for immediately swapping the webview to a different parent. This is helpful for race conditions where it's necessary to prevent the webview's document visibility to stay 'visible'.
There are some changes in this PR to enable this functionality:
rebindmethod to re-attach the internal webview to the parentWe'll do the iOS pr right after. The changes are just split out for ease of review. Apologies for the changes on the native side, I think there are some existing formatting issues