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
[android][web-browser] Fix WebBrowser sending 'dismiss' before opening #6743
[android][web-browser] Fix WebBrowser sending 'dismiss' before opening #6743
Conversation
I'm looking for ways to test this in E2E 🤞 |
@mczernek I'm unfortunately blocked on this one. |
Hi @LucaColonnello, to fix the problem with SDK, you need to run An issue with the client is strange. Maybe rebase help. Moreover, please add an entry to |
8617d93
to
49f9b89
Compare
@lukmccall @mczernek @sjchmiela Can I ask for a re review here please? :) Thanks in advance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't bother about failing CI job - this particular job always fails to people from community.
I'm not sure why you have changes in yarn.lock
. Did you rebase your branch?
@@ -149,7 +149,18 @@ let _redirectHandler: ((event: RedirectEvent) => void) | null = null; | |||
// returns to active | |||
let _onWebBrowserCloseAndroid: null | (() => void) = null; | |||
|
|||
// Store previous app state to check whether the listener has ever been attached | |||
let _previousAppState: null | string = AppState.currentState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be better if we change it to bool
and rename it. We don't have to remember the previous app state. We need information if it the first call or not.
Fixes expo#6679 TL;DR A wrong usage of AppState causes WebBrowser to resolve with `{ type: 'dismiss' }` before opening the view. This is related to AppState being not initialised yet, triggering a change before the user leaves the app view to focus on the browser view.
8822e92
to
e8190f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🥇I'll merge when you fix one more thing ;)
Co-Authored-By: Łukasz Kosmaty <lukasz.kosmaty@student.uj.edu.pl>
🎉 |
This will resolve with It seems that unless we can know whether the browser is actually closed, or if it's backgrounded, then we're better off just returning |
Hey @cruzach can you please explain your point better? Unfortunately I do not understand why it would result in dismiss. Only thing I’m tackling here is that there’s no guarantee to have an initial event from AppState triggered ONLY when you do change focus. The AppState could trigger the first time is currentState is called even if no change occurred. How is this change impacting the current feature as you say? Thanks in advance :) |
Sorry, I didn't mean to indicate this PR was negatively impacting the feature at all. Since, with a Chrome custom tab, we have no way of knowing whether the web browser was closed, or backgrounded, we state in the docs that:
I think we should revert back to this behavior, since resolving to
The best option would be to tell whether the chrome custom tab was closed or not, but I'm not sure it's possible |
I can't reproduce the issue described here on SDK36 on Android. import React from "react";
import { AppState, Button, StyleSheet, Text, View } from "react-native";
import * as WebBrowser from "expo-web-browser";
export default function App() {
return (
<View style={styles.container}>
<Text>{AppState.currentState}</Text>
<Button
title="Press me"
onPress={() => {
WebBrowser.openAuthSessionAsync(
"https://expo.io",
"exp://127.0.0.1:19000"
);
}}
/>
</View>
);
}
const styles = StyleSheet.create({
container: {
flex: 1,
backgroundColor: "#fff",
alignItems: "center",
justifyContent: "center"
}
});
I tried this inside of Expo client and in a standalone app (download apk here) Can you explain how I can reproduce this problem on SDK36? |
The reason is because you call For the past few months I have been using this proposed fix in my own app and found it resolve the issue. |
Are we ok to merge this @lukmccall? :) |
@thisBrian - that doesn't make any sense to me, i must be missing something -- accessing the property does not initialize it. it is initialized in the AppState constructor as i linked to. i would need more information on exactly what the problem is and how i can reproduce this before i'm comfortable merging |
Apologies if it didn't make sense, let me clarify: your code block where you read from AppState prior to launching the auth session has side effects that won't reproduce the bug. To demonstrate I forked your helloworld repo and quickly added a simple auth flow using reddit as an example.
The same holds true for any helper classes that use WebBrowser (such as AuthSession). I hope that clears it up enough to merge the fix by @LucaColonnello, cheers! Here's an excerpt from the aformentioned fork import { AuthSession } from 'expo';
import React from 'react';
import { Button, StyleSheet, View } from 'react-native';
import { getAuthorizeUrl } from './helpers';
export default function App() {
return (
<View style={styles.container}>
<Button
title="Press me"
onPress={async () => {
const returnUrl = 'helloworld://any';
const authUrl = getAuthorizeUrl(
{
client_id: 'INSERT_YOUR_CLIENT_ID', // https://www.reddit.com/prefs/apps/ + match redirect Url with aformentioned returnUrl
duration: 'temporary',
redirect_uri: returnUrl,
response_type: 'code',
scope: ['identity'],
},
'.compact'
);
const result = await AuthSession.startAsync({
authUrl,
returnUrl,
}).catch(alert);
alert(JSON.stringify(result));
}}
/>
</View>
);
}
const styles = StyleSheet.create({
container: {
flex: 1,
backgroundColor: '#fff',
alignItems: 'center',
justifyContent: 'center',
},
}); |
@brentvatne I understand where you're coming from, it is misleading. The ReactContext (read my comment here for details of why this is important in the context of You're right to say that if that is the case it should not matter as AppState constructor creates a listener as soon as an instance is created, but there's a detail you're missing. In the context of ReactNative, all modules are exported as getter of the The above might or might not have relevance, as WebBrowser imports AppState as well, so in theory should be the same as you importing it in your app. That said, depends on how Metro bundler is running those modules (which is unclear to me, I haven't seen a JS build from Metro yet) I'm investigating what could be in this that makes it work when you call I'll keep digging as it's weird, also I think to fix this the code should be changed as the original commit, as the suggested change, which I made, checks for |
@thisBrian could you tell me how to reproduce this in a virtual machine if you did this in a VM to take the videos? Thanks :) |
Oh I had used a screen recorder for those particular videos; however Expo has some good docs on setting up a virtual device. Alternatively if you want a quick and dirty virtual Android device to run the apk, you could use BlueStacks too. I have tested it with both virtual variations and they could reproduce the bug. Apologies, if I was able to get to the office I would upload my docker image that has it all set up. |
@LucaColonnello, I'm sorry it took so long. However, I think all doubts were resolved. So, we almost ready to merge it 🎉Please, just resolve conflicts and it will be good to go. |
Hey guys, We ran into the same issue with For us the reason this failed was because of a change in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still not totally sure why this is happening but was able to repro it in a prod build of the app and not in development. thanks for the patience!
#6743) * [android][web-browser] Fix WebBrowser sending 'dismiss' before opening Fixes #6679 TL;DR A wrong usage of AppState causes WebBrowser to resolve with `{ type: 'dismiss' }` before opening the view. This is related to AppState being not initialised yet, triggering a change before the user leaves the app view to focus on the browser view. * Build packages * Updated CHANGELOG * Store only if AppState is available instead of every state change * Clean yarn.lock * Update packages/expo-web-browser/CHANGELOG.md Co-Authored-By: Łukasz Kosmaty <lukasz.kosmaty@student.uj.edu.pl> Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@student.uj.edu.pl> Co-authored-by: Brent Vatne <brentvatne@gmail.com>
this is available in |
Any way to use that in an SDK 37 app? I tried pulling it but it wasn't fixed, and |
if you already have expo-web-browser in your app 8.1.0 matches ~8.1.0 which is what is installed by expo install. if you remove it and then install it again you will get the latest version, or just run yarn add expo-web-browser@8.1.1 |
Yeah, I installed expo-web-browser@8.1.1 and rebuilt the standalone app. Still immediately get a dismiss the first time around. Expo CLI also said |
I am getting this issue with expo-web-browser 8.2.1 |
@simplesthing It seems you got your issue resolved. btw the |
Why
Fixes #6679
How
#6679 (comment)
TL;DR
A wrong usage of AppState causes WebBrowser to resolve with
{ type: 'dismiss' }
before opening the view.This is related to AppState being not initialised yet,
triggering a change before the user leaves the app view
to focus on the browser view.
Test Plan
As this can be tested only in production (the bug does not happen in development), testing is hard to achieve.
I tested it in the app I'm building, other people (see comments in the issue) did the same, linking their forked package with the patch applied to their apps, and it seems to work fine.