-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 Android camera permission prompt #28619
Fix Android camera permission prompt #28619
Conversation
Generating native builds for testing |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪
|
Reviewer Checklist
Screenshots/VideosWebN/AMobile Web - ChromeN/AMobile Web - SafariN/ADesktopN/AiOS28619-ios-system-allow.mp428619-ios-system-deny.mp428619-ios-settings-allow.movAndroid28619-android-system-allow.mp428619-android-settings.mp4 |
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! Just need to further clarify that the QA steps
can be the same as the Tests
steps. : )
@ntdiary I edited the description accordingly 🙂 |
Nice, testing perfectly for me |
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.
Actually, it would be great to make one small change if possible.
When the user blocks the permission for the second time, could we NOT show the 'Camera Access' modal?
It feels a bit redundant as the user doesn't need to know this until they next try to use the camera. At that point, the modal makes total sense.
@Julesssss I agree, but this is kinda hard to achieve. I'm explaining myself: To prevent some dark patterns, the system will fire a let didNativeModalAppeared = false;
const subscription = AppState.addEventListener('blur', () => {
console.warn('AppState blur event');
didNativeModalAppeared = true;
});
CameraPermission.requestCameraPermission()
.then((status) => {
setCameraPermissionStatus(status);
subscription.remove();
if (!didNativeModalAppeared && status === RESULTS.BLOCKED) {
showPermissionsAlert();
}
})
.catch(() => {
setCameraPermissionStatus(RESULTS.UNAVAILABLE);
}); 1000000044.mp4Note that this also doesn't work with 1000000046.mp4Sure, we could check how much time it took to perform the request and show the settings alert if we think it was too fast, but it's hacky, and could lead to unexpected behavior with fast users / slow phones. That's why I recommend keeping it that way (at least on Android - as the user has to deny twice or tick "Never ask again" on older Android versions, it's not that bad). On iOS, we could add a simple check, as it's possible to determine if the permission is if (Platform.OS === 'ios' && cameraPermissionStatus === RESULTS.BLOCKED) {
return showPermissionsAlert();
}
// On Android, there's no way we can check for the BLOCKED status without requesting the permission first
// https://github.com/zoontek/react-native-permissions/blob/a836e114ce3a180b2b23916292c79841a267d828/README.md?plain=1#L670
CameraPermission.requestCameraPermission()
.then((status) => {
setCameraPermissionStatus(status);
if (Platform.OS === 'android' && cameraPermissionStatus === RESULTS.BLOCKED) {
showPermissionsAlert();
}
})
.catch(() => {
setCameraPermissionStatus(RESULTS.UNAVAILABLE);
}); WDYT? Should I add this? (I, personally, would not recommend it) |
Okay, thanks for explaining and taking a look. Given the complexity I agree we should just leave as is 👍 |
@Julesssss The linting issue does not occur locally, and this variable doesn't even exist on the branch (it has been renamed It might be a CI cache issue (?) |
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.
approve this gain, so we can try to run lint gain.
Eh, lint doesn't run again. @Julesssss, can we merge this pr? 😂 |
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
@Julesssss I merged EDIT: Fixed it, this was something else (one last missed occurence of A test fails, but it seems unrelated to the PR (it also fails on others PRs https://github.com/Expensify/App/actions/runs/6475630579/job/17582980641?pr=29242) |
Lol, fixed one unrelated check, broke another unrelated check. Second time lucky 🎉 |
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.81-2 🚀
|
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
I find it hard to trust the performance regression test 🤷 |
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.83-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
1 similar comment
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
Details
This PR simplifies the permission request flow to fix the #27442 issue and improve UX, closing #26499.
The new flow is:
undefined
(to avoid rendering an invalid UI until the check is done)useEffect
to check the status and store it in our state. As it's for camera permission, the result could beunavailable
,denied
orgranted
(notblocked
, as it's acheck()
call)granted
, display the camera. No extra step! ✨request()
call. Store the result in our state.request()
returnedblocked
, show an alert that explain why it's needed, and offers to redirect to the app settings.Fixed Issues
$ #27442
$ #26499
Tests
For each of those tests, you have to open FAB, click on "Request money", then "Scan Receipt" to be on the correct screen.
Test n°1
blocked
: an alert is shown and explain to the user why it's needed + offers him to open settings to change thatTest n°2
Test n°3
Test n°4
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android
271995029-0cd203a2-53d5-47e7-9579-c7acd4a82126.mp4